Merge branch 'master' into fix_static_code_analyzer_issues

sonic-net · Aug 4, 2023 · 8031178 · 8031178
2 parents 0d52774 + be425ed
commit 8031178
Show file tree

Hide file tree

Showing 5 changed files with 77 additions and 12 deletions.
diff --git a/common/binaryserializer.h b/common/binaryserializer.h
@@ -3,6 +3,8 @@
 
 #include "common/armhelper.h"
 
+using namespace std;
+
 namespace swss {
 
 class BinarySerializer {
@@ -64,7 +66,7 @@ class BinarySerializer {
                                                                                             size);
             }
 
-            auto pkey = tmp_buffer;
+            auto pkey = string(tmp_buffer, *pkeylen);
             tmp_buffer += *pkeylen;
 
             WARNINGS_NO_CAST_ALIGN;
@@ -79,7 +81,7 @@ class BinarySerializer {
                                                                                             size);
             }
 
-            auto pval = tmp_buffer;
+            auto pval = string(tmp_buffer, *pvallen);
             tmp_buffer += *pvallen;
 
             values.push_back(std::make_pair(pkey, pval));
@@ -107,9 +109,8 @@ class BinarySerializer {
     void setKeyAndValue(const char* key, size_t klen,
                         const char* value, size_t vlen)
     {
-        // to improve deserialize performance, copy null-terminated string. 
-        setData(key, klen + 1);
-        setData(value, vlen + 1);
+        setData(key, klen);
+        setData(value, vlen);
 
         m_kvp_count++;
     }

diff --git a/common/rediscommand.cpp b/common/rediscommand.cpp
@@ -25,16 +25,18 @@ void RedisCommand::format(const char *fmt, ...)
         redisFreeCommand(temp);
         temp = nullptr;
     }
+    len = 0;
 
     va_list ap;
     va_start(ap, fmt);
-    len = redisvFormatCommand(&temp, fmt, ap);
+    int ret = redisvFormatCommand(&temp, fmt, ap);
     va_end(ap);
-    if (len == -1) {
+    if (ret == -1) {
         throw std::bad_alloc();
-    } else if (len == -2) {
+    } else if (ret == -2) {
         throw std::invalid_argument("fmt");
     }
+    len = ret;
 }
 
 void RedisCommand::formatArgv(int argc, const char **argv, const size_t *argvlen)
@@ -44,11 +46,13 @@ void RedisCommand::formatArgv(int argc, const char **argv, const size_t *argvlen
         redisFreeCommand(temp);
         temp = nullptr;
     }
+    len = 0;
 
-    len = redisFormatCommandArgv(&temp, argc, argv, argvlen);
-    if (len == -1) {
+    int ret = redisFormatCommandArgv(&temp, argc, argv, argvlen);
+    if (ret == -1) {
         throw std::bad_alloc();
     }
+    len = ret;
 }
 
 void RedisCommand::format(const vector<string> &commands)
@@ -135,6 +139,8 @@ std::string RedisCommand::toPrintableString() const
 
 const char *RedisCommand::c_str() const
 {
+    if (len == 0)
+        return nullptr;
     return temp;
 }
 

diff --git a/tests/Makefile.am b/tests/Makefile.am
@@ -5,6 +5,7 @@ LDADD_GTEST = -L/usr/src/gtest -lgtest -lgtest_main -lgmock -lgmock_main
 
 tests_tests_SOURCES = tests/redis_ut.cpp                \
                       tests/redis_piped_ut.cpp          \
+                      tests/redis_command_ut.cpp        \
                       tests/redis_state_ut.cpp          \
                       tests/redis_piped_state_ut.cpp    \
                       tests/tokenize_ut.cpp             \
@@ -44,5 +45,5 @@ tests_tests_SOURCES = tests/redis_ut.cpp                \
                       tests/main.cpp
 
 tests_tests_CFLAGS = $(DBGFLAGS) $(AM_CFLAGS) $(CFLAGS_COMMON) $(CFLAGS_GTEST) $(LIBNL_CFLAGS)
