Correcting iterator leaks

Leaks in multi-command handling: - redisClusterGetReply() - command_post_fragment()
Nordix · Nov 30, 2020 · 866e67c · 866e67c
1 parent 34aab7b
commit 866e67c
Show file tree

Hide file tree

Showing 7 changed files with 115 additions and 47 deletions.
diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
@@ -15,49 +15,13 @@ jobs:
     - name: Run clang-format style check (.c and .h)
       uses: jidicula/clang-format-action@master
 
-  sanitizers:
-    name: Test with -fsanitizer=${{ matrix.sanitizer }}
-    runs-on: ubuntu-latest
-    strategy:
-      fail-fast: false
-      matrix:
-        sanitizer: [thread, undefined] # LEAK TO BE CORRECTED: leak, address
-    steps:
-    - name: Prepare
-      run: sudo apt install libevent-dev
-    - uses: actions/checkout@v2
-    - name: Create build folder
-      run: cmake -E make_directory build
-    - name: Generate makefiles
-      shell: bash
-      working-directory: build
-      run: cmake $GITHUB_WORKSPACE -DCMAKE_BUILD_TYPE=$BUILD_TYPE -DENABLE_SSL=ON -DUSE_SANITIZER=${{ matrix.sanitizer }} ..
-    - name: Build
-      shell: bash
-      working-directory: build
-      run: VERBOSE=1 make
-    - name: Setup clusters
-      shell: bash
-      working-directory: build
-      run: make start
-    - name: Wait for clusters to start..
-      uses: jakejarvis/wait-action@master
-      with:
-        time: '20s'
-    - name: Run tests
-      shell: bash
-      working-directory: build
-      run: make CTEST_OUTPUT_ON_FAILURE=1 test
-    - name: Teardown clusters
-      working-directory: build
-      shell: bash
-      run: make stop
-
   build:
-    name: Build with ${{ matrix.compiler }}
+    name: Build [${{ matrix.compiler }}, sanitizer="${{ matrix.sanitizer }}"]
     runs-on: ubuntu-latest
     strategy:
+      fail-fast: false
       matrix:
+        sanitizer: ["", thread, undefined, leak, address]
         compiler: [gcc, clang]
     steps:
     - name: Prepare
@@ -66,16 +30,16 @@ jobs:
     - name: Create build folder
       run: cmake -E make_directory build
     - name: Generate makefiles
+      shell: bash
       env:
         CC: ${{ matrix.compiler }}
         CXX: ${{ matrix.compiler }}
-      shell: bash
       working-directory: build
-      run: cmake $GITHUB_WORKSPACE -DCMAKE_BUILD_TYPE=$BUILD_TYPE -DENABLE_SSL=ON ..
+      run: cmake $GITHUB_WORKSPACE -DCMAKE_BUILD_TYPE=$BUILD_TYPE -DENABLE_SSL=ON -DUSE_SANITIZER=${{ matrix.sanitizer }} ..
     - name: Build
       shell: bash
       working-directory: build
-      run: cmake --build . --config $BUILD_TYPE
+      run: VERBOSE=1 make
     - name: Setup clusters
       shell: bash
       working-directory: build

diff --git a/CMakeLists.txt b/CMakeLists.txt
@@ -2,6 +2,7 @@ cmake_minimum_required(VERSION 3.14)
 include(GNUInstallDirs)
 project(hiredis-cluster)
 
+# Options
 option(DOWNLOAD_HIREDIS  "Download the dependency hiredis from GitHub" ON)
 option(ENABLE_SSL        "Enable SSL/TLS support" OFF)
 option(DISABLE_TESTS     "Disable compilation of test" OFF)

