This is an automated email from the ASF dual-hosted git repository.
wasphin 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 3f35c2c7 fix(redis): avoid formatted command overread (#3570)
3f35c2c7 is described below
commit 3f35c2c77ae327ef4dbb700b2a4f181ecba1ccce
Author: darion-yaphet <[email protected]>
AuthorDate: Sun Oct 4 22:54:53 2026 +0800
fix(redis): avoid formatted command overread (#3570)
Retry formatted values in dynamically sized storage when vsnprintf reports
an output larger than the stack buffer. Add a vector-based RedisRequest
component overload and use it for authentication and cluster command
construction while retaining the existing format APIs.
Add coverage for wide numeric formatting, binary-safe components, and
AUTH/SELECT credential encoding.
Verified with RedisCommandFormatTest.* and
RedisClusterChannelTest.basic_routing_and_multi_key_commands.
---
docs/cn/redis_client.md | 2 +-
docs/en/redis_client.md | 2 +-
src/brpc/policy/redis_authenticator.cpp | 22 +++++++++++++++-------
src/brpc/redis.cpp | 5 +++++
src/brpc/redis.h | 3 +++
src/brpc/redis_cluster.cpp | 4 ++--
src/brpc/redis_command.cpp | 22 ++++++++++++++++++++--
test/brpc_redis_unittest.cpp | 33 +++++++++++++++++++++++++++++++++
8 files changed, 80 insertions(+), 13 deletions(-)
diff --git a/docs/cn/redis_client.md b/docs/cn/redis_client.md
index 681e16f5..e4c573de 100644
--- a/docs/cn/redis_client.md
+++ b/docs/cn/redis_client.md
@@ -128,7 +128,7 @@ bool AddCommandByComponents(const butil::StringPiece*
components, size_t n);
> 注意，AddCommand和AddCommandV的fmt参数如果设置错误，有可能导致程序crash或者数据泄露，请谨慎设置。不应将受用户输入影响的内容作为fmt参数进行调用！
-AddCommandByComponents类似hiredis中的redisCommandArgv，用户通过数组指定命令中的每一个部分。这个方法对AddCommand和AddCommandV可能发生的转义问题免疫，且效率最高。如果你在使用AddCommand和AddCommandV时出现了“Unmatched
quote”，“无效格式”等问题且无法定位，可以试下这个方法。
+AddCommandByComponents类似hiredis中的redisCommandArgv，用户可通过数组或`std::vector<butil::StringPiece>`指定命令中的每个部分。这个方法对AddCommand和AddCommandV可能发生的转义问题免疫。如果你在使用AddCommand和AddCommandV时出现“Unmatched
quote”“无效格式”等问题且无法定位，可以试下这个方法。
如果AddCommand\*失败，后续的AddCommand\*和CallMethod都会失败。一般来说不用判AddCommand*的结果，失败后自然会通过RPC失败体现出来。
diff --git a/docs/en/redis_client.md b/docs/en/redis_client.md
index 8638502e..da8fa869 100644
--- a/docs/en/redis_client.md
+++ b/docs/en/redis_client.md
@@ -129,7 +129,7 @@ Formatting is compatible with hiredis, namely `%b`
corresponds to binary data (p
> Note that if the `fmt` parameter of `AddCommand` and `AddCommandV` is set
> incorrectly, it may cause the program to crash or lead to data leakage.
> Please set it with caution. Should not use user input-influenced content as
> the `fmt` parameter!
-`AddCommandByComponents` is similar to `redisCommandArgv` in hiredis. Users
specify each part of the command in an array, which is immune to escaping
issues often occurring in `AddCommand` and `AddCommandV`. If you encounter
errors such as "Unmatched quote" or "invalid format" when using `AddCommand`
and `AddCommandV`, try this method.
+`AddCommandByComponents` is similar to `redisCommandArgv` in hiredis. Users
specify each part of the command in an array or
`std::vector<butil::StringPiece>`, which is immune to escaping issues often
occurring in `AddCommand` and `AddCommandV`. If you encounter errors such as
"Unmatched quote" or "invalid format" when using `AddCommand` and
`AddCommandV`, try this method.
If `AddCommand*` fails, subsequent `AddCommand*` and `CallMethod` also fail.
In general, there is no need to check return value of `AddCommand*`, since the
RPC fails directly anyway.
diff --git a/src/brpc/policy/redis_authenticator.cpp
b/src/brpc/policy/redis_authenticator.cpp
index 152d9832..399a67b6 100644
--- a/src/brpc/policy/redis_authenticator.cpp
+++ b/src/brpc/policy/redis_authenticator.cpp
@@ -17,22 +17,30 @@
#include "brpc/policy/redis_authenticator.h"
-#include "butil/base64.h"
#include "butil/iobuf.h"
-#include "butil/string_printf.h"
-#include "butil/sys_byteorder.h"
-#include "brpc/redis_command.h"
+#include "brpc/redis.h"
namespace brpc {
namespace policy {
int RedisAuthenticator::GenerateCredential(std::string* auth_str) const {
- butil::IOBuf buf;
+ RedisRequest request;
if (!passwd_.empty()) {
- brpc::RedisCommandFormat(&buf, "AUTH %s", passwd_.c_str());
+ const std::vector<butil::StringPiece> components = {"AUTH", passwd_};
+ if (!request.AddCommandByComponents(components)) {
+ return -1;
+ }
}
if (db_ >= 0) {
- brpc::RedisCommandFormat(&buf, "SELECT %d", db_);
+ const std::string db = std::to_string(db_);
+ const std::vector<butil::StringPiece> components = {"SELECT", db};
+ if (!request.AddCommandByComponents(components)) {
+ return -1;
+ }
+ }
+ butil::IOBuf buf;
+ if (!request.SerializeTo(&buf)) {
+ return -1;
}
*auth_str = buf.to_string();
return 0;
diff --git a/src/brpc/redis.cpp b/src/brpc/redis.cpp
index d4a2108b..050f28d7 100644
--- a/src/brpc/redis.cpp
+++ b/src/brpc/redis.cpp
@@ -120,6 +120,11 @@ bool RedisRequest::AddCommandByComponents(const
butil::StringPiece* components,
}
}
+bool RedisRequest::AddCommandByComponents(
+ const std::vector<butil::StringPiece>& components) {
+ return AddCommandByComponents(components.data(), components.size());
+}
+
bool RedisRequest::AddCommandWithArgs(const char* fmt, ...) {
if (_has_error) {
return false;
diff --git a/src/brpc/redis.h b/src/brpc/redis.h
index 6a57715d..01b31c80 100644
--- a/src/brpc/redis.h
+++ b/src/brpc/redis.h
@@ -20,6 +20,7 @@
#define BRPC_REDIS_H
#include <unordered_map>
+#include <vector>
#include "brpc/destroyable.h"
#include "brpc/nonreflectable_message.h"
@@ -66,6 +67,8 @@ public:
// butil::StringPiece components[] = { "set", "key", "value" };
// request.AddCommandByComponents(components, arraysize(components));
bool AddCommandByComponents(const butil::StringPiece* components, size_t
n);
+ // Convenient when the components are already held in a vector.
+ bool AddCommandByComponents(const std::vector<butil::StringPiece>&
components);
// Add a command with variadic args to this request.
// The reason that adding so many overloads rather than using ... is that
diff --git a/src/brpc/redis_cluster.cpp b/src/brpc/redis_cluster.cpp
index c2c1ed6b..ebc0820d 100644
--- a/src/brpc/redis_cluster.cpp
+++ b/src/brpc/redis_cluster.cpp
@@ -666,7 +666,7 @@ bool RedisClusterChannel::SendToEndpoint(const std::string&
endpoint,
for (size_t i = 0; i < args.size(); ++i) {
components.push_back(args[i]);
}
- if (!request.AddCommandByComponents(&components[0],
components.size())) {
+ if (!request.AddCommandByComponents(components)) {
cntl->SetFailed(EREQUEST, "Fail to build redis command");
return false;
}
@@ -1064,7 +1064,7 @@ bool RedisClusterChannel::BuildRedisRequest(const
std::vector<std::string>& args
for (size_t i = 0; i < args.size(); ++i) {
components.push_back(args[i]);
}
- return request->AddCommandByComponents(&components[0], components.size());
+ return request->AddCommandByComponents(components);
}
void RedisClusterChannel::AppendIntegerReply(butil::IOBuf* buf, int64_t value)
{
diff --git a/src/brpc/redis_command.cpp b/src/brpc/redis_command.cpp
index 62b9bd1c..c0ff6477 100644
--- a/src/brpc/redis_command.cpp
+++ b/src/brpc/redis_command.cpp
@@ -17,6 +17,7 @@
#include <cctype>
#include <limits>
+#include <vector>
#include "butil/logging.h"
#include "brpc/log.h"
@@ -257,10 +258,27 @@ RedisCommandFormatV(butil::IOBuf* outbuf, const char*
fmt, va_list ap) {
if (_l < sizeof(_format)-2) {
memcpy(_format, c, _l);
_format[_l] = '\0';
- int plen = vsnprintf(_printed, sizeof(_printed), _format,
_cpy);
- if (plen > 0) {
+ va_list _retry;
+ va_copy(_retry, _cpy);
+ const int plen = vsnprintf(_printed, sizeof(_printed),
_format, _cpy);
+ if (plen < 0) {
+ va_end(_retry);
+ va_end(_cpy);
+ return butil::Status(EINVAL, "Failed to format redis
command");
+ }
+ if (static_cast<size_t>(plen) >= sizeof(_printed)) {
+ std::vector<char> full(static_cast<size_t>(plen) + 1);
+ const int written = vsnprintf(full.data(),
full.size(), _format, _retry);
+ if (written != plen) {
+ va_end(_retry);
+ va_end(_cpy);
+ return butil::Status(EINVAL, "Failed to format
redis command");
+ }
+ compbuf.append(full.data(), plen);
+ } else if (plen > 0) {
compbuf.append(_printed, plen);
}
+ va_end(_retry);
/* Update current position (note: outer blocks
* increment c twice so compensate here) */
c = _p - 1;
diff --git a/test/brpc_redis_unittest.cpp b/test/brpc_redis_unittest.cpp
index b1c57067..67ab8b98 100644
--- a/test/brpc_redis_unittest.cpp
+++ b/test/brpc_redis_unittest.cpp
@@ -139,6 +139,39 @@ protected:
void TearDown() {}
};
+TEST(RedisCommandFormatTest, wide_numeric_conversion) {
+ butil::IOBuf buf;
+ ASSERT_TRUE(brpc::RedisCommandFormat(&buf, "SET key %100d tail %d", 7,
9).ok());
+
+ const std::string padded_value(99, ' ');
+ const std::string expected =
+ std::string("*5\r\n$3\r\nSET\r\n$3\r\nkey\r\n$100\r\n") +
+ padded_value + "7\r\n$4\r\ntail\r\n$1\r\n9\r\n";
+ EXPECT_EQ(expected, buf.to_string());
+}
+
+TEST(RedisCommandFormatTest, vector_components_preserve_binary_payload) {
+ const std::string value("a\0b", 3);
+ const std::vector<butil::StringPiece> components = {
+ "SET", "key", butil::StringPiece(value.data(), value.size())};
+ brpc::RedisRequest request;
+ ASSERT_TRUE(request.AddCommandByComponents(components));
+
+ butil::IOBuf buf;
+ ASSERT_TRUE(request.SerializeTo(&buf));
+ std::string expected = "*3\r\n$3\r\nSET\r\n$3\r\nkey\r\n$3\r\n";
+ expected.append(value).append("\r\n");
+ EXPECT_EQ(expected, buf.to_string());
+}
+
+TEST(RedisCommandFormatTest, authenticator_encodes_password_and_database) {
+ brpc::policy::RedisAuthenticator authenticator("a b", 3);
+ std::string credential;
+ ASSERT_EQ(0, authenticator.GenerateCredential(&credential));
+ EXPECT_EQ("*2\r\n$4\r\nAUTH\r\n$3\r\na b\r\n"
+ "*2\r\n$6\r\nSELECT\r\n$1\r\n3\r\n", credential);
+}
+
void AssertReplyEqual(const brpc::RedisReply& reply1,
const brpc::RedisReply& reply2) {
if (&reply1 == &reply2) {
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
