wwbmmm commented on code in PR #3575:
URL: https://github.com/apache/brpc/pull/3575#discussion_r4163486347
##########
src/brpc/policy/mysql/mysql_reply.cpp:
##########
@@ -745,34 +791,43 @@ ParseError MysqlReply::Error::Parse(butil::IOBuf& buf,
butil::Arena* arena) {
return PARSE_OK;
}
MysqlHeader header;
- if (!parse_header(buf, &header)) {
+ butil::IOBuf payload;
+ if (!parse_header(buf, &header, &payload)) {
return PARSE_ERROR_NOT_ENOUGH_DATA;
}
- buf.pop_front(1); // 0xFF
+ // error message, Null-Terminated string.
+ // payload layout: 0xFF(1) + errcode(2) + '#'(1) + sql_state(5) = 9 bytes;
+ // guard against a malformed short packet to avoid reading past the
+ // packet boundary.
+ if (header.payload_size < 9) {
+ LOG(WARNING) << "MysqlReply::Error::Parse: truncated ERR packet,
payload_size "
+ << header.payload_size << " < 9
(0xFF+errcode+'#'+sql_state)";
+ return PARSE_ERROR_ABSOLUTELY_WRONG;
+ }
+ payload.pop_front(1); // 0xFF
{
uint8_t tmp[2];
- buf.cutn(tmp, sizeof(tmp));
+ MY_PARSE_CHECK(parse_fixed(payload, tmp, sizeof(tmp)));
_errcode = mysql_uint2korr(tmp);
}
- buf.pop_front(1); // '#'
+ {
+ uint8_t sharp = 0;
+ MY_PARSE_CHECK(parse_fixed(payload, &sharp, 1)); // '#'
+ if (sharp != '#') {
+ LOG(WARNING) << "MysqlReply::Error::Parse: expected '#' before
sql_state, got "
+ << sharp;
+ return PARSE_ERROR_ABSOLUTELY_WRONG;
Review Comment:
[replied by brpc-oncall robot] Valid compatibility concern, fixed in
38b3d1e8. `Error::Parse` no longer requires the `#` marker unconditionally.
After the error code it peeks the next byte: when it is `'#'` the protocol-4.1
layout is parsed (`#` + 5-byte sql_state + message, and a truncated sql_state
is still rejected via `parse_fixed`); otherwise the packet is treated as a
pre-4.1 / initial-handshake error and the whole tail becomes the message with
an empty sql_state — the same disambiguation MySQL's own clients use, so `FF 10
04 "Too many connections"` now parses into a proper error reply (code 1040,
full message). This also removes the pre-existing `payload_size >= 9` guard,
which rejected any short pre-4.1 error packet regardless of the `#` check, and
skips the message allocation for an empty tail. The initial-handshake
regression test you asked for is `AcceptInitialHandshakeErr` (with
`is_auth=true`), alongside `AcceptProtocol41Err` (4.1 layout keeps its `42000`
sql_state)
and `RejectTruncatedSqlState` (`#` present but only 2 of 5 state bytes still
rejected). All 27 cases in `brpc_mysql_reply_parse_unittest` plus the auth
packet/handshake/scramble suites pass.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
