Copilot commented on code in PR #3566:
URL: https://github.com/apache/brpc/pull/3566#discussion_r4100709389
##########
src/brpc/redis_command.cpp:
##########
@@ -153,18 +159,30 @@ RedisCommandFormatV(butil::IOBuf* outbuf, const char*
fmt, va_list ap) {
switch(c[1]) {
case 's':
arg = va_arg(ap, char*);
+ // strlen(NULL) is UB; forbid it explicitly instead of
crashing.
+ if (arg == nullptr) {
+ return butil::Status(EINVAL, "%%s argument is NULL");
+ }
Review Comment:
The `%s`/`%b` arguments are read as `char*`, while normal C++ callers pass
`const char*` (`c_str()`, `data()`, and the new tests). `va_arg` requires the
requested type to match the actual variadic argument type; this mismatch is
undefined behavior and makes these new null/binary tests sanitizer-unsafe. The
variadic boundary needs to be changed to preserve a correctly typed pointer (or
replaced with typed overloads).
##########
test/brpc_redis_unittest.cpp:
##########
@@ -544,6 +544,82 @@ TEST_F(RedisTest, cmd_format) {
request.AddCommand(" get key'ext' value "); // == get key ext value
ASSERT_STREQ("*4\r\n$3\r\nget\r\n$3\r\nkey\r\n$3\r\next\r\n$5\r\nvalue\r\n",
request._buf.to_string().c_str());
request.Clear();
+
+ // empty %b must still form a component (issue: empty arg was dropped)
+ {
+ std::string empty;
+ request.AddCommand("set key %b", empty.data(), empty.size());
+ ASSERT_STREQ("*3\r\n$3\r\nset\r\n$3\r\nkey\r\n$0\r\n\r\n",
+ request._buf.to_string().c_str());
+ request.Clear();
+ }
+ // empty %s must still form a component
+ {
+ std::string empty;
+ request.AddCommand("set key %s", empty.c_str());
+ ASSERT_STREQ("*3\r\n$3\r\nset\r\n$3\r\nkey\r\n$0\r\n\r\n",
+ request._buf.to_string().c_str());
+ request.Clear();
+ }
+ // %b with binary data containing \0
+ {
+ const char bin[] = {'a', '\0', 'b'};
+ request.AddCommand("set key %b", bin, (size_t)3);
+ ASSERT_STREQ("*3\r\n$3\r\nset\r\n$3\r\nkey\r\n$3\r\na\x00"
+ "b\r\n",
+ request._buf.to_string().c_str());
Review Comment:
`ASSERT_STREQ` compares C strings and stops at the embedded NUL, so this
test would still pass if the formatter truncated the payload after `a` (or
omitted `b\r\n`). Compare the serialized bytes as a `std::string` instead so
the `%b` binary-data behavior is actually covered.
--
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]
