This is an automated email from the ASF dual-hosted git repository.
chenBright pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/brpc.git
The following commit(s) were added to refs/heads/master by this push:
new 49f8d6f1 Fix empty %b/%s argument dropped in redis command formatting
(#3566)
49f8d6f1 is described below
commit 49f8d6f151ddbde6a2302d69a80479bc38fcac93
Author: NeutralMilkHotel <[email protected]>
AuthorDate: Wed Oct 7 17:54:10 2026 +0800
Fix empty %b/%s argument dropped in redis command formatting (#3566)
* Fix empty %b/%s argument dropped in redis command formatting (#2275)
* Fix %s/%b variadic argument type mismatch in RedisCommandFormatV
* Fix %b binary test to compare full payload bytes instead of ASSERT_STREQ
* Tighten va_arg const char* comment in RedisCommandFormatV
* Return false instead of CHECK-abort on AddCommand format errors
---
src/brpc/redis.cpp | 8 ++--
src/brpc/redis_command.cpp | 35 ++++++++++++++---
test/brpc_redis_unittest.cpp | 94 ++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 127 insertions(+), 10 deletions(-)
diff --git a/src/brpc/redis.cpp b/src/brpc/redis.cpp
index 050f28d7..a64e60bb 100644
--- a/src/brpc/redis.cpp
+++ b/src/brpc/redis.cpp
@@ -98,7 +98,7 @@ bool RedisRequest::AddCommand(const butil::StringPiece&
command) {
++_ncommand;
return true;
} else {
- CHECK(st.ok()) << st;
+ LOG(ERROR) << "RedisRequest::AddCommand failed: " << st;
_has_error = true;
return false;
}
@@ -114,7 +114,7 @@ bool RedisRequest::AddCommandByComponents(const
butil::StringPiece* components,
++_ncommand;
return true;
} else {
- CHECK(st.ok()) << st;
+ LOG(ERROR) << "RedisRequest::AddCommand failed: " << st;
_has_error = true;
return false;
}
@@ -137,7 +137,7 @@ bool RedisRequest::AddCommandWithArgs(const char* fmt, ...)
{
++_ncommand;
return true;
} else {
- CHECK(st.ok()) << st;
+ LOG(ERROR) << "RedisRequest::AddCommand failed: " << st;
_has_error = true;
return false;
}
@@ -152,7 +152,7 @@ bool RedisRequest::AddCommandV(const char* fmt, va_list ap)
{
++_ncommand;
return true;
} else {
- CHECK(st.ok()) << st;
+ LOG(ERROR) << "RedisRequest::AddCommand failed: " << st;
_has_error = true;
return false;
}
diff --git a/src/brpc/redis_command.cpp b/src/brpc/redis_command.cpp
index c0ff6477..2513d160 100644
--- a/src/brpc/redis_command.cpp
+++ b/src/brpc/redis_command.cpp
@@ -114,20 +114,25 @@ RedisCommandFormatV(butil::IOBuf* outbuf, const char*
fmt, va_list ap) {
char quote_char = 0;
const char* quote_pos = fmt;
int nargs = 0;
+ // Set after consuming %s/%b so that an empty arg still forms a component
+ // (e.g. "set key %b" with an empty value must emit 3 components, not 2).
+ bool format_arg_in_comp = false;
for (; *c; ++c) {
if (*c != '%' || c[1] == '\0') {
if (*c == ' ') {
if (quote_char) {
compbuf.push_back(*c);
- } else if (!compbuf.empty()) {
+ } else if (!compbuf.empty() || format_arg_in_comp) {
FlushComponent(&nocount_buf, &compbuf, &ncomponent);
+ format_arg_in_comp = false;
}
} else if (*c == '"' || *c == '\'') { // Check quotation.
if (!quote_char) { // begin quote
quote_char = *c;
quote_pos = c;
- if (!compbuf.empty()) {
+ if (!compbuf.empty() || format_arg_in_comp) {
FlushComponent(&nocount_buf, &compbuf, &ncomponent);
+ format_arg_in_comp = false;
}
} else if (quote_char == *c) {
const char last_char = (compbuf.empty() ? 0 :
compbuf.back());
@@ -140,6 +145,7 @@ RedisCommandFormatV(butil::IOBuf* outbuf, const char* fmt,
va_list ap) {
} else { // end quote
quote_char = 0;
FlushComponent(&nocount_buf, &compbuf, &ncomponent);
+ format_arg_in_comp = false;
}
} else {
compbuf.push_back(*c);
@@ -148,24 +154,40 @@ RedisCommandFormatV(butil::IOBuf* outbuf, const char*
fmt, va_list ap) {
compbuf.push_back(*c);
}
} else {
- char *arg;
+ const char* arg;
size_t size;
switch(c[1]) {
case 's':
- arg = va_arg(ap, char*);
+ // Most callers pass const char* (c_str()/data()); reading as
+ // const char* matches them exactly. char* is ABI-identical to
+ // const char*, so mutable callers stay correct in practice; a
+ // single va_arg cannot serve both types exactly.
+ arg = va_arg(ap, const char*);
+ // strlen(NULL) is UB; forbid it explicitly instead of
crashing.
+ if (arg == nullptr) {
+ return butil::Status(EINVAL, "%%s argument is NULL");
+ }
size = strlen(arg);
if (size > 0) {
compbuf.append(arg, size);
}
+ format_arg_in_comp = true;
++nargs;
break;
case 'b':
- arg = va_arg(ap, char*);
+ arg = va_arg(ap, const char*);
size = va_arg(ap, size_t);
+ // append(NULL, n>0) is UB; size==0 with NULL is a valid empty.
+ if (arg == nullptr && size > 0) {
+ return butil::Status(EINVAL,
+ "%%b argument is NULL with size=%lu",
+ (unsigned long)size);
+ }
if (size > 0) {
compbuf.append(arg, size);
}
+ format_arg_in_comp = true;
++nargs;
break;
case '%':
@@ -300,8 +322,9 @@ RedisCommandFormatV(butil::IOBuf* outbuf, const char* fmt,
va_list ap) {
(int)ctx_size, ctx_begin, quote_pos - fmt);
}
- if (!compbuf.empty()) {
+ if (!compbuf.empty() || format_arg_in_comp) {
FlushComponent(&nocount_buf, &compbuf, &ncomponent);
+ format_arg_in_comp = false;
}
LOG_IF(ERROR, nargs == 0) << "You must call RedisCommandNoFormat() "
diff --git a/test/brpc_redis_unittest.cpp b/test/brpc_redis_unittest.cpp
index 67ab8b98..610ea2ea 100644
--- a/test/brpc_redis_unittest.cpp
+++ b/test/brpc_redis_unittest.cpp
@@ -577,6 +577,100 @@ 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);
+ // Compare full bytes: ASSERT_STREQ would stop at the embedded NUL and
+ // miss a payload truncated after 'a'.
+ std::string expected = "*3\r\n$3\r\nset\r\n$3\r\nkey\r\n$3\r\n";
+ expected.append(bin, sizeof(bin));
+ expected.append("\r\n", 2);
+ ASSERT_EQ(expected, request._buf.to_string());
+ request.Clear();
+ }
+}
+
+// Format-only checks: do not need a running redis-server.
+TEST_F(RedisTest, empty_and_null_format_args) {
+ butil::IOBuf buf;
+ const std::string empty;
+
+ // empty %b still forms a component
+ ASSERT_TRUE(
+ brpc::RedisCommandFormat(&buf, "set key %b", empty.data(),
empty.size())
+ .ok());
+ ASSERT_STREQ("*3\r\n$3\r\nset\r\n$3\r\nkey\r\n$0\r\n\r\n",
+ buf.to_string().c_str());
+ buf.clear();
+
+ // empty %s still forms a component
+ ASSERT_TRUE(
+ brpc::RedisCommandFormat(&buf, "set key %s", empty.c_str()).ok());
+ ASSERT_STREQ("*3\r\n$3\r\nset\r\n$3\r\nkey\r\n$0\r\n\r\n",
+ buf.to_string().c_str());
+ buf.clear();
+
+ // %b with NULL + size 0 is a valid empty arg (no deref)
+ ASSERT_TRUE(brpc::RedisCommandFormat(&buf, "set key %b",
+ (const char*)nullptr, (size_t)0)
+ .ok());
+ ASSERT_STREQ("*3\r\n$3\r\nset\r\n$3\r\nkey\r\n$0\r\n\r\n",
+ buf.to_string().c_str());
+ buf.clear();
+
+ // %s with NULL must report an error instead of strlen(UB)/crash
+ ASSERT_FALSE(
+ brpc::RedisCommandFormat(&buf, "set key %s", (const char*)nullptr)
+ .ok());
+ buf.clear();
+
+ // %b with NULL + size>0 must report an error instead of append(UB)/crash
+ ASSERT_FALSE(brpc::RedisCommandFormat(&buf, "set key %b",
+ (const char*)nullptr, (size_t)3)
+ .ok());
+ buf.clear();
+
+ // multiple consecutive empty args: no state crosstalk, exact count
+ ASSERT_TRUE(brpc::RedisCommandFormat(
+ &buf, "mset %b %b %b", empty.data(), empty.size(),
+ empty.data(), empty.size(), empty.data(), empty.size())
+ .ok());
+ ASSERT_STREQ("*4\r\n$4\r\nmset\r\n$0\r\n\r\n$0\r\n\r\n$0\r\n\r\n",
+ buf.to_string().c_str());
+ buf.clear();
+}
+
+// NULL through the public AddCommand path must return false / set has_error,
+// not abort the process (the failure branch used to CHECK and crash).
+TEST_F(RedisTest, addcommand_null_returns_false) {
+ brpc::RedisRequest request;
+
+ ASSERT_FALSE(request.AddCommand("set key %s", (const char*)nullptr));
+ ASSERT_TRUE(request.has_error());
+ ASSERT_FALSE(request.AddCommand("ping")); // sticky: still false after
error
+
+ request.Clear();
+ ASSERT_FALSE(
+ request.AddCommand("set key %b", (const char*)nullptr, (size_t)3));
+ ASSERT_TRUE(request.has_error());
}
TEST_F(RedisTest, quote_and_escape) {
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
