Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Preserve String Length When Pushing String Temporary Containing Null Char #345

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

connoranderson
Copy link

I was running some demo / example code, and serializing protobuf data. I noticed that sometimes, when the payload data contained byte \0, the subscriber would fail to parse.

This difference can easily be seen by comparing the bytes received when doing these two approaches

// APPROACH 1, remaining bytes are lost
session.put("my/topic", std::string("Hello\0World", 11));

// APPROACH 2, remaining bytes are not lost
auto str = std::string("Hello\0World", 11);
session.put("my/topic", str);

Upon further investigation, I saw that the handling of Bytes object construction differed depending on construction from temporary string std::string&& and string ref std::string&. It appears that we are converting here to char* without preserving the length of the original string, which causes the remaining bytes including the first \0 to be lost.

@connoranderson connoranderson changed the title Use reinterpret_cast + z_bytes_from_buf to preserve length Preserve String Length When Pushing String Temporary Containing Null Char Dec 19, 2024
Copy link

PR missing one of the required labels: {'bug', 'new feature', 'documentation', 'enhancement', 'breaking-change', 'internal', 'dependencies'}

@DenisBiryukov91 DenisBiryukov91 added the bug Something isn't working label Dec 20, 2024
@DenisBiryukov91
Copy link
Contributor

Hello @connoranderson . Thanks for your PR. Would it be possible for you to sign eclipse eca https://www.eclipse.org/legal/eca/ ?

@connoranderson
Copy link
Author

Hello @connoranderson . Thanks for your PR. Would it be possible for you to sign eclipse eca https://www.eclipse.org/legal/eca/ ?

Verifying with employer. Let me get back to you on that, likely after holidays.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants