wwbmmm commented on code in PR #3575:
URL: https://github.com/apache/brpc/pull/3575#discussion_r4151513390
##########
src/brpc/policy/mysql/mysql_reply.cpp:
##########
@@ -161,32 +161,60 @@ inline bool parse_header(butil::IOBuf& buf, MysqlHeader*
value) {
return true;
}
// use this carefully, we depending on parse_header for checking IOBuf contain
full package
-inline uint64_t parse_encode_length(butil::IOBuf& buf) {
- if (buf.size() == 0) {
- return 0;
- }
-
- uint64_t value = 0;
+// Parse a MySQL length-encoded integer. Returns the decoded value, or -1
+// when the prefix byte or its 2/3/8 value bytes are not fully present in
+// |buf| (a truncated packet), or when the prefix is the invalid 0xFF marker.
+// Never reads uninitialized memory: every byte returned was cut from |buf|.
+inline int64_t parse_encode_length(butil::IOBuf& buf) {
uint8_t f = 0;
- buf.cut1((char*)&f);
+ if (!buf.cut1((char*)&f)) {
+ return -1;
+ }
if (f <= 250) {
- value = f;
- } else if (f == 251) {
- value = 0;
+ return f;
+ } else if (f == 251) { // NULL
+ return 0;
} else if (f == 252) {
uint8_t tmp[2];
- buf.cutn(tmp, sizeof(tmp));
- value = mysql_uint2korr(tmp);
+ if (buf.cutn(tmp, sizeof(tmp)) != sizeof(tmp)) {
Review Comment:
[replied by brpc-oncall robot] Valid point, fixed in the latest commit.
`parse_header` now takes an optional payload `IOBuf` and cuts the packet's
`payload_size` bytes into it right after the 4-byte header; every sub-parser
(`Auth`, `AuthMoreData`, `ResultSetHeader`, `Column`, `Ok`, `Eof`, `Error`,
`Row`, `PrepareOk::Header`) decodes fields from that bounded buffer, so a
truncated length prefix can no longer borrow bytes from a coalesced next packet
— it now fails with `PARSE_ERROR_ABSOLUTELY_WRONG`. The `old_size/new_size`
arithmetic in `Ok::Parse`/`ResultSetHeader::Parse` collapsed into simple
`payload.size()` checks. Added `RejectLenEncCrossingPacketBoundary` (an OK
packet `00 FC` followed by a coalesced packet that would have completed the
2-byte value) which previously returned `PARSE_OK` and now fails. As a side
benefit, leftover trailing bytes of a packet are now dropped with its payload
instead of desyncing the stream.
##########
src/brpc/policy/mysql/mysql_reply.cpp:
##########
@@ -161,32 +161,60 @@ inline bool parse_header(butil::IOBuf& buf, MysqlHeader*
value) {
return true;
}
// use this carefully, we depending on parse_header for checking IOBuf contain
full package
-inline uint64_t parse_encode_length(butil::IOBuf& buf) {
- if (buf.size() == 0) {
- return 0;
- }
-
- uint64_t value = 0;
+// Parse a MySQL length-encoded integer. Returns the decoded value, or -1
+// when the prefix byte or its 2/3/8 value bytes are not fully present in
+// |buf| (a truncated packet), or when the prefix is the invalid 0xFF marker.
+// Never reads uninitialized memory: every byte returned was cut from |buf|.
+inline int64_t parse_encode_length(butil::IOBuf& buf) {
uint8_t f = 0;
- buf.cut1((char*)&f);
+ if (!buf.cut1((char*)&f)) {
+ return -1;
+ }
if (f <= 250) {
- value = f;
- } else if (f == 251) {
- value = 0;
+ return f;
+ } else if (f == 251) { // NULL
+ return 0;
} else if (f == 252) {
uint8_t tmp[2];
- buf.cutn(tmp, sizeof(tmp));
- value = mysql_uint2korr(tmp);
+ if (buf.cutn(tmp, sizeof(tmp)) != sizeof(tmp)) {
+ return -1;
+ }
+ return mysql_uint2korr(tmp);
} else if (f == 253) {
uint8_t tmp[3];
- buf.cutn(tmp, sizeof(tmp));
- value = mysql_uint3korr(tmp);
+ if (buf.cutn(tmp, sizeof(tmp)) != sizeof(tmp)) {
+ return -1;
+ }
+ return mysql_uint3korr(tmp);
} else if (f == 254) {
uint8_t tmp[8];
- buf.cutn(tmp, sizeof(tmp));
- value = mysql_uint8korr(tmp);
+ if (buf.cutn(tmp, sizeof(tmp)) != sizeof(tmp)) {
+ return -1;
+ }
+ return (int64_t)mysql_uint8korr(tmp);
Review Comment:
[replied by brpc-oncall robot] Agreed, fixed in the latest commit.
`parse_encode_length` now returns `bool` with a `uint64_t*` out-param, so
values above `INT64_MAX` decode faithfully; no signed cast anywhere.
`Ok::Parse` writes straight into the `uint64_t` `_affect_row`/`_index` members.
Added `AcceptUint64MaxAffectedRows` verifying an affected-rows value of
`0xFFFFFFFFFFFFFFFF` parses successfully and round-trips through
`ok().affect_row()`.
--
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]
