From 126de75d90d98bac25affb575e63ebe511c55d12 Mon Sep 17 00:00:00 2001 From: Yossi Gottlieb Date: Thu, 23 Jun 2016 22:30:32 +0300 Subject: [PATCH 1/5] Fix RedisModule_Calloc() definition typo. --- src/redismodule.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/redismodule.h b/src/redismodule.h index f1aaea49b..fd9e46dc6 100644 --- a/src/redismodule.h +++ b/src/redismodule.h @@ -100,7 +100,7 @@ typedef void (*RedisModuleTypeFreeFunc)(void *value); void *REDISMODULE_API_FUNC(RedisModule_Alloc)(size_t bytes); void *REDISMODULE_API_FUNC(RedisModule_Realloc)(void *ptr, size_t bytes); void REDISMODULE_API_FUNC(RedisModule_Free)(void *ptr); -void REDISMODULE_API_FUNC(RedisModule_Calloc)(size_t nmemb, size_t size); +void *REDISMODULE_API_FUNC(RedisModule_Calloc)(size_t nmemb, size_t size); char *REDISMODULE_API_FUNC(RedisModule_Strdup)(const char *str); int REDISMODULE_API_FUNC(RedisModule_GetApi)(const char *, void *); int REDISMODULE_API_FUNC(RedisModule_CreateCommand)(RedisModuleCtx *ctx, const char *name, RedisModuleCmdFunc cmdfunc, const char *strflags, int firstkey, int lastkey, int keystep); From b7298917f922cc06fbe53c720f29d196ffc4a8ae Mon Sep 17 00:00:00 2001 From: antirez Date: Mon, 27 Jun 2016 18:02:33 +0200 Subject: [PATCH 2/5] Fix quicklistReplaceAtIndex() by updating the quicklist ziplist size. The quicklist takes a cached version of the ziplist representation size in bytes. The implementation must update this length every time the underlying ziplist changes. However quicklistReplaceAtIndex() failed to fix the length. During LSET calls, the size of the ziplist blob and the cached size inside the quicklist diverged. Later, when this size is used in an authoritative way, for example during nodes splitting in order to copy the nodes, we end with a duplicated node that may contain random garbage. This commit should fix issue #3343, however several problems were found reviewing the quicklist.c code in search of this bug that should be addressed soon or later. For example: 1. To take a cached ziplist length is fragile since failing to update it leads to this kind of issues. 2. The node splitting code needs auditing. For example it works just for a side effect of ziplistDeleteRange() to be able to cope with a wrong count of elements to remove. The code inside quicklist.c assumes that -1 means "delete till the end" while actually it's just a count of how many elements to delete, and is an unsigned count. So -1 gets converted into the maximum integer, and just by chance the ziplist code stops deleting elements after there are no more to delete. 3. Node splitting is extremely inefficient, it copies the node and removes elements from both nodes even when actually there is to move a single entry from one node to the other, or when the new resulting node is empty at all so there is nothing to copy but just to create a new node. However at least for Redis 3.2 to introduce fresh code inside quicklist.c may be even more risky, so instead I'm writing a better fuzzy tester to stress the internals a bit more in order to anticipate other possible bugs. This bug was found using a fuzzy tester written after having some clue about where the bug could be. The tester eventually created a ~2000 commands sequence able to always crash Redis. I wrote a better version of the tester that searched for the smallest sequence that could crash Redis automatically. Later this smaller sequence was minimized by removing random commands till it still crashed the server. This resulted into a sequence of 7 commands. With this small sequence it was just a matter of filling the code with enough printf() to understand enough state to fix the bug. --- src/quicklist.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/quicklist.c b/src/quicklist.c index adf9ba1de..9cb052525 100644 --- a/src/quicklist.c +++ b/src/quicklist.c @@ -671,6 +671,7 @@ int quicklistReplaceAtIndex(quicklist *quicklist, long index, void *data, /* quicklistIndex provides an uncompressed node */ entry.node->zl = ziplistDelete(entry.node->zl, &entry.zi); entry.node->zl = ziplistInsert(entry.node->zl, entry.zi, data, sz); + quicklistNodeUpdateSz(entry.node); quicklistCompress(quicklist, entry.node); return 1; } else { From 224533b4540a87b94577f91a81156611fb2ae417 Mon Sep 17 00:00:00 2001 From: antirez Date: Tue, 28 Jun 2016 09:26:28 +0200 Subject: [PATCH 3/5] Regression test for issue #3343 exact min crash sequence. Note: it was verified that it can crash the test suite without the patch applied. --- tests/unit/type/list-3.tcl | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/tests/unit/type/list-3.tcl b/tests/unit/type/list-3.tcl index ece6ea2d5..744f70371 100644 --- a/tests/unit/type/list-3.tcl +++ b/tests/unit/type/list-3.tcl @@ -13,6 +13,22 @@ start_server { assert_equal [r lindex l 1] [lindex $mylist 1] } + test {Regression for quicklist #3343 bug} { + r del mylist + r lpush mylist 401 + r lpush mylist 392 + r rpush mylist [string repeat x 5105]"799" + r lset mylist -1 [string repeat x 1014]"702" + r lpop mylist + r lset mylist -1 [string repeat x 4149]"852" + r linsert mylist before 401 [string repeat x 9927]"12" + r lrange mylist 0 -1 + r ping ; # It's enough if the server is still alive + } {PONG} + + test {Stress tester for #3343-alike bugs} { + } + tags {slow} { test {ziplist implementation: value encoding and backlink} { if {$::accurate} {set iterations 100} else {set iterations 10} From bb0776971f7e413f12217dfed3855e74f7907b65 Mon Sep 17 00:00:00 2001 From: antirez Date: Tue, 28 Jun 2016 09:33:36 +0200 Subject: [PATCH 4/5] Stress tester WIP. --- tests/unit/type/list-3.tcl | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/unit/type/list-3.tcl b/tests/unit/type/list-3.tcl index 744f70371..7888409ec 100644 --- a/tests/unit/type/list-3.tcl +++ b/tests/unit/type/list-3.tcl @@ -27,6 +27,9 @@ start_server { } {PONG} test {Stress tester for #3343-alike bugs} { + for {set j 0} {$j < 100} {incr j} { + puts [randomInt 10] + } } tags {slow} { From a0c8276e1695a89f43da037327f7112464492d43 Mon Sep 17 00:00:00 2001 From: antirez Date: Tue, 28 Jun 2016 09:42:20 +0200 Subject: [PATCH 5/5] Test: new randomized stress tester for #3343 alike bugs. --- tests/unit/type/list-3.tcl | 29 +++++++++++++++++++++++++++-- 1 file changed, 27 insertions(+), 2 deletions(-) diff --git a/tests/unit/type/list-3.tcl b/tests/unit/type/list-3.tcl index 7888409ec..b5bd48cb0 100644 --- a/tests/unit/type/list-3.tcl +++ b/tests/unit/type/list-3.tcl @@ -27,8 +27,33 @@ start_server { } {PONG} test {Stress tester for #3343-alike bugs} { - for {set j 0} {$j < 100} {incr j} { - puts [randomInt 10] + r del key + for {set j 0} {$j < 10000} {incr j} { + set op [randomInt 6] + set small_signed_count [expr 5-[randomInt 10]] + if {[randomInt 2] == 0} { + set ele [randomInt 1000] + } else { + set ele [string repeat x [randomInt 10000]][randomInt 1000] + } + switch $op { + 0 {r lpush key $ele} + 1 {r rpush key $ele} + 2 {r lpop key} + 3 {r rpop key} + 4 { + catch {r lset key $small_signed_count $ele} + } + 5 { + set otherele [randomInt 1000] + if {[randomInt 2] == 0} { + set where before + } else { + set where after + } + r linsert key $where $otherele $ele + } + } } }