From 4dbecf3f79c2202f4dc11c60e4d8d50ef8a1672e Mon Sep 17 00:00:00 2001 From: chenyang8094 Date: Tue, 26 Sep 2023 09:15:47 +0000 Subject: [PATCH] Support sanitizer test and fix uaf (#55) Add sanitizer CI and fix UAF bug caused when key and feild expire at the same time. --- .github/workflows/ci.yml | 78 ++++++++++++++++++++++++++++++++++++++++ CMakeLists.txt | 14 ++++++++ src/scan_algorithm.c | 65 +++++++++++++++++++++++++++++---- src/tairhash.c | 12 +++++-- 4 files changed, 161 insertions(+), 8 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index ec72ce5..ce47eaa 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -169,6 +169,84 @@ jobs: run: | cd build lcov -c -d ./ -o cover.info + - uses: codecov/codecov-action@v1 + with: + file: build/cover.info + token: ${{ secrets.CODECOV_TOKEN }} + verbose: true + + test-sanitizer-address-scan-mode: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v2 + - name: clone and make redis + run: | + sudo apt-get install git + git clone https://github.com/redis/redis + cd redis + git checkout 7.0 + make SANITIZER=address REDIS_CFLAGS='-Werror' BUILD_TLS=yes MALLOC=libc + - name: Install LCOV + run: | + sudo apt-get --assume-yes install lcov > /dev/null + - name: make tairhash + run: | + mkdir build + cd build + cmake ../ -DSANITIZER_MODE=address + make + - name: test + run: | + sudo apt-get install tcl8.6 tclx + work_path=$(pwd) + module_path=$work_path/lib + sed -e "s#your_path#$module_path#g" tests/tairhash.tcl > redis/tests/unit/type/tairhash.tcl + sed -i 's#unit/type/string#unit/type/tairhash#g' redis/tests/test_helper.tcl + cd redis + ./runtest --stack-logging --single unit/type/tairhash + - name: lcov collection + run: | + cd build + lcov -c -d ./ -o cover.info + - uses: codecov/codecov-action@v1 + with: + file: build/cover.info + token: ${{ secrets.CODECOV_TOKEN }} + verbose: true + + test-sanitizer-address-sort-mode: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v2 + - name: clone and make redis + run: | + sudo apt-get install git + git clone https://github.com/redis/redis + cd redis + git checkout 7.0 + make SANITIZER=address REDIS_CFLAGS='-Werror' BUILD_TLS=yes MALLOC=libc + - name: Install LCOV + run: | + sudo apt-get --assume-yes install lcov > /dev/null + - name: make tairhash + run: | + mkdir build + cd build + cmake ../ -DSANITIZER_MODE=address -DSORT_MODE=yes + make + - name: test + run: | + sudo apt-get install tcl8.6 tclx + work_path=$(pwd) + module_path=$work_path/lib + sed -e "s#your_path#$module_path#g" tests/tairhash.tcl > redis/tests/unit/type/tairhash.tcl + sed -i 's#unit/type/string#unit/type/tairhash#g' redis/tests/test_helper.tcl + cd redis + ./runtest --stack-logging --single unit/type/tairhash + - name: lcov collection + run: | + cd build + lcov -c -d ./ -o cover.info - uses: codecov/codecov-action@v1 with: file: build/cover.info diff --git a/CMakeLists.txt b/CMakeLists.txt index cafe310..9447861 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -12,6 +12,20 @@ if (GCOV_MODE) set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -fprofile-arcs -ftest-coverage") endif() +if(SANITIZER_MODE MATCHES "address") + set(CMAKE_BUILD_TYPE "DEBUG") + set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -O0 -fsanitize=address -fno-omit-frame-pointer") + set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -O0 -fsanitize=address -fno-omit-frame-pointer") +elseif(SANITIZER_MODE MATCHES "undefined") + set(CMAKE_BUILD_TYPE "DEBUG") + set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -O0 -fsanitize=undefined -fno-omit-frame-pointer") + set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -O0 -fsanitize=undefined -fno-omit-frame-pointer") +elseif(SANITIZER_MODE MATCHES "thread") + set(CMAKE_BUILD_TYPE "DEBUG") + set(CMAKE_C_FLAGS "${CMAKE_C_FLAGS} -O0 -fsanitize=thread -fno-omit-frame-pointer") + set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -O0 -fsanitize=thread -fno-omit-frame-pointer") +endif(SANITIZER_MODE MATCHES "address") + set(CMAKE_CXX_VISIBILITY_PRESET hidden) set(CMAKE_C_VISIBILITY_PRESET hidden) diff --git a/src/scan_algorithm.c b/src/scan_algorithm.c index b3d1446..5892dab 100644 --- a/src/scan_algorithm.c +++ b/src/scan_algorithm.c @@ -55,6 +55,7 @@ void activeExpire(RedisModuleCtx *ctx, int dbid, uint64_t keys_per_loop) { RedisModuleString *key, *field; RedisModuleKey *real_key; + int may_delkey = 0; long long when, now; unsigned long zsl_len; @@ -79,8 +80,7 @@ void activeExpire(RedisModuleCtx *ctx, int dbid, uint64_t keys_per_loop) { Module_Assert(RedisModule_CallReplyType(keys_reply) == REDISMODULE_REPLY_ARRAY); size_t keynum = RedisModule_CallReplyLength(keys_reply); - int j; - for (j = 0; j < keynum; j++) { + for (int j = 0; j < keynum; j++) { RedisModuleCallReply *key_reply = RedisModule_CallReplyArrayElement(keys_reply, j); Module_Assert(RedisModule_CallReplyType(key_reply) == REDISMODULE_REPLY_STRING); key = RedisModule_CreateStringFromCallReply(key_reply); @@ -89,8 +89,10 @@ void activeExpire(RedisModuleCtx *ctx, int dbid, uint64_t keys_per_loop) { return REDISMODULE_KEYTYPE_EMPTY here, so we must deal with it until after this bugfix: https://github.com/redis/redis/commit/1833d008b3af8628835b5f082c5b4b1359557893 */ if (RedisModule_KeyType(real_key) == REDISMODULE_KEYTYPE_EMPTY) { + RedisModule_CloseKey(real_key); continue; } + if (RedisModule_ModuleTypeGetType(real_key) == TairHashType) { tair_hash_obj = RedisModule_ModuleTypeGetValue(real_key); if (tair_hash_obj->expire_index->length > 0) { @@ -117,6 +119,7 @@ void activeExpire(RedisModuleCtx *ctx, int dbid, uint64_t keys_per_loop) { m_listNode *node; while ((node = listFirst(keys)) != NULL) { key = listNodeValue(node); + may_delkey = 0; real_key = RedisModule_OpenKey(ctx, key, REDISMODULE_READ | REDISMODULE_WRITE | REDISMODULE_OPEN_KEY_NOTOUCH); int type = RedisModule_KeyType(real_key); if (type != REDISMODULE_KEYTYPE_EMPTY) { @@ -124,10 +127,20 @@ void activeExpire(RedisModuleCtx *ctx, int dbid, uint64_t keys_per_loop) { } else { /* Note: redis scan may return dup key. */ m_listDelNode(keys, node); + RedisModule_CloseKey(real_key); continue; } + mstime_t key_ttl = RedisModule_GetExpire(real_key); + if (key_ttl != REDISMODULE_NO_EXPIRE && key_ttl < 1000) { // + may_delkey = 1; + } + tair_hash_obj = RedisModule_ModuleTypeGetValue(real_key); + if (dictSize(tair_hash_obj->hash) == 1) { + may_delkey = 1; + } + RedisModule_CloseKey(real_key); zsl_len = tair_hash_obj->expire_index->length; Module_Assert(zsl_len > 0); @@ -140,15 +153,30 @@ void activeExpire(RedisModuleCtx *ctx, int dbid, uint64_t keys_per_loop) { g_expire_algorithm.stat_active_expired_field[dbid]++; start_index++; expire_keys_per_loop--; + if (may_delkey) { + break; + } } else { break; } ln2 = ln2->level[0].forward; } + // If the key happens to expire, it will be released in fieldExpireIfNeeded. + if (may_delkey) { + real_key = RedisModule_OpenKey(ctx, key, REDISMODULE_READ | REDISMODULE_OPEN_KEY_NOTOUCH); + int type = RedisModule_KeyType(real_key); + if (type == REDISMODULE_KEYTYPE_EMPTY) { + m_listDelNode(keys, node); + RedisModule_CloseKey(real_key); + continue; + } + RedisModule_CloseKey(real_key); + } + if (start_index) { m_zslDeleteRangeByRank(tair_hash_obj->expire_index, 1, start_index); - delEmptyTairHashIfNeeded(ctx, real_key, key, tair_hash_obj); + delEmptyTairHashIfNeeded(ctx, NULL, key, tair_hash_obj); } m_listDelNode(keys, node); } @@ -164,6 +192,7 @@ void passiveExpire(RedisModuleCtx *ctx, int dbid, RedisModuleString *key) { RedisModuleString *field; RedisModuleKey *real_key; unsigned long zsl_len; + int may_delkey = 0; /* 1. The current db does not have a key that needs to expire. */ list *keys = m_listCreate(); @@ -186,6 +215,7 @@ void passiveExpire(RedisModuleCtx *ctx, int dbid, RedisModuleString *key) { m_listNode *node; while ((node = listFirst(keys)) != NULL) { key = listNodeValue(node); + may_delkey = 0; real_key = RedisModule_OpenKey(ctx, key, REDISMODULE_READ | REDISMODULE_WRITE); int type = RedisModule_KeyType(real_key); @@ -195,6 +225,17 @@ void passiveExpire(RedisModuleCtx *ctx, int dbid, RedisModuleString *key) { zsl_len = tair_hash_obj->expire_index->length; Module_Assert(zsl_len > 0); + mstime_t key_ttl = RedisModule_GetExpire(real_key); + if (key_ttl != REDISMODULE_NO_EXPIRE && key_ttl < 100) { // 100ms + may_delkey = 1; + } + + tair_hash_obj = RedisModule_ModuleTypeGetValue(real_key); + if (dictSize(tair_hash_obj->hash) == 1) { + may_delkey = 1; + } + RedisModule_CloseKey(real_key); + start_index = 0; ln = tair_hash_obj->expire_index->header->level[0].forward; while (ln && keys_per_loop) { @@ -203,17 +244,29 @@ void passiveExpire(RedisModuleCtx *ctx, int dbid, RedisModuleString *key) { g_expire_algorithm.stat_passive_expired_field[dbid]++; start_index++; keys_per_loop--; + if (may_delkey) { + break; + } } else { break; } ln = ln->level[0].forward; } + if (may_delkey) { + real_key = RedisModule_OpenKey(ctx, key, REDISMODULE_READ | REDISMODULE_OPEN_KEY_NOTOUCH); + int type = RedisModule_KeyType(real_key); + if (type == REDISMODULE_KEYTYPE_EMPTY) { + m_listDelNode(keys, node); + RedisModule_CloseKey(real_key); + continue; + } + RedisModule_CloseKey(real_key); + } + if (start_index) { m_zslDeleteRangeByRank(tair_hash_obj->expire_index, 1, start_index); - if (!delEmptyTairHashIfNeeded(ctx, real_key, key, tair_hash_obj)) { - RedisModule_CloseKey(real_key); - } + delEmptyTairHashIfNeeded(ctx, NULL, key, tair_hash_obj); } m_listDelNode(keys, node); } diff --git a/src/tairhash.c b/src/tairhash.c index bd97c84..aa9035b 100755 --- a/src/tairhash.c +++ b/src/tairhash.c @@ -55,13 +55,13 @@ void _moduleAssert(const char *estr, const char *file, int line) { *((char *)-1) = 'x'; } -inline struct TairHashVal *createTairHashVal(void) { +struct TairHashVal *createTairHashVal(void) { struct TairHashVal *o; o = RedisModule_Calloc(1, sizeof(*o)); return o; } -inline void tairHashValRelease(struct TairHashVal *o) { +void tairHashValRelease(struct TairHashVal *o) { if (o) { if (o->value) { RedisModule_FreeString(NULL, o->value); @@ -85,6 +85,14 @@ int delEmptyTairHashIfNeeded(RedisModuleCtx *ctx, RedisModuleKey *key, RedisModu return 0; } + if (key == NULL) { + key = RedisModule_OpenKey(ctx, raw_key, REDISMODULE_WRITE); + if (RedisModule_KeyType(key) == REDISMODULE_KEYTYPE_EMPTY) { + RedisModule_CloseKey(key); + return 0; + } + } + if (redis_major_ver < 6 || (redis_major_ver == 6 && redis_minor_ver < 2)) { /* See bugfix: https://github.com/redis/redis/pull/8617 https://github.com/redis/redis/pull/8097