wwbmmm commented on code in PR #3575:
URL: https://github.com/apache/brpc/pull/3575#discussion_r4163875109
##########
src/brpc/policy/mysql/mysql_reply.cpp:
##########
@@ -745,35 +791,45 @@ 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
+ // ERR payload: 0xFF(1) + error code(2), followed by either
+ // - protocol-4.1 layout: '#'(1) + sql_state(5) + message, or
+ // - pre-4.1 layout (initial-handshake errors sent before capabilities
+ // are negotiated, e.g. "Too many connections"): the message directly.
+ // Disambiguate by peeking for the '#' marker, like MySQL clients do.
+ 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); // '#'
- // 5 byte server status
- char* status = nullptr;
- MY_ALLOC_CHECK(my_alloc_check(arena, 5, status));
- buf.cutn(status, 5);
- _status.set(status, 5);
+ {
+ // IOBuf::fetch may return a pointer into its own storage instead of
+ // filling the aux buffer, so always dereference the returned pointer.
+ uint8_t aux = 0;
+ const void* peek = payload.fetch(&aux, 1);
+ if (peek != nullptr && *(const uint8_t*)peek == '#') {
Review Comment:
[replied by brpc-oncall robot] Agreed — the sniff was ambiguous. Fixed in
fe4d6402 by selecting the layout from the connection phase instead of the
message content. `ParseMysqlMessage` computes a `protocol41` flag and threads
it through `MysqlResponse/MysqlReply::ConsumePartialIOBuf` into `Error::Parse`
(new defaulted parameter, so existing callers and tests are unaffected):
- `protocol41 == false` only while the server greeting has not been
processed yet, which is detectable from the per-connection
`AuthContext::group()` staying empty — exactly the window where an ERR replaces
the greeting ("Too many connections", "Host is blocked") and uses the pre-4.1
layout. The whole tail is the message, sql_state empty.
- Once the HandshakeResponse41 (which carries CLIENT_PROTOCOL_41) has been
sent, or in the command phase, `protocol41 == true`: the `#` marker is required
and validated, and a truncated 5-byte sql_state is still rejected.
Regression tests as requested: `AcceptLegacyErrMessageStartingWithHash`
(message "#quota exceeded" parses intact with an empty sql_state, and the short
"#bad" is no longer rejected as a truncated sql_state) alongside the
protocol-4.1 control `AcceptProtocol41ErrMessageStartingWithHash` (marker +
`42000` state + message "#boom" all parsed correctly), plus the existing
`AcceptInitialHandshakeErr`, `RejectTruncatedSqlState` and
`AcceptProtocol41Err`. All 29 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]
