Copilot commented on code in PR #3310: URL: https://github.com/apache/brpc/pull/3310#discussion_r3338285509
########## src/brpc/policy/mysql/mysql_auth_packet.cpp: ########## @@ -0,0 +1,139 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +#include "brpc/policy/mysql/mysql_auth_packet.h" + +#include <cstring> + +namespace brpc { +namespace policy { +namespace mysql { + +size_t DecodeLengthEncodedInt(const butil::StringPiece& buf, uint64_t* out) { + if (buf.empty()) { + return 0; + } + const unsigned char first = static_cast<unsigned char>(buf[0]); + if (first < 0xfb) { + *out = first; + return 1; + } + if (first == 0xfc) { Review Comment: DecodeLengthEncodedInt treats the 0xFB marker as invalid, but in the MySQL protocol 0xFB represents the NULL value for length-encoded integers. Returning 0 here will cause valid packets containing NULL (common in resultsets) to be rejected/misparsed by this shared codec. ########## src/brpc/policy/mysql/mysql_auth_packet.h: ########## @@ -0,0 +1,87 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +// Wire-format helpers for the MySQL client protocol (length-encoded +// integers, length-encoded strings, packet headers) used by the +// authentication-handshake layer. Specification: +// https://dev.mysql.com/doc/dev/mysql-server/latest/page_protocol_basic_dt_integers.html +// https://dev.mysql.com/doc/dev/mysql-server/latest/page_protocol_basic_dt_strings.html +// https://dev.mysql.com/doc/dev/mysql-server/latest/page_protocol_basic_packets.html + +#ifndef BRPC_POLICY_MYSQL_MYSQL_AUTH_PACKET_H +#define BRPC_POLICY_MYSQL_MYSQL_AUTH_PACKET_H + +#include <stdint.h> + +#include <string> + +#include "butil/strings/string_piece.h" + +namespace brpc { +namespace policy { +namespace mysql { + +// MySQL packet header: 3-byte little-endian payload length + 1-byte +// sequence id. +struct PacketHeader { + uint32_t payload_len; // 0 .. (1 << 24) - 1 + uint8_t seq; +}; +static const size_t kPacketHeaderLen = 4; + +// Maximum payload length representable in a single MySQL packet +// (24-bit length field; larger payloads are split across packets). +static const uint32_t kMaxPayloadLen = (1u << 24) - 1; + +// Decodes a length-encoded integer (lenenc-int) from |buf|. +// On success, stores the value in *out and returns the number of +// bytes consumed (1, 3, 4, or 9). Returns 0 on truncation or on the +// reserved 0xff marker. Review Comment: The docstring for DecodeLengthEncodedInt currently only calls out 0xFF as special, but 0xFB is also a protocol marker (NULL). Without documenting this, callers may incorrectly treat 0xFB as an error or as an ordinary value. ########## src/brpc/policy/mysql/mysql_auth_handshake.cpp: ########## @@ -0,0 +1,229 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +#include "brpc/policy/mysql/mysql_auth_handshake.h" + +#include <cstring> + +#include "brpc/policy/mysql/mysql_auth_packet.h" +#include "brpc/policy/mysql/mysql_auth_scramble.h" + +namespace brpc { +namespace policy { +namespace mysql { + +namespace { + +// MySQL HandshakeV10 fixed-size pieces and constants. +const size_t kAuthPluginDataPart1Len = 8; +const size_t kReservedAfterCapsLen = 10; +const size_t kFillerAfterPart1Len = 1; +const size_t kReservedInResponseLen = 23; + +// Reads N little-endian bytes from |buf| at |off| into |out|. +template <typename T> +bool ReadLE(const butil::StringPiece& buf, size_t off, size_t n, T* out) { + if (off + n > buf.size()) return false; + T v = 0; + for (size_t i = 0; i < n; ++i) { + v |= static_cast<T>(static_cast<unsigned char>(buf[off + i])) << (8 * i); + } + *out = v; + return true; +} + +template <typename T> +void WriteLE(T value, size_t n, std::string* out) { + for (size_t i = 0; i < n; ++i) { + out->push_back(static_cast<char>((value >> (8 * i)) & 0xff)); + } +} + +} // namespace + +bool ParseHandshakeV10(const butil::StringPiece& payload, HandshakeV10* out) { + if (payload.empty()) return false; + + size_t off = 0; + out->protocol_version = static_cast<uint8_t>(payload[off++]); + if (out->protocol_version != kHandshakeV10Tag) { + return false; + } + + // server_version: NUL-terminated string + std::string version; + { + const butil::StringPiece rest(payload.data() + off, + payload.size() - off); + const size_t consumed = DecodeNullTerminatedString(rest, &version); + if (consumed == 0) return false; + off += consumed; + } + out->server_version = std::move(version); + + // connection_id: 4 LE bytes + if (!ReadLE<uint32_t>(payload, off, 4, &out->connection_id)) return false; + off += 4; + + // auth-plugin-data-part-1: 8 bytes + if (off + kAuthPluginDataPart1Len > payload.size()) return false; + std::string salt(payload.data() + off, kAuthPluginDataPart1Len); + off += kAuthPluginDataPart1Len; + + // filler 0x00 + if (off + kFillerAfterPart1Len > payload.size()) return false; + off += kFillerAfterPart1Len; + + // capability flags (lower 2 bytes) + uint16_t caps_lo = 0; + if (!ReadLE<uint16_t>(payload, off, 2, &caps_lo)) return false; + off += 2; + out->capability_flags = caps_lo; + + if (off == payload.size()) { + // Pre-4.1 server. We don't support these — bail. + return false; + } + + // character_set + if (off >= payload.size()) return false; + out->character_set = static_cast<uint8_t>(payload[off++]); + + // status_flags + if (!ReadLE<uint16_t>(payload, off, 2, &out->status_flags)) return false; + off += 2; + + // capability flags upper 2 bytes + uint16_t caps_hi = 0; + if (!ReadLE<uint16_t>(payload, off, 2, &caps_hi)) return false; + off += 2; + out->capability_flags |= static_cast<uint32_t>(caps_hi) << 16; + + // length of auth-plugin-data (or 0x00 when CLIENT_PLUGIN_AUTH is absent) + if (off >= payload.size()) return false; + const uint8_t apd_total_len = static_cast<uint8_t>(payload[off++]); + + // 10 reserved bytes (all 0x00) + if (off + kReservedAfterCapsLen > payload.size()) return false; + off += kReservedAfterCapsLen; + + if (out->capability_flags & CLIENT_SECURE_CONNECTION) { + // auth-plugin-data-part-2: max(13, apd_total_len - 8) bytes. Modern + // servers send 13 (12 salt bytes + 1 NUL filler). + const size_t part2_len = apd_total_len > kAuthPluginDataPart1Len + ? static_cast<size_t>(apd_total_len) - kAuthPluginDataPart1Len + : static_cast<size_t>(13); + const size_t want = part2_len < 13 ? 13 : part2_len; + if (off + want > payload.size()) return false; + // Concat salt parts; trim trailing NUL filler so callers see the + // raw 20-byte salt. + salt.append(payload.data() + off, want); + off += want; + if (!salt.empty() && salt.back() == '\0') { + salt.pop_back(); + } + } + if (salt.size() != kSaltLen) { + return false; + } + out->auth_plugin_data = std::move(salt); + + if (out->capability_flags & CLIENT_PLUGIN_AUTH) { + std::string name; + const butil::StringPiece rest(payload.data() + off, + payload.size() - off); + const size_t consumed = DecodeNullTerminatedString(rest, &name); + // Some servers omit the trailing NUL; tolerate by treating the + // remainder of the payload as the plugin name. + if (consumed == 0) { + out->auth_plugin_name.assign(rest.data(), rest.size()); + } else { + out->auth_plugin_name = std::move(name); + } + } + + return true; +} + +void BuildHandshakeResponse41(const HandshakeResponse41& req, std::string* out) { + WriteLE<uint32_t>(req.capability_flags, 4, out); + WriteLE<uint32_t>(req.max_packet_size, 4, out); + out->push_back(static_cast<char>(req.character_set)); + out->append(kReservedInResponseLen, '\0'); + out->append(req.username); + out->push_back('\0'); + + if (req.capability_flags & CLIENT_PLUGIN_AUTH_LENENC_CLIENT_DATA) { + EncodeLengthEncodedString(req.auth_response, out); + } else if (req.capability_flags & CLIENT_SECURE_CONNECTION) { + // Auth response length must fit in one byte under this scheme. + // Callers using payloads >255 bytes (e.g., RSA ciphertext) must + // set CLIENT_PLUGIN_AUTH_LENENC_CLIENT_DATA instead. + const uint8_t len = static_cast<uint8_t>( + req.auth_response.size() > 0xff ? 0xff : req.auth_response.size()); + out->push_back(static_cast<char>(len)); + out->append(req.auth_response.data(), len); Review Comment: BuildHandshakeResponse41 silently truncates auth_response to 255 bytes when using the CLIENT_SECURE_CONNECTION (1-byte length) encoding. Truncation here will produce an invalid auth response and can desynchronize the packet layout if the caller expected the full payload. Prefer making this a hard error (e.g., return bool / Status, or at least DCHECK/LOG and stop writing) rather than clamping. ########## src/brpc/policy/mysql/mysql_auth_packet.h: ########## @@ -0,0 +1,87 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +// Wire-format helpers for the MySQL client protocol (length-encoded +// integers, length-encoded strings, packet headers) used by the +// authentication-handshake layer. Specification: +// https://dev.mysql.com/doc/dev/mysql-server/latest/page_protocol_basic_dt_integers.html +// https://dev.mysql.com/doc/dev/mysql-server/latest/page_protocol_basic_dt_strings.html +// https://dev.mysql.com/doc/dev/mysql-server/latest/page_protocol_basic_packets.html + +#ifndef BRPC_POLICY_MYSQL_MYSQL_AUTH_PACKET_H +#define BRPC_POLICY_MYSQL_MYSQL_AUTH_PACKET_H + +#include <stdint.h> + +#include <string> + +#include "butil/strings/string_piece.h" + +namespace brpc { +namespace policy { +namespace mysql { + +// MySQL packet header: 3-byte little-endian payload length + 1-byte +// sequence id. +struct PacketHeader { + uint32_t payload_len; // 0 .. (1 << 24) - 1 + uint8_t seq; +}; +static const size_t kPacketHeaderLen = 4; + +// Maximum payload length representable in a single MySQL packet +// (24-bit length field; larger payloads are split across packets). +static const uint32_t kMaxPayloadLen = (1u << 24) - 1; + +// Decodes a length-encoded integer (lenenc-int) from |buf|. +// On success, stores the value in *out and returns the number of +// bytes consumed (1, 3, 4, or 9). Returns 0 on truncation or on the +// reserved 0xff marker. +size_t DecodeLengthEncodedInt(const butil::StringPiece& buf, uint64_t* out); + +// Appends a length-encoded integer encoding of |value| to |out|. +void EncodeLengthEncodedInt(uint64_t value, std::string* out); + +// Decodes a length-encoded string into |out_value| and returns the +// number of bytes consumed. Returns 0 if the leading lenenc-int is +// invalid or the declared payload is truncated. +size_t DecodeLengthEncodedString(const butil::StringPiece& buf, + std::string* out_value); Review Comment: DecodeLengthEncodedString cannot faithfully decode the protocol's NULL value: a length-encoded string with leading 0xFB represents NULL, but this API has no way to distinguish NULL from an empty string. This will matter once this codec is used for text/binary resultsets where NULL is common; consider adding an out-param (e.g. bool* is_null) or returning an optional/status type. -- 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]
