Copilot commented on code in PR #3575:
URL: https://github.com/apache/brpc/pull/3575#discussion_r4151358590
##########
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:
This validates against the entire shared `IOBuf`, not the current packet
payload. Because later MySQL packets may already be coalesced, a truncated
prefix can consume the next packet's header instead of returning `-1`; for
example, an OK payload `00 FC` followed by a 3-byte ERR packet is fully
consumed and `Ok::Parse` can return `PARSE_OK`. Bound decoding to
`header.payload_size` (for example, parse from a payload-only `IOBuf`) so no
field can cross a packet boundary.
##########
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:
Casting the protocol's unsigned 64-bit value to `int64_t` loses valid values
above `INT64_MAX`; they become negative on supported two's-complement targets
and are rejected as truncation. This is observable for OK-packet affected-row
and last-insert-id fields, whose storage and public getters are `uint64_t`.
Return success/failure separately from a `uint64_t` output value rather than
reserving `-1` as a sentinel.
--
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]
