bounds-check OnUserControlMessage like sibling control handlers#3329
bounds-check OnUserControlMessage like sibling control handlers#3329sahvx655-wq wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR hardens RTMP control-message parsing by adding a lower-bound length check to RtmpChunkStream::OnUserControlMessage, aligning it with sibling control handlers and preventing out-of-bounds reads / unsigned underflow when the message body is shorter than the 2-byte event type.
Changes:
- Reject user control messages with
message_length < 2(and keep the existing upper bound of 32). - Improve the error log to report the invalid length value.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const RtmpMessageHeader& mh, butil::IOBuf* msg_body, Socket* socket) { | ||
| if (mh.message_length > 32) { | ||
| RTMP_ERROR(socket, mh) << "No user control message long as " | ||
| if (mh.message_length < 2 || mh.message_length > 32) { |
There was a problem hiding this comment.
Good point. Pushed making both bounds 2u/32u so they line up with the != 4u / != 5u siblings and the uint32_t message_length. No behaviour change, just removes the signed/unsigned mismatch on the comparison.
|
LGTM |
|
Only change since your LGTM is the 2u/32u literal tidy-up the bot flagged, so the bounds match the != 4u / != 5u siblings in the same file. The guard logic is unchanged. |
the sibling control-message handlers (OnSetChunkSize, OnAck, OnWindowAckSize, OnSetPeerBandwidth) all validate message_length before touching the body, but OnUserControlMessage only caps the upper bound at 32. reading the code, a user control message with length 0 or 1 reads the 2-byte event type past the end of the stack buffer, and message_length - 2 underflows (uint32_t) to roughly 4G for the event_data StringPiece. require at least 2 bytes up front like the siblings do.