-
Notifications
You must be signed in to change notification settings - Fork 592
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
Fix buffer overflow in WireFormatting.WriteLongstr #1162
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that this length limitation only affects protocol field values, including property tables.
It should be backwards compatible for almost every environment, except maybe
for applications that use unreasonably long header values.
So this seems OK to me, although I wonder if we can improve the error message
to indicate that this is an implementation-imposed limit, not a protocol one, for example.
I'll port this to @mot256 thank you for these contributions! I'm planning to release |
@lukebakken I do not have any more that are critical ATM. The PR and #1145 are very critical to the stability of our platform. We would appreciate it if you can release soon. Thanks for your help. |
@mot256 I just published version 6.2.4 to nuget.org. It will be indexed and available very soon. Thank you again for your contributions! |
@@ -428,7 +428,7 @@ public static unsafe int WriteShortstr(Span<byte> span, string val) | |||
{ | |||
try | |||
{ | |||
int bytesWritten = Encoding.UTF8.GetBytes(chars, val.Length, bytes, maxLength); | |||
int bytesWritten = val.Length > 0 ? Encoding.UTF8.GetBytes(chars, val.Length, bytes, maxLength) : 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the purpose of the val.Length check? GetBytes returns 0 for 0 length inputs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fixed (char* chars = val)
assigns a null pointer to "chars" if "val" is an empty string and GetBytes throws an exception if "chars" is null. Hence the val.Length check to support empty strings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, correction (actually removed the val.Length to verify) if the provided span only has space for the length bytes (4) then fixed (byte* bytes = &span.Slice(4).GetPinnableReference())
produces a null pointer in "bytes".
The val.Length check is therefor to satisfy the first test case in TestWriteLongstr_BytesWritten
and TestWriteShortstr_BytesWritten
Proposed Changes
While debugging for reasons why our linux containers fail with exit code 139 (SIGSEGV), we found a buffer overflow in the "unsafe" code of RabbitMQ.Client.Impl.WireFormatting.WriteLongstr
After fixing the bug, RabbitMQ.Client.Impl.WireFormatting.WriteLongstr now works similar to RabbitMQ.Client.Impl.WireFormatting.WriteShortstr, by throwing an ArgumentOutOfRangeException when the provided string is too large for the output buffer.
This PR also fixes both RabbitMQ.Client.Impl.WireFormatting.WriteShortstr and RabbitMQ.Client.Impl.WireFormatting.WriteLongstr to allow it to write empty strings
Types of Changes
What types of changes does your code introduce to this project?
Put an
x
in the boxes that applyChecklist
Put an
x
in the boxes that apply. You can also fill these out after creatingthe PR. If you're unsure about any of them, don't hesitate to ask on the
mailing list. We're here to help! This is simply a reminder of what we are
going to look for before merging your code.
CONTRIBUTING.md
documentFurther Comments
N/A