From 3f8756a06ae48258779ffc7cbef605a3ec0a926f Mon Sep 17 00:00:00 2001 From: Binbin Date: Tue, 22 Nov 2022 17:20:24 +0800 Subject: [PATCH] Fix set with duplicate elements causes sdiff to hang (#11530) This payload produces a set with duplicate elements (listpack encoding): ``` restore _key 0 "\x14\x25\x25\x00\x00\x00\x0A\x00\x06\x01\x82\x5F\x35\x03\x04\x01\x82\x5F\x31\x03\x82\x5F\x33\x03\x00\x01\x82\x5F\x39\x03\x82\x5F\x33\x03\x08\x01\x02\x01\xFF\x0B\x00\x31\xBE\x7D\x41\x01\x03\x5B\xEC" smembers key 1) "6" 2) "_5" 3) "4" 4) "_1" 5) "_3" ---> dup 6) "0" 7) "_9" 8) "_3" ---> dup 9) "8" 10) "2" ``` This kind of sets will cause SDIFF to hang, SDIFF generated a broken protocol and left the client hung. (Expected ten elements, but only got nine elements due to the duplication.) If we set `sanitize-dump-payload` to yes, we will be able to find the duplicate elements and report "ERR Bad data format". Discovered and discussed in #11290. This PR also improve prints when corrupt-dump-fuzzer hangs, it will print the cmds and the payload, an example like: ``` Testing integration/corrupt-dump-fuzzer [TIMEOUT]: clients state report follows. sock6 => (SPAWNED SERVER) pid:28884 Killing still running Redis server 28884 commands caused test to hang: SDIFF __key payload that caused test to hang: "\x14\balabala" ``` Co-authored-by: Oran Agra --- src/t_set.c | 3 +-- tests/integration/corrupt-dump-fuzzer.tcl | 13 ++++++++++++- tests/integration/corrupt-dump.tcl | 19 +++++++++++++++++++ tests/support/util.tcl | 13 ++++++++++--- 4 files changed, 42 insertions(+), 6 deletions(-) diff --git a/src/t_set.c b/src/t_set.c index 8ab602791..b56b38238 100644 --- a/src/t_set.c +++ b/src/t_set.c @@ -1493,8 +1493,7 @@ void sunionDiffGenericCommand(client *c, robj **setkeys, int setnum, } if (j == setnum) { /* There is no other set with this element. Add it. */ - setTypeAddAux(dstset, str, len, llval, encoding == OBJ_ENCODING_HT); - cardinality++; + cardinality += setTypeAddAux(dstset, str, len, llval, encoding == OBJ_ENCODING_HT); } } setTypeReleaseIterator(si); diff --git a/tests/integration/corrupt-dump-fuzzer.tcl b/tests/integration/corrupt-dump-fuzzer.tcl index 7c3b90969..9cd4ff913 100644 --- a/tests/integration/corrupt-dump-fuzzer.tcl +++ b/tests/integration/corrupt-dump-fuzzer.tcl @@ -147,10 +147,21 @@ foreach sanitize_dump {no yes} { if {$dbsize != [r dbsize]} { puts "unexpected keys" puts "keys: [r keys *]" - puts $sent + puts "commands leading to it:" + foreach cmd $sent { + foreach arg $cmd { + puts -nonewline "[string2printable $arg] " + } + puts "" + } exit 1 } } err ] } { + set err [format "%s" $err] ;# convert to string for pattern matching + if {[string match "*SIGTERM*" $err]} { + puts "payload that caused test to hang: $printable_dump" + exit 1 + } # if the server terminated update stats and restart it set report_and_restart true incr stat_terminated_in_traffic diff --git a/tests/integration/corrupt-dump.tcl b/tests/integration/corrupt-dump.tcl index ef878457a..35dca23be 100644 --- a/tests/integration/corrupt-dump.tcl +++ b/tests/integration/corrupt-dump.tcl @@ -810,5 +810,24 @@ test {corrupt payload: fuzzer findings - empty set listpack} { } } +test {corrupt payload: fuzzer findings - set with duplicate elements causes sdiff to hang} { + start_server [list overrides [list loglevel verbose use-exit-on-panic yes crash-memcheck-enabled no] ] { + r config set sanitize-dump-payload yes + r debug set-skip-checksum-validation 1 + catch {r restore _key 0 "\x14\x25\x25\x00\x00\x00\x0A\x00\x06\x01\x82\x5F\x35\x03\x04\x01\x82\x5F\x31\x03\x82\x5F\x33\x03\x00\x01\x82\x5F\x39\x03\x82\x5F\x33\x03\x08\x01\x02\x01\xFF\x0B\x00\x31\xBE\x7D\x41\x01\x03\x5B\xEC" replace} err + assert_match "*Bad data format*" $err + r ping + + # In the past, it generated a broken protocol and left the client hung in sdiff + r config set sanitize-dump-payload no + assert_equal {OK} [r restore _key 0 "\x14\x25\x25\x00\x00\x00\x0A\x00\x06\x01\x82\x5F\x35\x03\x04\x01\x82\x5F\x31\x03\x82\x5F\x33\x03\x00\x01\x82\x5F\x39\x03\x82\x5F\x33\x03\x08\x01\x02\x01\xFF\x0B\x00\x31\xBE\x7D\x41\x01\x03\x5B\xEC" replace] + assert_type set _key + assert_encoding listpack _key + assert_equal 10 [r scard _key] + assert_equal {0 2 4 6 8 _1 _3 _3 _5 _9} [lsort [r smembers _key]] + assert_equal {0 2 4 6 8 _1 _3 _5 _9} [lsort [r sdiff _key]] + } +} + } ;# tags diff --git a/tests/support/util.tcl b/tests/support/util.tcl index 22774868c..b2c9bdf7a 100644 --- a/tests/support/util.tcl +++ b/tests/support/util.tcl @@ -730,13 +730,20 @@ proc generate_fuzzy_traffic_on_key {key duration} { } else { set err [format "%s" $err] ;# convert to string for pattern matching if {[string match "*SIGTERM*" $err]} { - puts "command caused test to hang? $cmd" - exit 1 + puts "commands caused test to hang:" + foreach cmd $sent { + foreach arg $cmd { + puts -nonewline "[string2printable $arg] " + } + puts "" + } + # Re-raise, let handler up the stack take care of this. + error $err $::errorInfo } } } - # print stats so that we know if we managed to generate commands that actually made senes + # print stats so that we know if we managed to generate commands that actually made sense #if {$::verbose} { # set count [llength $sent] # puts "Fuzzy traffic sent: $count, succeeded: $succeeded"