-tests_tests_CPPFLAGS = $(DBGFLAGS) $(AM_CFLAGS) $(CFLAGS_COMMON) $(CFLAGS_GTEST) $(LIBNL_CFLAGS)
+tests_tests_CPPFLAGS = $(DBGFLAGS) $(AM_CFLAGS) $(CFLAGS_COMMON) $(CFLAGS_GTEST) $(LIBNL_CFLAGS) -fno-access-control
 tests_tests_LDADD = $(LDADD_GTEST) -lpthread common/libswsscommon.la $(LIBNL_LIBS) $(CODE_COVERAGE_LIBS) sonic-db-cli/libsonicdbcli.la -lzmq -luuid -lboost_serialization
diff --git a/tests/binary_serializer_ut.cpp b/tests/binary_serializer_ut.cpp
@@ -29,7 +29,7 @@ TEST(BinarySerializer, serialize_deserialize)
                                                                 test_table);
     string serialized_str(buffer);
 
-    EXPECT_EQ(serialized_len, 107);
+    EXPECT_EQ(serialized_len, 101);
 
     auto ptr = std::make_shared<KeyOpFieldsValuesTuple>();
     KeyOpFieldsValuesTuple &kco = *ptr;
@@ -84,3 +84,48 @@ TEST(BinarySerializer, deserialize_overflow)
     auto& deserialized_values = kfvFieldsValues(kco);
     EXPECT_THROW(BinarySerializer::deserializeBuffer(buffer, serialized_len - 10, deserialized_values), runtime_error);
 }
+
+TEST(BinarySerializer, protocol_buffer)
+{
+    string test_entry_key = "test_key";
+    string test_command = "test_command";
+    string test_db = "test_db";
+    string test_table = "test_table";
+    string test_key = "key";
+    unsigned char binary_proto_buf[] = {0x0a, 0x05, 0x0d, 0x0a, 0x01, 0x00, 0x20, 0x10, 0xe1, 0x21};
+    string proto_buf_val = string((const char *)binary_proto_buf, sizeof(binary_proto_buf));
+    EXPECT_TRUE(proto_buf_val.length() == sizeof(binary_proto_buf));
+
+    char buffer[200];
+    std::vector<FieldValueTuple> values;
+    values.push_back(std::make_pair(test_key, proto_buf_val));
+    int serialized_len = (int)BinarySerializer::serializeBuffer(
+                                                                buffer,
+                                                                sizeof(buffer),
+                                                                test_entry_key,
+                                                                values,
+                                                                test_command,
+                                                                test_db,
+                                                                test_table);
+    string serialized_str(buffer);
+
+    EXPECT_EQ(serialized_len, 106);
+
+    auto ptr = std::make_shared<KeyOpFieldsValuesTuple>();
+    KeyOpFieldsValuesTuple &kco = *ptr;
+    auto& deserialized_values = kfvFieldsValues(kco);
+    BinarySerializer::deserializeBuffer(buffer, serialized_len, deserialized_values);
+
+    swss::FieldValueTuple fvt = deserialized_values.at(0);
+    EXPECT_TRUE(fvField(fvt) == test_db);
+    EXPECT_TRUE(fvValue(fvt) == test_table);
+
+    fvt = deserialized_values.at(1);
+    EXPECT_TRUE(fvField(fvt) == test_entry_key);
+    EXPECT_TRUE(fvValue(fvt) == test_command);
+
+    fvt = deserialized_values.at(2);
+    EXPECT_TRUE(fvField(fvt) == test_key);
+    EXPECT_TRUE(fvValue(fvt) == proto_buf_val);
+    EXPECT_TRUE(fvValue(fvt).length() == sizeof(binary_proto_buf));
+}
diff --git a/tests/redis_command_ut.cpp b/tests/redis_command_ut.cpp
@@ -0,0 +1,12 @@
+#include "gtest/gtest.h"
+#include "common/rediscommand.h"
+
+TEST(RedisCommand, invalid_redis_command)
+{
+    swss::RedisCommand cmd;
+    EXPECT_THROW(cmd.format("Invalid redis command %l^", 1), std::invalid_argument);
+    EXPECT_EQ(cmd.c_str(), nullptr);
+    EXPECT_EQ(cmd.length(), 0);
+    EXPECT_EQ(cmd.len, 0);
+    EXPECT_EQ(cmd.temp, nullptr);
+}