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

MurmurIce: avoid NUL bytes in Ice messages #2909

Merged
merged 2 commits into from Mar 7, 2017

Conversation

@mkrautz
Copy link
Member

commented Mar 5, 2017

Ice messages, parameters and return values are not allowed by
the Ice spec to contain NUL bytes. Some Ice implementations
(such as C#) fail if they encounter them.

@mkrautz mkrautz force-pushed the mkrautz:ice-avoid-nul-bytes branch from cdb1a9f to 4510758 Mar 5, 2017

@mkrautz mkrautz changed the title WIP: Ice avoid nul bytes WIP: MurmurIce: avoid NUL bytes in Ice messages Mar 5, 2017

@mkrautz mkrautz force-pushed the mkrautz:ice-avoid-nul-bytes branch 3 times, most recently from d5bbad6 to b23909f Mar 5, 2017

@mkrautz mkrautz changed the title WIP: MurmurIce: avoid NUL bytes in Ice messages MurmurIce: avoid NUL bytes in Ice messages Mar 5, 2017

@mkrautz mkrautz force-pushed the mkrautz:ice-avoid-nul-bytes branch from b23909f to 9d91f3b Mar 5, 2017

@mkrautz mkrautz requested review from Kissaki, hacst and davidebeatrici Mar 5, 2017

@@ -40,15 +40,32 @@ void IceStop() {
mi = NULL;
}

/// iceRemoveNul removes all NUL bytes from |s|.

This comment has been minimized.

Copy link
@Kissaki

Kissaki Mar 5, 2017

Member

I don't think we usually write the function name again?

This comment has been minimized.

Copy link
@mkrautz

mkrautz Mar 5, 2017

Author Member

Fixed.

@@ -40,15 +40,32 @@ void IceStop() {
mi = NULL;
}

/// iceRemoveNul removes all NUL bytes from |s|.
static std::string iceRemoveNul(std::string s) {
s.erase(std::find(s.begin(), s.end(), '\0'));

This comment has been minimized.

Copy link
@Kissaki

This comment has been minimized.

Copy link
@mkrautz

mkrautz Mar 5, 2017

Author Member

Fixed.

@Kissaki

This comment has been minimized.

Copy link
Member

commented Mar 5, 2017

Typo in the first second commit message "We most important" -> "The…"

@@ -57,6 +59,20 @@ static std::string iceString(const QString &s) {
return iceRemoveNul(u8(s));
}

/// iceStringBase64 converts the Ice-safe string in std::string

This comment has been minimized.

Copy link
@Kissaki

Kissaki Mar 5, 2017

Member

Didn't jump into my eye here as much as on the other method comment, but this also names its name again

This comment has been minimized.

Copy link
@mkrautz

mkrautz Mar 5, 2017

Author Member

Fixed.

/// base64 alphabet from RFC 2045.
static std::string iceStringBase64(const std::string &s) {
if (s.size() > std::numeric_limits<int>::max()) {
return std::string();

This comment has been minimized.

Copy link
@Kissaki

Kissaki Mar 5, 2017

Member

The method comment should document this error case/error return value

This comment has been minimized.

Copy link
@mkrautz

mkrautz Mar 5, 2017

Author Member

Fixed.

@@ -57,6 +59,20 @@ static std::string iceString(const QString &s) {
return iceRemoveNul(u8(s));
}

/// iceStringBase64 converts the Ice-safe string in std::string
/// (assumed to be the output of iceString) to base64 using the

This comment has been minimized.

Copy link
@Kissaki

Kissaki Mar 5, 2017

Member

This is not correct, is it? The context does not have to be changed/cleaned because we base64 it instead to preserve it. (The change below also changes the call to not include the call to iceString)

This comment has been minimized.

Copy link
@mkrautz

mkrautz Mar 5, 2017

Author Member

Re-wrote this.

MurmurIce: avoid NUL bytes in Ice messages
Ice messages, parameters and return values are not allowed by
the Ice spec to contain NUL bytes. Some Ice implementations
(such as C#) fail if they encounter them.

@mkrautz mkrautz force-pushed the mkrautz:ice-avoid-nul-bytes branch from 9d91f3b to e24986c Mar 5, 2017

@mkrautz

This comment has been minimized.

Copy link
Member Author

commented Mar 5, 2017

Typo in the first second commit message "We most important" -> "The…"

Fixed.

@mkrautz

This comment has been minimized.

Copy link
Member Author

commented Mar 5, 2017

@Kissaki PTAL.

@Kissaki
Kissaki approved these changes Mar 5, 2017
Copy link
Member

left a comment

I did not check for completeness of iceString use.

@@ -40,15 +40,39 @@ void IceStop() {
mi = NULL;
}

/// Remove all NUL bytes from |s|.
static std::string iceRemoveNul(std::string s) {
std::vector<char> newstr;

This comment has been minimized.

Copy link
@Kissaki

Kissaki Mar 5, 2017

Member

Should reserve s.size space?

This comment has been minimized.

Copy link
@mkrautz

mkrautz Mar 5, 2017

Author Member

Sadly, we can only set the size in the constructor, not the capacity.

And we can only "fit to size" in C++11. :(

@@ -64,6 +66,23 @@ static std::string iceString(const QString &s) {
return iceRemoveNul(u8(s));
}

/// Cnvert the bytes in std::string to base64 using the

This comment has been minimized.

Copy link
@Kissaki

This comment has been minimized.

Copy link
@mkrautz

mkrautz Mar 6, 2017

Author Member

Fixed.

This comment has been minimized.

Copy link
@mkrautz

mkrautz Mar 6, 2017

Author Member

Fixed.

MurmurIce: base64-encode MurmurUser::context on the wire to avoid NUL…
… bytes w/o losing data.

We now base64-encode the user's plugin context.
It is a binary blob, not generally useful for RPC users.

The most important property is the ability to detect that two
users are in the same game context -- and that property is preserved.

If the exact byte seqeuence is needed, it is possible to base64-decode
the returned string to retrieve it.

Fixes #1874

@mkrautz mkrautz force-pushed the mkrautz:ice-avoid-nul-bytes branch from e24986c to 63bf486 Mar 6, 2017

@mkrautz

This comment has been minimized.

Copy link
Member Author

commented Mar 6, 2017

@Kissaki PTAL.

@Kissaki
Kissaki approved these changes Mar 7, 2017

@mkrautz mkrautz merged commit 4fe90ae into mumble-voip:master Mar 7, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.