From 866e67c6b5ece6f89f1468cc124c6b425f37bfb5 Mon Sep 17 00:00:00 2001 From: Bjorn Svensson Date: Thu, 26 Nov 2020 14:56:08 +0100 Subject: [PATCH] Correcting iterator leaks Leaks in multi-command handling: - redisClusterGetReply() - command_post_fragment() --- .github/workflows/ci.yml | 48 +++++----------------------------------- CMakeLists.txt | 1 + README.md | 11 ++++++--- hircluster.c | 16 ++++++++++++-- tests/ct_commands.c | 44 ++++++++++++++++++++++++++++++++++++ tests/ct_pipeline.c | 35 +++++++++++++++++++++++++++++ tests/test_utils.h | 7 ++++++ 7 files changed, 115 insertions(+), 47 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index c1f4b60..4c7f9f2 100644 --- 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 index 1880fbc..1f1f3e8 100644 --- 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 index 6711692..a490085 100644 --- 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 index 66dc08a..464cf60 100644 --- a/hircluster.c +++ b/hircluster.c @@ -2867,19 +2867,23 @@ 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; } @@ -2887,6 +2891,7 @@ static void *command_post_fragment(redisClusterContext *cc, struct cmd *command, } 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 index 687ff16..34be823 100644 --- 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 index 774e995..f999ced 100644 --- 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 index e3f0a91..fa65557 100644 --- 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