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.

Author: darion-yaphet

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失败体现出来。
 

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.
 

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;

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;

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 @@ class RedisRequest : public NonreflectableMessage<RedisRequest> {
     //   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

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) {

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;

test/brpc_redis_unittest.cpp

@@ -139,6 +139,39 @@ class RedisTest : public testing::Test {
     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) {