Skip to content

Conversation

@r-n-o
Copy link
Contributor

@r-n-o r-n-o commented Jul 21, 2025

Summary & Motivation (Problem vs. Solution)

Followup to #527 -- instead of allocating 128MB we now allocate 2MB per connection, and scale by doubling the size if the receive buffer is full. This avoids attacks where the declared size is 128MB and the connection hangs.

How I Tested These Changes

Existing unit tests.

@r-n-o r-n-o requested a review from a team as a code owner July 21, 2025 15:57
@cr-tk
Copy link
Collaborator

cr-tk commented Jul 21, 2025

@r-n-o and I went over this internally.

The receive logic and buffer resizing looks good to me.

The 2^n buffer size steps at >= 2MiB should work well without too much buffer resizing overhead, while the < 2MiB case avoids a large buffer size overhead or any resizing for short messages.

With the new PR, malicious client connections now have to provide at least 2^(n-1) byte of message body data on the wire before the QOS side allocates a new buffer size of 2^n byte, for up to n = 27 in the 128MiB case.

We clarified the documentation around the maximum allowed input size and MB<>MiB. We also discussed the validity of messages with a body length of 0, which are accepted by the current code.

Note that recv() internally uses the length of the available mutable message buffer that is known on the Rust side when translating the call to the actual recv, which avoids overflows while receiving a new chunk of data.

Overall, I think this is ready for approval and merge.

@Ulexus Ulexus merged commit a19efda into main Jul 21, 2025
8 checks passed
@Ulexus Ulexus deleted the rno/allocate-buffer-gradually branch July 21, 2025 17:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants