You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
parse_encode_length in src/brpc/policy/mysql/mysql_reply.cpp declared an uninitialized uint8_t tmp[N] (N=2/3/8) and called IOBuf::cutn without checking the return value. When a MySQL server sends a packet in which a length-encoded integer's 0xFC/0xFD/0xFE prefix is followed by fewer value bytes than promised, cutn only partially fills tmp and mysql_uint*korr then reads the uninitialized stack bytes as the value. The garbage value propagates into column counts, field lengths and loop bounds, producing unpredictable parse behavior (bogus lengths, stream desync, oversized allocations). By contrast, parse_header in the same file already validates its cutn result — this closes the protection gap.
What is changed and the side effects?
Changed:
parse_encode_length now returns int64_t and returns -1 when the prefix byte or its 2/3/8 value bytes are not fully present, or when the prefix is the invalid 0xFF marker. cut1/cutn return values are checked, so no uninitialized memory is ever interpreted.
All call sites fail fast with PARSE_ERROR_ABSOLUTELY_WRONG on a truncated value: ResultSetHeader::Parse (column count / extra message), Column::Parse (all six length-encoded strings), Ok::Parse (affected rows / last insert id), text and binary Field::Parse, and the binary TIME/DATETIME parsers.
The six duplicated length-encoded-string blocks in Column::Parse are folded into one parse_column_string helper (same checks and log messages as before).
Side effects:
Performance effects: none — the fix adds one branch per length-encoded integer on a non-hot parse path.
Breaking backward compatibility: no API changes. Packets that were previously parsed with garbage values (truncated length prefixes) are now rejected as malformed, which is the intended behavior; well-formed packets are unaffected.
Check List:
Tests: added 4 cases to test/brpc_mysql_reply_parse_unittest.cpp — truncated 0xFC/0xFD/0xFE prefixes in column definitions (the reported scenario), truncated prefixes in row fields, truncated OK packets, plus a positive multi-byte (0xFC) length test. All 9 cases in brpc_mysql_reply_parse_unittest pass, and the auth/handshake/scramble mysql unit tests still pass.
Compilable: verified with cmake (BUILD_UNIT_TESTS=ON) full build.
🤖 This PR was automatically created by brpc-oncall
parse_encode_length did not check the return value of IOBuf::cutn, so a
truncated 0xFC/0xFD/0xFE prefix left part of the stack buffer
uninitialized and mysql_uint*korr interpreted that garbage as the value.
A malicious server could turn it into bogus column counts, field lengths
or loop counts. Check cut1/cutn and return -1 on truncated or invalid
input; all call sites now fail with PARSE_ERROR_ABSOLUTELY_WRONG and the
six duplicated column-string blocks are folded into one helper.
Address review feedback on the length-encoded integer hardening:
1. parse_header now optionally cuts the packet payload into a separate
IOBuf and every sub-parser decodes from that bounded buffer, so a
truncated field can no longer borrow bytes from a coalesced next
packet and silently desync the stream.
2. parse_encode_length returns bool with a uint64_t out-param instead
of an int64_t sentinel, which rejected legitimate values above
INT64_MAX (e.g. a 2^64-1 affected-rows count in OK packets).
Add regression tests for a truncated prefix followed by a coalesced
packet and for the maximum encodable affected-rows value.
Address review feedback: the dispatcher matched the first payload byte
against synthetic MysqlRspType values, so a result set whose column
count used the multi-byte 0xFC form (252..65535 columns) was
misclassified as a prepare-ok and fed to unchecked fixed-width header
reads; 0xFB/0xFD counts hit the unknown-type fallback; and every
0xFE-leading packet was treated as EOF although MySQL only treats a
SHORT (< 9 payload bytes) 0xFE packet as EOF.
- Dispatch fresh wire bytes 0x01..0xFE to the result-set parser;
prepare-ok resume is keyed on |_type| instead of the wire byte.
- is_an_eof and the dispatcher now require payload < 9 for EOF, so a
row starting with an 8-byte length-encoded value is a row.
- All remaining unchecked fixed-width cuts (prepare-ok header, column
fixed fields, OK status/warnings, EOF, ERR sql-state, auth fixed
fields and NUL-terminated strings, binary row/field values, binary
TIME/DATETIME) go through a parse_fixed helper that rejects short
reads instead of using uninitialized stack memory.
Tests: 0xFC/251-column result sets parse; truncated 0xFC count, huge
0xFE count, 0xFE-leading row and truncated prepare-ok are rejected;
standalone EOF and a full prepare-ok still parse.
Cover the review scenario directly: a greeting ending before the 4-byte
thread id (previously left tmp partially uninitialized and continued)
and a greeting with a non-NUL-terminated server version are both
rejected.
A bare 0xFB is the length-encoded NULL marker, never a legal column
count (251 is encoded as FC FB 00). The widened fresh-dispatch range
accepted it as a zero-column result set when followed by two EOF
packets. Exclude 0xFB from result-set dispatch (it falls to the
unknown-type rejection) and reject an explicitly encoded zero column
count in ResultSetHeader::Parse; a result set always carries at least
one column.
Distinguish malformed length prefixes from oversized lengths
src/brpc/policy/mysql/mysql_reply.cpp:225
The combined condition emits an inaccurate message when the length prefix itself is truncated or is 0xFF: parse_encode_length leaves len as zero, so the log claims that “length 0 exceeds” the remaining buffer. Split prefix-decoding failure from the oversized-length check so malformed-prefix diagnostics identify the actual failure.
Rename test to reflect three-byte length encoding
test/brpc_mysql_reply_parse_unittest.cpp:378
This name says “SingleByte,” but the test deliberately verifies the three-byte 0xFC 0xFB 0x00 encoding. Rename it to reflect the multi-byte encoding so the test name does not contradict its setup and comment.
- RejectTruncatedGreeting copied only 11 of the 12 bytes of
"5.7.99-fake\x00", so it failed at the cut_until delimiter check
instead of exercising the truncated thread-id parse_fixed path it was
written for. Copy the terminating NUL.
- parse_column_string now distinguishes a truncated/invalid length
prefix from an oversized length in its log message instead of
reporting a misleading "length 0 exceeds ...".
- Rename AcceptSingleByte251ColumnCount to AcceptMultiByte251ColumnCount
to match the three-byte encoding it actually verifies.
Reject truncated column-definition filler instead of unchecked removal
src/brpc/policy/mysql/mysql_reply.cpp:710
The final two-byte column-definition filler is still removed with an unchecked pop_front. If the packet ends immediately after decimal, pop_front(2) removes zero bytes and the column is marked parsed, so this truncated fixed-width field is accepted. Consume it through parse_fixed (as with the preceding fixed fields) so the packet is rejected.
Remaining unchecked pop_front calls on fixed filler fields silently
removed zero bytes from a truncated packet and accepted it as parsed:
the column definition's filler-length byte and final 2 reserved bytes,
the auth greeting's 10 reserved bytes, the ERR packet's '#' sql_state
marker (now also validated) and the prepare-ok header's filler byte.
Consume them through parse_fixed so a truncated tail is rejected.
Add tests for a column definition missing 1..3 tail bytes and for an
ERR packet without the '#' marker.
An ERR packet sent before capabilities are negotiated (e.g. the
'Too many connections' error) carries no '#' and sql_state; the
unconditional marker check rejected these legitimate packets during
authentication. Peek for the '#' marker instead (as MySQL clients do):
when present, parse the protocol-4.1 layout and keep rejecting a
truncated 5-byte sql_state; otherwise treat the whole tail as the
message with an empty sql_state. This also drops the pre-existing
payload_size >= 9 guard, which rejected short pre-4.1 errors, and
skips the message allocation for an empty tail.
Fixes a fetch misuse found on the way: IOBuf::fetch returns a pointer
into its own storage when the range fits one block, so the marker must
be read through the returned pointer, not the aux buffer.
Tests: initial-handshake ERR parses with the full message, 4.1 ERR
keeps its sql_state, and a truncated sql_state is still rejected.
The '#' sniff could not distinguish a pre-4.1 initial-handshake error
from a 4.1 error whose message starts with '#': '#quota exceeded'
was misparsed as sql_state 'quota' + message ' exceeded', and a short
'#bad' message was rejected as a truncated sql_state. Thread a
protocol41 flag from ParseMysqlMessage through ConsumePartialIOBuf
into Error::Parse: it is false only while the server greeting has not
been processed yet (per-connection AuthContext group still empty),
which is exactly when ERR packets use the pre-4.1 layout ('Too many
connections'); after the HandshakeResponse41 and in the command phase
the 4.1 layout ('#' + sql_state) is required.
Tests: legacy messages starting with '#' (long and short) keep their
message intact with an empty sql_state, a 4.1 error whose message
starts with '#' still parses marker + sql_state, plus the existing
initial-handshake / truncated-sql-state / plain 4.1 controls.
A defaulted protocol41 parameter preserves source compatibility but
changes the mangled symbol, breaking prebuilt clients that link against
the old signatures. Restore both legacy signatures as out-of-line
wrappers that forward protocol41=true, and drop the default argument
from the new overloads so overload resolution stays unambiguous.
Add binary-row short-read tests for truncated packet data
src/brpc/policy/mysql/mysql_reply.cpp:884
The new binary-row short-read checks are not exercised by the added row tests: those use MYSQL_NORMAL_STATEMENT, and the prepared-statement integration tests consume well-formed server replies. Add synthetic MYSQL_PREPARED_STATEMENT result sets in test/brpc_mysql_reply_parse_unittest.cpp covering a truncated NULL bitmap, fixed-width numeric value, and string/TIME/DATETIME length or value. Include a coalesced following packet and a valid binary-row control. Assert PARSE_ERROR_ABSOLUTELY_WRONG for malformed rows so the tests verify that decoding cannot borrow bytes from the next packet.
The short-read hardening of the binary protocol path (NULL bitmap,
fixed-width values, string/TIME/DATETIME lengths) had no coverage:
all unit tests used the text protocol and the prepared-statement
integration tests need a live server. Add synthetic
MYSQL_PREPARED_STATEMENT result sets with a valid control row plus
truncated NULL bitmap, truncated LONGLONG value, truncated string
length prefix/value and truncated TIME/DATETIME values, each followed
by a coalesced EOF packet to verify that decoding never borrows bytes
from the next packet.
The \x00ab escape greedily consumes the following hex digits, so the
literal held 4 bytes while std::string(str, 5) copied five -- a
global-buffer-overflow caught by the ASan CI build. Split the literal
after \x00 so the escape terminates at the quote; the adjacent-literal
concatenation yields the intended 5 bytes fc 05 00 'a' 'b'.
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
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.
What problem does this PR solve?
Issue Number: resolve #N/A
Problem Summary:
parse_encode_lengthinsrc/brpc/policy/mysql/mysql_reply.cppdeclared an uninitializeduint8_t tmp[N](N=2/3/8) and calledIOBuf::cutnwithout checking the return value. When a MySQL server sends a packet in which a length-encoded integer's 0xFC/0xFD/0xFE prefix is followed by fewer value bytes than promised,cutnonly partially fillstmpandmysql_uint*korrthen reads the uninitialized stack bytes as the value. The garbage value propagates into column counts, field lengths and loop bounds, producing unpredictable parse behavior (bogus lengths, stream desync, oversized allocations). By contrast,parse_headerin the same file already validates itscutnresult — this closes the protection gap.What is changed and the side effects?
Changed:
parse_encode_lengthnow returnsint64_tand returns-1when the prefix byte or its 2/3/8 value bytes are not fully present, or when the prefix is the invalid 0xFF marker.cut1/cutnreturn values are checked, so no uninitialized memory is ever interpreted.PARSE_ERROR_ABSOLUTELY_WRONGon a truncated value:ResultSetHeader::Parse(column count / extra message),Column::Parse(all six length-encoded strings),Ok::Parse(affected rows / last insert id), text and binaryField::Parse, and the binary TIME/DATETIME parsers.Column::Parseare folded into oneparse_column_stringhelper (same checks and log messages as before).Side effects:
Check List:
test/brpc_mysql_reply_parse_unittest.cpp— truncated 0xFC/0xFD/0xFE prefixes in column definitions (the reported scenario), truncated prefixes in row fields, truncated OK packets, plus a positive multi-byte (0xFC) length test. All 9 cases inbrpc_mysql_reply_parse_unittestpass, and the auth/handshake/scramble mysql unit tests still pass.BUILD_UNIT_TESTS=ON) full build.🤖 This PR was automatically created by brpc-oncall