diff --git a/README.md b/README.md
@@ -85,9 +85,14 @@ The following CMake options are available:
   * `ON` Enable IPv6 tests. Requires that IPv6 is
     [setup](https://docs.docker.com/config/daemon/ipv6/) in Docker.
 * `USE_SANITIZER`
-   Compile with a sanitizer. Options depends on
-   [compiler](https://gcc.gnu.org/onlinedocs/gcc/Instrumentation-Options.html#index-fsanitize_003daddress),
-   but usually: `address`, `thread`, `undefined`, `leak`
+   Compile using a specific sanitizer that detect issues. The value of this
+   option specifies which sanitizer to activate, but it depends on support in the
+   [compiler](https://gcc.gnu.org/onlinedocs/gcc/Instrumentation-Options.html#index-fsanitize_003daddress).
+   Common option values are: `address`, `thread`, `undefined`, `leak`
+
+Options needs to be set with the `-D` flag when generating makefiles, e.g.
+
+`cmake -DENABLE_SSL=ON -DUSE_SANITIZER=address ..`
 
 ### Build details
 

diff --git a/hircluster.c b/hircluster.c
@@ -2867,26 +2867,31 @@ static void *command_post_fragment(redisClusterContext *cc, struct cmd *command,
         sub_command = list_node->value;
         reply = sub_command->reply;
         if (reply == NULL) {
+            listReleaseIterator(list_iter);
             return NULL;
         } else if (reply->type == REDIS_REPLY_ERROR) {
+            listReleaseIterator(list_iter);
             return reply;
         }
 
         if (command->type == CMD_REQ_REDIS_MGET) {
             if (reply->type != REDIS_REPLY_ARRAY) {
                 __redisClusterSetError(cc, REDIS_ERR_OTHER, "reply type error");
+                listReleaseIterator(list_iter);
                 return NULL;
             }
         } else if (command->type == CMD_REQ_REDIS_DEL) {
             if (reply->type != REDIS_REPLY_INTEGER) {
                 __redisClusterSetError(cc, REDIS_ERR_OTHER, "reply type error");
+                listReleaseIterator(list_iter);
                 return NULL;
             }
 
             count += reply->integer;
         } else if (command->type == CMD_REQ_REDIS_EXISTS) {
             if (reply->type != REDIS_REPLY_INTEGER) {
                 __redisClusterSetError(cc, REDIS_ERR_OTHER, "reply type error");
+                listReleaseIterator(list_iter);
                 return NULL;
             }
 
@@ -2895,12 +2900,14 @@ static void *command_post_fragment(redisClusterContext *cc, struct cmd *command,
             if (reply->type != REDIS_REPLY_STATUS || reply->len != 2 ||
                 strcmp(reply->str, REDIS_STATUS_OK) != 0) {
                 __redisClusterSetError(cc, REDIS_ERR_OTHER, "reply type error");
+                listReleaseIterator(list_iter);
                 return NULL;
             }
         } else {
             NOT_REACHED();
         }
     }
+    listReleaseIterator(list_iter);
 
     reply = hi_calloc(1, sizeof(*reply));
 
@@ -3449,7 +3456,7 @@ int redisClusterGetReply(redisClusterContext *cc, void **reply) {
     struct cmd *command, *sub_command;
     hilist *commands = NULL;
     listNode *list_command, *list_sub_command;
-    listIter *list_iter;
+    listIter *list_iter = NULL;
     int slot_num;
     void *sub_reply;
 
@@ -3462,7 +3469,7 @@ int redisClusterGetReply(redisClusterContext *cc, void **reply) {
     *reply = NULL;
 
     if (cc->requests == NULL)
-        return REDIS_ERR;
+        return REDIS_ERR; // No queued requests
 
     list_command = listFirst(cc->requests);
 
@@ -3515,6 +3522,7 @@ int redisClusterGetReply(redisClusterContext *cc, void **reply) {
 
         sub_command->reply = sub_reply;
     }
+    listReleaseIterator(list_iter);
 
     *reply = command_post_fragment(cc, command, commands);
     if (*reply == NULL) {
@@ -3526,6 +3534,10 @@ int redisClusterGetReply(redisClusterContext *cc, void **reply) {
 
 error:
 
+    if (list_iter != NULL) {
+        listReleaseIterator(list_iter);
+    }
+
     listDelNode(cc->requests, list_command);
     return REDIS_ERR;
 }

diff --git a/tests/ct_commands.c b/tests/ct_commands.c
@@ -30,6 +30,48 @@ void test_exists(redisClusterContext *cc) {
     freeReplyObject(reply);
 }
 
+void test_mset(redisClusterContext *cc) {
+    redisReply *reply;
+    reply = (redisReply *)redisClusterCommand(
+        cc, "MSET key1 mset1 key2 mset2 key3 mset3");
+    CHECK_REPLY_OK(cc, reply);
+    freeReplyObject(reply);
+
+    reply = (redisReply *)redisClusterCommand(cc, "GET key1");
+    CHECK_REPLY_STR(cc, reply, "mset1");
+    freeReplyObject(reply);
+
+    reply = (redisReply *)redisClusterCommand(cc, "GET key2");
+    CHECK_REPLY_STR(cc, reply, "mset2");
+    freeReplyObject(reply);
+
+    reply = (redisReply *)redisClusterCommand(cc, "GET key3");
+    CHECK_REPLY_STR(cc, reply, "mset3");
+    freeReplyObject(reply);
+}
+
+void test_mget(redisClusterContext *cc) {
+    redisReply *reply;
+    reply = (redisReply *)redisClusterCommand(cc, "SET key1 mget1");
+    CHECK_REPLY_OK(cc, reply);
+    freeReplyObject(reply);
+
+    reply = (redisReply *)redisClusterCommand(cc, "SET key2 mget2");
+    CHECK_REPLY_OK(cc, reply);
+    freeReplyObject(reply);
+
+    reply = (redisReply *)redisClusterCommand(cc, "SET key3 mget3");
+    CHECK_REPLY_OK(cc, reply);
+    freeReplyObject(reply);
+
+    reply = (redisReply *)redisClusterCommand(cc, "MGET key1 key2 key3");
+    CHECK_REPLY_ARRAY(cc, reply, 3);
+    CHECK_REPLY_STR(cc, reply->element[0], "mget1");
+    CHECK_REPLY_STR(cc, reply->element[1], "mget2");
+    CHECK_REPLY_STR(cc, reply->element[2], "mget3");
+    freeReplyObject(reply);
+}
+
 int main() {
     struct timeval timeout = {0, 500000};
 
@@ -43,6 +85,8 @@ int main() {
     ASSERT_MSG(status == REDIS_OK, cc->errstr);
 
     test_exists(cc);
+    test_mset(cc);
+    test_mget(cc);
 
     redisClusterFree(cc);
     return 0;

diff --git a/tests/ct_pipeline.c b/tests/ct_pipeline.c
@@ -51,6 +51,39 @@ void test_pipeline() {
     redisClusterFree(cc);
 }
 
+// Test of pipelines containing multi-node commands
+void test_pipeline_with_multinode_commands() {
+    redisClusterContext *cc = redisClusterContextInit();
+    assert(cc);
+
+    int status;
+    status = redisClusterSetOptionAddNodes(cc, CLUSTER_NODE);
+    ASSERT_MSG(status == REDIS_OK, cc->errstr);
+
+    status = redisClusterConnect2(cc);
+    ASSERT_MSG(status == REDIS_OK, cc->errstr);
+
+    status = redisClusterAppendCommand(cc, "MSET key1 Hello key2 World key3 !");
+    ASSERT_MSG(status == REDIS_OK, cc->errstr);
+
+    status = redisClusterAppendCommand(cc, "MGET key1 key2 key3");
+    ASSERT_MSG(status == REDIS_OK, cc->errstr);
+
+    redisReply *reply;
+    redisClusterGetReply(cc, (void *)&reply);
+    CHECK_REPLY_OK(cc, reply);
+    freeReplyObject(reply);
+
+    redisClusterGetReply(cc, (void *)&reply);
+    CHECK_REPLY_ARRAY(cc, reply, 3);
+    CHECK_REPLY_STR(cc, reply->element[0], "Hello");
+    CHECK_REPLY_STR(cc, reply->element[1], "World");
+    CHECK_REPLY_STR(cc, reply->element[2], "!");
+    freeReplyObject(reply);
+
+    redisClusterFree(cc);
+}
+
 //------------------------------------------------------------------------------
 // Async API
 //------------------------------------------------------------------------------
@@ -125,8 +158,10 @@ void test_async_pipeline() {
 int main() {
 
     test_pipeline();
+    test_pipeline_with_multinode_commands();
 
     test_async_pipeline();
+    // Asynchronous API does not support multi-key commands
 
     return 0;
 }
diff --git a/tests/test_utils.h b/tests/test_utils.h
@@ -36,4 +36,11 @@
         ASSERT_MSG((strcmp(_reply->str, _str) == 0), _ctx->errstr);            \
     }
 
+#define CHECK_REPLY_ARRAY(_ctx, _reply, _num_of_elements)                      \
+    {                                                                          \
+        CHECK_REPLY(_ctx, _reply);                                             \
+        CHECK_REPLY_TYPE(_reply, REDIS_REPLY_ARRAY);                           \
+        ASSERT_MSG(_reply->elements == _num_of_elements, _ctx->errstr);        \
+    }
+
 #endif