Leak corrections and testing

.github/workflows/ci.yml

@@ -8,44 +8,38 @@ env:
 
 jobs:
   checkers:
+    name: Run static checkers
     runs-on: ubuntu-latest
     steps:
     - uses: actions/checkout@v2
     - name: Run clang-format style check (.c and .h)
       uses: jidicula/clang-format-action@master
 
   build:
+    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
       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
       env:
         CC: ${{ matrix.compiler }}
         CXX: ${{ matrix.compiler }}
-      # Use a bash shell so we can use the same syntax for environment variable
-      # access regardless of the host operating system
-      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 CI tests
+      run: VERBOSE=1 make
     - name: Setup clusters
       shell: bash
       working-directory: build
@@ -57,7 +51,7 @@ jobs:
     - name: Run tests
       shell: bash
       working-directory: build
-      run: make test
+      run: make CTEST_OUTPUT_ON_FAILURE=1 test
     - name: Teardown clusters
       working-directory: build
       shell: bash

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)
@@ -22,6 +23,11 @@ getVersionBit(HIREDIS_CLUSTER_SONAME)
 set(VERSION "${HIREDIS_CLUSTER_MAJOR}.${HIREDIS_CLUSTER_MINOR}.${HIREDIS_CLUSTER_PATCH}")
 message("Detected version: ${VERSION}")
 
+# Build using a sanitizer
+if(USE_SANITIZER)
+    set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -fno-omit-frame-pointer -fsanitize=${USE_SANITIZER}")
+endif()
+
 project(hiredis-cluster
   VERSION "${VERSION}"
   LANGUAGES C)

README.md

@@ -84,6 +84,15 @@ The following CMake options are available:
   * `OFF` (default)
   * `ON` Enable IPv6 tests. Requires that IPv6 is
     [setup](https://docs.docker.com/config/daemon/ipv6/) in Docker.
+* `USE_SANITIZER`
+   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
 

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;
 }

tests/CMakeLists.txt

@@ -48,6 +48,8 @@ endif()
 
 # Debug mode for tests
 set(CMAKE_BUILD_TYPE "Debug" CACHE STRING "" FORCE)
+# Make sure ctest gives the output when tests fail
+list(APPEND CMAKE_CTEST_ARGUMENTS "--output-on-failure")
 
 add_executable(ct_async ct_async.c)
 target_link_libraries(ct_async hiredis_cluster hiredis ${SSL_LIBRARY} ${EVENT_LIBRARY})

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;

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;
 }

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