From 67eca1bddee925e4aa552de7dc645348fb9f4458 Mon Sep 17 00:00:00 2001 From: Xudong Zhang Date: Thu, 2 Apr 2020 23:43:19 +0800 Subject: [PATCH 1/8] fix integer overflow --- src/quicklist.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/quicklist.c b/src/quicklist.c index ae183ffd8..52e3988f5 100644 --- a/src/quicklist.c +++ b/src/quicklist.c @@ -110,7 +110,7 @@ quicklist *quicklistCreate(void) { return quicklist; } -#define COMPRESS_MAX (1 << QL_COMP_BITS) +#define COMPRESS_MAX ((1 << QL_COMP_BITS)-1) void quicklistSetCompressDepth(quicklist *quicklist, int compress) { if (compress > COMPRESS_MAX) { compress = COMPRESS_MAX; @@ -120,7 +120,7 @@ void quicklistSetCompressDepth(quicklist *quicklist, int compress) { quicklist->compress = compress; } -#define FILL_MAX (1 << (QL_FILL_BITS-1)) +#define FILL_MAX ((1 << (QL_FILL_BITS-1))-1) void quicklistSetFill(quicklist *quicklist, int fill) { if (fill > FILL_MAX) { fill = FILL_MAX; From 115fd1136e27819e9f0171699c80eaa7e75d98d6 Mon Sep 17 00:00:00 2001 From: Guy Benoish Date: Tue, 25 Feb 2020 16:51:35 +0530 Subject: [PATCH 2/8] Stale replica should allow MULTI/EXEC Example: Client uses a pipe to send the following to a stale replica: MULTI .. do something ... DISCARD The replica will reply the MUTLI with -MASTERDOWN and execute the rest of the commands... A client using a pipe might not be aware that MULTI failed until it's too late. I can't think of a reason why MULTI/EXEC/DISCARD should not be executed on stale replicas... Also, enable MULTI/EXEC/DISCARD during loading --- src/server.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/server.c b/src/server.c index c89e9c075..ff7a39df5 100644 --- a/src/server.c +++ b/src/server.c @@ -672,15 +672,15 @@ struct redisCommand redisCommandTable[] = { 0,NULL,1,1,1,0,0,0}, {"multi",multiCommand,1, - "no-script fast @transaction", + "no-script fast ok-loading ok-stale @transaction", 0,NULL,0,0,0,0,0,0}, {"exec",execCommand,1, - "no-script no-monitor no-slowlog @transaction", + "no-script no-monitor no-slowlog ok-loading ok-stale @transaction", 0,NULL,0,0,0,0,0,0}, {"discard",discardCommand,1, - "no-script fast @transaction", + "no-script fast ok-loading ok-stale @transaction", 0,NULL,0,0,0,0,0,0}, {"sync",syncCommand,1, From dc38619afb6bcf60b011e4225d3931d6226e8303 Mon Sep 17 00:00:00 2001 From: Valentino Geron Date: Thu, 26 Mar 2020 11:49:21 +0200 Subject: [PATCH 3/8] XREAD and XREADGROUP should not be allowed from scripts when BLOCK option is being used --- src/server.c | 4 ++-- src/t_stream.c | 5 +++++ tests/unit/scripting.tcl | 11 +++++++++++ 3 files changed, 18 insertions(+), 2 deletions(-) diff --git a/src/server.c b/src/server.c index ff7a39df5..9ebf0ee6b 100644 --- a/src/server.c +++ b/src/server.c @@ -947,11 +947,11 @@ struct redisCommand redisCommandTable[] = { 0,NULL,1,1,1,0,0,0}, {"xread",xreadCommand,-4, - "read-only no-script @stream @blocking", + "read-only @stream @blocking", 0,xreadGetKeys,1,1,1,0,0,0}, {"xreadgroup",xreadCommand,-7, - "write no-script @stream @blocking", + "write @stream @blocking", 0,xreadGetKeys,1,1,1,0,0,0}, {"xgroup",xgroupCommand,-2, diff --git a/src/t_stream.c b/src/t_stream.c index 3f8cbfcfa..e0af87f97 100644 --- a/src/t_stream.c +++ b/src/t_stream.c @@ -1374,6 +1374,11 @@ void xreadCommand(client *c) { int moreargs = c->argc-i-1; char *o = c->argv[i]->ptr; if (!strcasecmp(o,"BLOCK") && moreargs) { + if (c->flags & CLIENT_LUA) { + /* There is no sense to use BLOCK option within LUA */ + addReplyErrorFormat(c, "%s command is not allowed with BLOCK option from scripts", (char *)c->argv[0]->ptr); + return; + } i++; if (getTimeoutFromObjectOrReply(c,c->argv[i],&timeout, UNIT_MILLISECONDS) != C_OK) return; diff --git a/tests/unit/scripting.tcl b/tests/unit/scripting.tcl index fb36d0b80..8b364b287 100644 --- a/tests/unit/scripting.tcl +++ b/tests/unit/scripting.tcl @@ -146,6 +146,17 @@ start_server {tags {"scripting"}} { set e } {*not allowed*} + test {EVAL - Scripts can't run XREAD and XREADGROUP with BLOCK option} { + r del s + r xgroup create s g $ MKSTREAM + set res [r eval {return redis.pcall('xread','STREAMS','s','$')} 1 s] + assert {$res eq {}} + assert_error "*xread command is not allowed with BLOCK option from scripts" {r eval {return redis.pcall('xread','BLOCK',0,'STREAMS','s','$')} 1 s} + set res [r eval {return redis.pcall('xreadgroup','group','g','c','STREAMS','s','>')} 1 s] + assert {$res eq {}} + assert_error "*xreadgroup command is not allowed with BLOCK option from scripts" {r eval {return redis.pcall('xreadgroup','group','g','c','BLOCK',0,'STREAMS','s','>')} 1 s} + } + test {EVAL - Scripts can't run certain commands} { set e {} r debug lua-always-replicate-commands 0 From 9e4f7b426f321d2b3f112ce7d0b7759cda3a3388 Mon Sep 17 00:00:00 2001 From: Guy Benoish Date: Thu, 6 Feb 2020 15:13:52 +0530 Subject: [PATCH 4/8] Modules: Perform printf-like format checks in variadic API --- src/redismodule.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/redismodule.h b/src/redismodule.h index d26c41456..754ee4fc6 100644 --- a/src/redismodule.h +++ b/src/redismodule.h @@ -470,7 +470,7 @@ RedisModuleString *REDISMODULE_API_FUNC(RedisModule_CreateStringFromLongLong)(Re RedisModuleString *REDISMODULE_API_FUNC(RedisModule_CreateStringFromDouble)(RedisModuleCtx *ctx, double d); RedisModuleString *REDISMODULE_API_FUNC(RedisModule_CreateStringFromLongDouble)(RedisModuleCtx *ctx, long double ld, int humanfriendly); RedisModuleString *REDISMODULE_API_FUNC(RedisModule_CreateStringFromString)(RedisModuleCtx *ctx, const RedisModuleString *str); -RedisModuleString *REDISMODULE_API_FUNC(RedisModule_CreateStringPrintf)(RedisModuleCtx *ctx, const char *fmt, ...); +RedisModuleString *REDISMODULE_API_FUNC(RedisModule_CreateStringPrintf)(RedisModuleCtx *ctx, const char *fmt, ...); __attribute__ ((format (printf, 2, 3))); void REDISMODULE_API_FUNC(RedisModule_FreeString)(RedisModuleCtx *ctx, RedisModuleString *str); const char *REDISMODULE_API_FUNC(RedisModule_StringPtrLen)(const RedisModuleString *str, size_t *len); int REDISMODULE_API_FUNC(RedisModule_ReplyWithError)(RedisModuleCtx *ctx, const char *err); @@ -554,8 +554,8 @@ void REDISMODULE_API_FUNC(RedisModule_SaveLongDouble)(RedisModuleIO *io, long do long double REDISMODULE_API_FUNC(RedisModule_LoadLongDouble)(RedisModuleIO *io); void *REDISMODULE_API_FUNC(RedisModule_LoadDataTypeFromString)(const RedisModuleString *str, const RedisModuleType *mt); RedisModuleString *REDISMODULE_API_FUNC(RedisModule_SaveDataTypeToString)(RedisModuleCtx *ctx, void *data, const RedisModuleType *mt); -void REDISMODULE_API_FUNC(RedisModule_Log)(RedisModuleCtx *ctx, const char *level, const char *fmt, ...); -void REDISMODULE_API_FUNC(RedisModule_LogIOError)(RedisModuleIO *io, const char *levelstr, const char *fmt, ...); +void REDISMODULE_API_FUNC(RedisModule_Log)(RedisModuleCtx *ctx, const char *level, const char *fmt, ...); __attribute__ ((format (printf, 3, 4))); +void REDISMODULE_API_FUNC(RedisModule_LogIOError)(RedisModuleIO *io, const char *levelstr, const char *fmt, ...); __attribute__ ((format (printf, 3, 4))); void REDISMODULE_API_FUNC(RedisModule__Assert)(const char *estr, const char *file, int line); void REDISMODULE_API_FUNC(RedisModule_LatencyAddSample)(const char *event, mstime_t latency); int REDISMODULE_API_FUNC(RedisModule_StringAppendBuffer)(RedisModuleCtx *ctx, RedisModuleString *str, const char *buf, size_t len); From 25875dddac69175ad0f1b246d37ca46ea7c73974 Mon Sep 17 00:00:00 2001 From: Guy Benoish Date: Fri, 3 Apr 2020 14:49:40 +0300 Subject: [PATCH 5/8] Use __attribute__ only if __GNUC__ is defined --- src/redismodule.h | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/src/redismodule.h b/src/redismodule.h index 754ee4fc6..23b4d26e0 100644 --- a/src/redismodule.h +++ b/src/redismodule.h @@ -470,7 +470,11 @@ RedisModuleString *REDISMODULE_API_FUNC(RedisModule_CreateStringFromLongLong)(Re RedisModuleString *REDISMODULE_API_FUNC(RedisModule_CreateStringFromDouble)(RedisModuleCtx *ctx, double d); RedisModuleString *REDISMODULE_API_FUNC(RedisModule_CreateStringFromLongDouble)(RedisModuleCtx *ctx, long double ld, int humanfriendly); RedisModuleString *REDISMODULE_API_FUNC(RedisModule_CreateStringFromString)(RedisModuleCtx *ctx, const RedisModuleString *str); -RedisModuleString *REDISMODULE_API_FUNC(RedisModule_CreateStringPrintf)(RedisModuleCtx *ctx, const char *fmt, ...); __attribute__ ((format (printf, 2, 3))); +#ifdef __GNUC__ +RedisModuleString *REDISMODULE_API_FUNC(RedisModule_CreateStringPrintf)(RedisModuleCtx *ctx, const char *fmt, ...) __attribute__ ((format (printf, 2, 3))); +#else +RedisModuleString *REDISMODULE_API_FUNC(RedisModule_CreateStringPrintf)(RedisModuleCtx *ctx, const char *fmt, ...); +#endif void REDISMODULE_API_FUNC(RedisModule_FreeString)(RedisModuleCtx *ctx, RedisModuleString *str); const char *REDISMODULE_API_FUNC(RedisModule_StringPtrLen)(const RedisModuleString *str, size_t *len); int REDISMODULE_API_FUNC(RedisModule_ReplyWithError)(RedisModuleCtx *ctx, const char *err); @@ -554,8 +558,13 @@ void REDISMODULE_API_FUNC(RedisModule_SaveLongDouble)(RedisModuleIO *io, long do long double REDISMODULE_API_FUNC(RedisModule_LoadLongDouble)(RedisModuleIO *io); void *REDISMODULE_API_FUNC(RedisModule_LoadDataTypeFromString)(const RedisModuleString *str, const RedisModuleType *mt); RedisModuleString *REDISMODULE_API_FUNC(RedisModule_SaveDataTypeToString)(RedisModuleCtx *ctx, void *data, const RedisModuleType *mt); -void REDISMODULE_API_FUNC(RedisModule_Log)(RedisModuleCtx *ctx, const char *level, const char *fmt, ...); __attribute__ ((format (printf, 3, 4))); -void REDISMODULE_API_FUNC(RedisModule_LogIOError)(RedisModuleIO *io, const char *levelstr, const char *fmt, ...); __attribute__ ((format (printf, 3, 4))); +#ifdef __GNUC__ +void REDISMODULE_API_FUNC(RedisModule_Log)(RedisModuleCtx *ctx, const char *level, const char *fmt, ...) __attribute__ ((format (printf, 3, 4))); +void REDISMODULE_API_FUNC(RedisModule_LogIOError)(RedisModuleIO *io, const char *levelstr, const char *fmt, ...) __attribute__ ((format (printf, 3, 4))); +#else +void REDISMODULE_API_FUNC(RedisModule_Log)(RedisModuleCtx *ctx, const char *level, const char *fmt, ...); +void REDISMODULE_API_FUNC(RedisModule_LogIOError)(RedisModuleIO *io, const char *levelstr, const char *fmt, ...); +#endif void REDISMODULE_API_FUNC(RedisModule__Assert)(const char *estr, const char *file, int line); void REDISMODULE_API_FUNC(RedisModule_LatencyAddSample)(const char *event, mstime_t latency); int REDISMODULE_API_FUNC(RedisModule_StringAppendBuffer)(RedisModuleCtx *ctx, RedisModuleString *str, const char *buf, size_t len); From 728c62056256e8f467264d6974d6361ed17ab8cc Mon Sep 17 00:00:00 2001 From: Guy Benoish Date: Thu, 2 Apr 2020 14:57:17 +0300 Subject: [PATCH 6/8] Try to fix time-sensitive tests in blockonkey.tcl There is an inherent race between the deferring client and the "main" client of the test: While the deferring client issues a blocking command, we can't know for sure that by the time the "main" client tries to issue another command (Usually one that unblocks the deferring client) the deferring client is even blocked... For lack of a better choice this commit uses TCL's 'after' in order to give some time for the deferring client to issues its blocking command before the "main" client does its thing. This problem probably exists in many other tests but this commit tries to fix blockonkeys.tcl --- tests/unit/moduleapi/blockonkeys.tcl | 55 +++++++++++++++++++++++++++- 1 file changed, 54 insertions(+), 1 deletion(-) diff --git a/tests/unit/moduleapi/blockonkeys.tcl b/tests/unit/moduleapi/blockonkeys.tcl index c8b8f23ed..5e5d93da3 100644 --- a/tests/unit/moduleapi/blockonkeys.tcl +++ b/tests/unit/moduleapi/blockonkeys.tcl @@ -11,6 +11,12 @@ start_server {tags {"modules"}} { $rd1 fsl.bpoppush src dst 0 $rd2 fsl.bpoppush dst src 0 + ;# wait until clients are actually blocked + wait_for_condition 50 100 { + [s 0 blocked_clients] eq {2} + } else { + fail "Clients are not blocked" + } r fsl.push src 42 @@ -24,7 +30,12 @@ start_server {tags {"modules"}} { r del src $rd1 fsl.bpoppush src src 0 - + ;# wait until clients are actually blocked + wait_for_condition 50 100 { + [s 0 blocked_clients] eq {1} + } else { + fail "Clients are not blocked" + } r fsl.push src 42 assert_equal {42} [r fsl.getall src] @@ -48,6 +59,12 @@ start_server {tags {"modules"}} { r del k set rd [redis_deferring_client] $rd fsl.bpop k 0 + ;# wait until clients are actually blocked + wait_for_condition 50 100 { + [s 0 blocked_clients] eq {1} + } else { + fail "Clients are not blocked" + } r fsl.push k 34 assert_equal {34} [$rd read] } @@ -76,6 +93,12 @@ start_server {tags {"modules"}} { set cid [$rd read] r fsl.push k 33 $rd fsl.bpopgt k 33 0 + ;# wait until clients are actually blocked + wait_for_condition 50 100 { + [s 0 blocked_clients] eq {1} + } else { + fail "Clients are not blocked" + } r fsl.push k 34 assert_equal {34} [$rd read] r client kill id $cid ;# try to smoke-out client-related memory leak @@ -85,6 +108,12 @@ start_server {tags {"modules"}} { r del k set rd [redis_deferring_client] $rd fsl.bpopgt k 35 0 + ;# wait until clients are actually blocked + wait_for_condition 50 100 { + [s 0 blocked_clients] eq {1} + } else { + fail "Clients are not blocked" + } r fsl.push k 33 r fsl.push k 34 r fsl.push k 35 @@ -98,6 +127,12 @@ start_server {tags {"modules"}} { $rd client id set cid [$rd read] $rd fsl.bpopgt k 35 0 + ;# wait until clients are actually blocked + wait_for_condition 50 100 { + [s 0 blocked_clients] eq {1} + } else { + fail "Clients are not blocked" + } r client kill id $cid ;# try to smoke-out client-related memory leak } @@ -107,6 +142,12 @@ start_server {tags {"modules"}} { $rd client id set cid [$rd read] $rd fsl.bpopgt k 35 0 + ;# wait until clients are actually blocked + wait_for_condition 50 100 { + [s 0 blocked_clients] eq {1} + } else { + fail "Clients are not blocked" + } r client unblock $cid timeout ;# try to smoke-out client-related memory leak assert_equal {Request timedout} [$rd read] } @@ -117,6 +158,12 @@ start_server {tags {"modules"}} { $rd client id set cid [$rd read] $rd fsl.bpopgt k 35 0 + ;# wait until clients are actually blocked + wait_for_condition 50 100 { + [s 0 blocked_clients] eq {1} + } else { + fail "Clients are not blocked" + } r client unblock $cid error ;# try to smoke-out client-related memory leak assert_error "*unblocked*" {$rd read} } @@ -125,6 +172,12 @@ start_server {tags {"modules"}} { r del k set rd [redis_deferring_client] $rd fsl.bpop k 0 + ;# wait until clients are actually blocked + wait_for_condition 50 100 { + [s 0 blocked_clients] eq {1} + } else { + fail "Clients are not blocked" + } r lpush k 12 r lpush k 13 r lpush k 14 From dcd67263664e97ddfdd6986db7f78f4f799f8f4c Mon Sep 17 00:00:00 2001 From: Oran Agra Date: Mon, 6 Apr 2020 09:41:14 +0300 Subject: [PATCH 7/8] diffrent fix for runtest --host --port --- tests/support/server.tcl | 7 ++----- tests/test_helper.tcl | 19 +++++++++++-------- 2 files changed, 13 insertions(+), 13 deletions(-) diff --git a/tests/support/server.tcl b/tests/support/server.tcl index 400017c5f..d086366dc 100644 --- a/tests/support/server.tcl +++ b/tests/support/server.tcl @@ -159,12 +159,9 @@ proc start_server {options {code undefined}} { if {$::external} { if {[llength $::servers] == 0} { set srv {} - # In test_server_main(tests/test_helper.tcl:215~218), increase the value of start_port - # and assign it to ::port through the `--port` option, so we need to reduce it. - set baseport [expr {$::port-100}] dict set srv "host" $::host - dict set srv "port" $baseport - set client [redis $::host $baseport 0 $::tls] + dict set srv "port" $::port + set client [redis $::host $::port 0 $::tls] dict set srv "client" $client $client select 9 diff --git a/tests/test_helper.tcl b/tests/test_helper.tcl index 5cb43104b..d80cb6907 100644 --- a/tests/test_helper.tcl +++ b/tests/test_helper.tcl @@ -212,13 +212,19 @@ proc test_server_main {} { # Start the client instances set ::clients_pids {} - set start_port [expr {$::port+100}] - for {set j 0} {$j < $::numclients} {incr j} { - set start_port [find_available_port $start_port] + if {$::external} { set p [exec $tclsh [info script] {*}$::argv \ - --client $port --port $start_port &] + --client $port --port $::port &] lappend ::clients_pids $p - incr start_port 10 + } else { + set start_port [expr {$::port+100}] + for {set j 0} {$j < $::numclients} {incr j} { + set start_port [find_available_port $start_port] + set p [exec $tclsh [info script] {*}$::argv \ + --client $port --port $start_port &] + lappend ::clients_pids $p + incr start_port 10 + } } # Setup global state for the test server @@ -506,9 +512,6 @@ for {set j 0} {$j < [llength $argv]} {incr j} { } elseif {$opt eq {--host}} { set ::external 1 set ::host $arg - # If we use an external server, we can only set numclients to 1, - # otherwise the port will be miscalculated. - set ::numclients 1 incr j } elseif {$opt eq {--port}} { set ::port $arg From c5d805f87771581d3f6b29861ed2062c0ae2a688 Mon Sep 17 00:00:00 2001 From: srzhao Date: Mon, 20 Jan 2020 21:17:02 +0800 Subject: [PATCH 8/8] Check OOM at script start to get stable lua OOM state. Checking OOM by `getMaxMemoryState` inside script might get different result with `freeMemoryIfNeededAndSafe` at script start, because lua stack and arguments also consume memory. This leads to memory `borderline` when memory grows near server.maxmemory: - `freeMemoryIfNeededAndSafe` at script start detects no OOM, no memory freed - `getMaxMemoryState` inside script detects OOM, script aborted We solve this 'borderline' issue by saving OOM state at script start to get stable lua OOM state. related to issue #6565 and #5250. --- src/scripting.c | 7 +++---- src/server.c | 7 +++++++ src/server.h | 1 + 3 files changed, 11 insertions(+), 4 deletions(-) diff --git a/src/scripting.c b/src/scripting.c index 7f64e06db..32a511e13 100644 --- a/src/scripting.c +++ b/src/scripting.c @@ -657,12 +657,11 @@ int luaRedisGenericCommand(lua_State *lua, int raise_error) { !server.loading && /* Don't care about mem if loading. */ !server.masterhost && /* Slave must execute the script. */ server.lua_write_dirty == 0 && /* Script had no side effects so far. */ + server.lua_oom && /* Detected OOM when script start. */ (cmd->flags & CMD_DENYOOM)) { - if (getMaxmemoryState(NULL,NULL,NULL,NULL) != C_OK) { - luaPushError(lua, shared.oomerr->ptr); - goto cleanup; - } + luaPushError(lua, shared.oomerr->ptr); + goto cleanup; } if (cmd->flags & CMD_RANDOM) server.lua_random_dirty = 1; diff --git a/src/server.c b/src/server.c index 9ebf0ee6b..d6abc72d9 100644 --- a/src/server.c +++ b/src/server.c @@ -3441,6 +3441,13 @@ int processCommand(client *c) { addReply(c, shared.oomerr); return C_OK; } + + /* Save out_of_memory result at script start, otherwise if we check OOM + * untill first write within script, memory used by lua stack and + * arguments might interfere. */ + if (c->cmd->proc == evalCommand || c->cmd->proc == evalShaCommand) { + server.lua_oom = out_of_memory; + } } /* Make sure to use a reasonable amount of memory for client side diff --git a/src/server.h b/src/server.h index dbd72281f..f37fe4b12 100644 --- a/src/server.h +++ b/src/server.h @@ -1392,6 +1392,7 @@ struct redisServer { execution. */ int lua_kill; /* Kill the script if true. */ int lua_always_replicate_commands; /* Default replication type. */ + int lua_oom; /* OOM detected when script start? */ /* Lazy free */ int lazyfree_lazy_eviction; int lazyfree_lazy_expire;