Repository navigation
Conversation
Two defects meant the frame size a peer sends was effectively unbounded. The limit was guarded on connection.maxMessageSize being non-zero, but that field is taken from the broker's Connected response, so it is still zero on the first read of a connection. The check therefore did not run at all until the handshake had completed. That first read happens before the peer has authenticated, and the frame size it carries decides how large a buffer readAtLeast allocates for the rest of the frame, so a peer could choose that allocation size. The comparison also converted the size to int32. Any size of 2^31 or more has its top bit set, so the conversion made it negative and the check accepted it, both before and after the handshake. Such a size is not harmless once accepted: readAtLeast computes totalFrameSize * 2 to size the buffer, which overflows uint32 and wraps to a small value, leaving the read to fail on a short buffer well after the point the size should have been refused. Fall back to MaxFrameSize while maxMessageSize is unknown, so the frame is always bounded by the protocol maximum, and keep the comparison in uint32 so no size can pass by being reinterpreted as negative. Widening maxFrameSize to uint32 also removes an int32 overflow in maxMessageSize + MessageFramePadding. Both halves are needed: bounding sizes below 2^31 is what stops the large allocation, and comparing as unsigned is what stops the largest sizes skipping the check entirely. Add a reader test that drives readSingleCommand over a net.Pipe, covering both handshake states and sizes at and above 2^31. The pipe carries a read deadline so a regression that accepts an oversized frame fails on the assertion rather than blocking in io.ReadAtLeast waiting for bytes that never arrive.
The command size is a second length read straight off the wire, and it
indexes into the frame that has just been read. Nothing checked that it
fits inside that frame before it was passed to buffer.Read.
A peer could therefore declare an unremarkable frame size and then a
command size far larger than the frame holds. With a frame size of 8 and
a command size of 4294967295, buffer.Read panics with a slice bounds
error, taking down the process rather than the connection:
panic: runtime error: slice bounds out of range [8:7]
internal.(*buffer).Read buffer.go
internal.(*connectionReader).readSingleCommand connection_reader.go
internal.(*connection).doHandshake connection.go
internal.(*connection).start.func1 connection.go
Reject a command size that does not fit within the frame, and a frame too
short to carry one at all. The comparison is written as frameSize-4 so
that neither side of it can overflow.
Add reader cases for a command size at the top of the uint32 range, one
just past the end of the frame, and a frame shorter than the command size
field itself.
The bounds check added in apache#709 for issue apache#435 adds readerIdx to the requested size and compares the sum against the length of the data. That addition is uint32 arithmetic and wraps, so a large size produces a small sum which passes the check and then panics on the slice expression below. With readerIdx at 8 and a size of 4294967295 the sum wraps to 7, 7 is not past the end of a 4096 byte buffer, and the slice becomes b.data[8:7]: panic: runtime error: slice bounds out of range [8:7] The check is therefore defeated by the values it exists to reject, which completes the fix apache#709 intended. Sizes reaching Read come straight off the wire, so this is reachable from a peer rather than theoretical. Compute the readable remainder by subtraction instead, so nothing can wrap, and report that remainder in the log line rather than the wrapped sum. Skip does not bound readerIdx, so also handle readerIdx having already moved past the end of the data. Add cases for a size past the end, a size one past the readable remainder, sizes that make the addition wrap, and a read attempted after Skip has moved readerIdx beyond the data.
The package starts goroutines for connecting, reading and the keep-alive check, and none of them guard against a panic. A panic on any of them reaches the top of its goroutine and terminates the calling process, so a fault while talking to one broker costs every other connection in that process, and any work unrelated to Pulsar that happens to share it. Add recoverPanic and defer it at the top of each goroutine this package starts. A recovered panic is logged with its stack and closes the connection, which the client already knows how to reconnect. Each goroutine needs its own guard. recover only reaches the goroutine it runs in, so the guard in start cannot cover the reader and ping-check goroutines that run starts. Close is idempotent, so the guard is safe on a connection the panicking path had already closed. This changes what a fault costs, not whether one happens. A deterministic panic becomes a reconnect loop rather than a process exit, which is why it does not replace bounding the sizes a peer can ask for.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Master Issue: relates to #435 / #709 (the buffer.Read bounds check this completes)
Motivation
The connection reader trusts two sizes a peer sends on the wire, and a panic on any connection goroutine terminates the whole process.
The frame size check was guarded on
maxMessageSize != 0, and that field is only set from the broker's Connected response. The check therefore never ran on the first read of a connection, before the peer had authenticated, and the frame size decides how large a bufferreadAtLeastallocates. The check also compared asint32, so any size of 2^31 or more turned negative and passed.The command size is a second length read off the wire, and nothing checked it fits inside the frame. A frame size of 8 with a command size of 4294967295 defeats the bounds check in
buffer.Read(thereaderIdx+sizeaddition wraps uint32) and panics withslice bounds out of range [8:7], taking down the process.Modifications
MaxFrameSizeuntil the handshake suppliesmaxMessageSize, and compare in uint32 so large sizes cannot pass as negative.cmdSize > frameSize-4so neither side of the comparison can wrap.buffer.Readbounds check to compute the readable remainder by subtraction, and refuse reads afterSkiphas movedreaderIdxpast the end of the data.recoverPanicguard on each goroutine the package starts (connect/run, reader, ping check). A recovered panic is logged with its stack and closes the connection.