1. 01 Aug, 2021 3 commits
    • Binbin's avatar
      GEO* STORE with empty src key delete the dest key and return 0, not empty array (#9271) · 86555ae0
      Binbin authored
      
      
      With an empty src key, we need to deal with two situations:
      1. non-STORE: We should return emptyarray.
      2. STORE: Try to delete the store key and return 0.
      
      This applies to both GEOSEARCHSTORE (new to v6.2), and
      also GEORADIUS STORE (which was broken since forever)
      
      This pr try to fix #9261. i.e. both STORE variants would have behaved
      like the non-STORE variants when the source key was missing,
      returning an empty array and not deleting the destination key,
      instead of returning 0, and deleting the destination key.
      
      Also add more tests for some commands.
      - GEORADIUS: wrong type src key, non existing src key, empty search,
        store with non existing src key, store with empty search
      - GEORADIUSBYMEMBER: wrong type src key, non existing src key,
        non existing member, store with non existing src key
      - GEOSEARCH: wrong type src key, non existing src key, empty search,
        frommember with non existing member
      - GEOSEARCHSTORE: wrong type key, non existing src key,
        fromlonlat with empty search, frommember with non existing member
      Co-authored-by: default avatarOran Agra <oran@redislabs.com>
      86555ae0
    • Yossi Gottlieb's avatar
      Tests: avoid short reads on redis-cli output. (#9301) · 68b8b45c
      Yossi Gottlieb authored
      In some cases large replies on slow systems may only be partially read
      by the test suite, resulting with parsing errors.
      
      This fix is still timing sensitive but should greatly reduce the chances
      of this happening.
      68b8b45c
    • Guy Korland's avatar
      1483f5aa
  2. 30 Jul, 2021 1 commit
  3. 29 Jul, 2021 3 commits
    • Yossi Gottlieb's avatar
    • Wen Hui's avatar
      Remove duplicate zero-port sentinels (#9240) · db415364
      Wen Hui authored
      The issue is that when a sentinel with the same address and IP is turned on with a different runid, its port is set to 0 but it is still present in the dictionary master->sentinels which contain all the sentinels for a master.
      
      This causes a problem when we do INFO SENTINEL because it takes the size of the dictionary of sentinels. This might also cause a problem for failover if enough sentinels have their port set to 0 since the number of voters in failover is also determined by the size of the dictionary of sentinels.
      
      This commits removes the sentinels with the port set to zero from the dictionary of sentinels.
      Fixes #8786
      db415364
    • sundb's avatar
      Fix missing check for sanitize_dump in corrupt-dump-fuzzer test (#9285) · 3db0f1a2
      sundb authored
      this means the assertion that checks that when deep sanitization is enabled,
      there are no crashes, was missing.
      3db0f1a2
  4. 25 Jul, 2021 1 commit
  5. 21 Jul, 2021 1 commit
    • Huang Zhw's avatar
      On 32 bit platform, the bit position of GETBIT/SETBIT/BITFIELD/BITCOUNT,BITPOS... · 71d45287
      Huang Zhw authored
      On 32 bit platform, the bit position of GETBIT/SETBIT/BITFIELD/BITCOUNT,BITPOS may overflow (see CVE-2021-32761) (#9191)
      
      GETBIT, SETBIT may access wrong address because of wrap.
      BITCOUNT and BITPOS may return wrapped results.
      BITFIELD may access the wrong address but also allocate insufficient memory and segfault (see CVE-2021-32761).
      
      This commit uses `uint64_t` or `long long` instead of `size_t`.
      related https://github.com/redis/redis/pull/8096
      
      At 32bit platform:
      > setbit bit 4294967295 1
      (integer) 0
      > config set proto-max-bulk-len 536870913
      OK
      > append bit "\xFF"
      (integer) 536870913
      > getbit bit 4294967296
      (integer) 0
      
      When the bit index is larger than 4294967295, size_t can't hold bit index. In the past,  `proto-max-bulk-len` is limit to 536870912, so there is no problem.
      
      After this commit, bit position is stored in `uint64_t` or `long long`. So when `proto-max-bulk-len > 536870912`, 32bit platforms can still be correct.
      
      For 64bit platform, this problem still exists. The major ...
      71d45287
  6. 17 Jul, 2021 1 commit
  7. 14 Jul, 2021 1 commit
    • Oran Agra's avatar
      Test infra, handle RESP3 attributes and big-numbers and bools (#9235) · 6a5bac30
      Oran Agra authored
      - promote the code in DEBUG PROTOCOL to addReplyBigNum
      - DEBUG PROTOCOL ATTRIB skips the attribute when client is RESP2
      - networking.c addReply for push and attributes generate assertion when
        called on a RESP2 client, anything else would produce a broken
        protocol that clients can't handle.
      6a5bac30
  8. 11 Jul, 2021 2 commits
  9. 07 Jul, 2021 1 commit
    • Mikhail Fesenko's avatar
      Direct redis-cli repl prints to stderr, because --rdb can print to stdout.... · 1eb4baa5
      Mikhail Fesenko authored
      
      Direct redis-cli repl prints to stderr, because --rdb can print to stdout. fflush stdout after responses  (#9136)
      
      1. redis-cli can output --rdb data to stdout
         but redis-cli also write some messages to stdout which will mess up the rdb.
      
      2. Make redis-cli flush stdout when printing a reply
        This was needed in order to fix a hung in redis-cli test that uses
        --replica.
         Note that printf does flush when there's a newline, but fwrite does not.
      
      3. fix the redis-cli --replica test which used to pass previously
         because it didn't really care what it read, and because redis-cli
         used printf to print these other things to stdout.
      
      4. improve redis-cli --replica test to run with both diskless and disk-based.
      Co-authored-by: default avatarOran Agra <oran@redislabs.com>
      Co-authored-by: default avatarViktor Söderqvist <viktor@zuiderkwast.se>
      1eb4baa5
  10. 05 Jul, 2021 1 commit
  11. 04 Jul, 2021 2 commits
  12. 01 Jul, 2021 1 commit
    • Yossi Gottlieb's avatar
      Fix CLIENT UNBLOCK crashing modules. (#9167) · aa139e2f
      Yossi Gottlieb authored
      Modules that use background threads with thread safe contexts are likely
      to use RM_BlockClient() without a timeout function, because they do not
      set up a timeout.
      
      Before this commit, `CLIENT UNBLOCK` would result with a crash as the
      `NULL` timeout callback is called. Beyond just crashing, this is also
      logically wrong as it may throw the module into an unexpected client
      state.
      
      This commits makes `CLIENT UNBLOCK` on such clients behave the same as
      any other client that is not in a blocked state and therefore cannot be
      unblocked.
      aa139e2f
  13. 30 Jun, 2021 2 commits
    • Binbin's avatar
      Add missing pause tcl test to test_helper.tcl (#9158) · 5dddf496
      Binbin authored
      * Add keyname tags to avoid CROSSSLOT errors in external server CI
      * Use new wait_for_blocked_clients_count in pause.tcl
      5dddf496
    • Binbin's avatar
      Fix timing issue in psync2 test. (#9159) · 1d5aa37d
      Binbin authored
      
      
      *** [err]: PSYNC2: total sum of full synchronizations is exactly 4 intests/integration/psync2.tcl
      Expected 5 == 4 (context: type eval line 8 cmd {assert {$sum == 4}} proc::test)
      
      Sometime the test got an unexpected full sync since a replica switch to master,
      before the new master change propagated the new replid to all replicas,
      a replica attempted to sync with it using a wrong replid and triggered a full resync.
      
      Consider this scenario:
          1 slaveof 4 full resync
          0 slaveof 4 full resync
          2 slaveof 0 full resync
          3 slaveof 1 full resync
      
          1 slaveof no one, replid changed
          3 reconnect 1, did a partial resyn and got the new replid
      
          Before 2 inherits the new replid.
          3 slaveof 2
          3 try to do a partial resyn with 2.
          But their replication ids are inconsistent, so a full resync happens.
      
      :) A special thank you for oran and helping me in this test case.
      Co-authored-by: default avatarOran Agra <oran@redislabs.com>
      1d5aa37d
  14. 29 Jun, 2021 3 commits
  15. 24 Jun, 2021 2 commits
    • Yossi Gottlieb's avatar
      Add bind-source-addr configuration argument. (#9142) · f233c4c5
      Yossi Gottlieb authored
      In the past, the first bind address that was explicitly specified was
      also used to bind outgoing connections. This could result with some
      problems. For example: on some systems using `bind 127.0.0.1` would
      result with outgoing connections also binding to `127.0.0.1` and failing
      to connect to remote addresses.
      
      With the recent change to the way `bind` is handled, this presented
      other issues:
      
      * The default first bind address is '*' which is not a valid address.
      * We make no distinction between user-supplied config that is identical
      to the default, and the default config.
      
      This commit addresses both these issues by introducing an explicit
      configuration parameter to control the bind address on outgoing
      connections.
      f233c4c5
    • Oran Agra's avatar
      5ffdbae1
  16. 22 Jun, 2021 7 commits
    • Oran Agra's avatar
      Adjustments to recent RM_StringTruncate fix (#3718) (#9125) · ae418eca
      Oran Agra authored
      - Introduce a new sdssubstr api as a building block for sdsrange.
        The API of sdsrange is many times hard to work with and also has
        corner case that cause bugs. sdsrange is easy to work with and also
        simplifies the implementation of sdsrange.
      - Revert the fix to RM_StringTruncate and just use sdssubstr instead of
        sdsrange.
      - Solve valgrind warnings from the new tests introduced by the previous
        PR.
      ae418eca
    • Yossi Gottlieb's avatar
      a49b7668
    • Yossi Gottlieb's avatar
      Fix typo in test. (#9128) · 8284544a
      Yossi Gottlieb authored
      8284544a
    • Yossi Gottlieb's avatar
      Improve bind and protected-mode config handling. (#9034) · 07b0d144
      Yossi Gottlieb authored
      * Specifying an empty `bind ""` configuration prevents Redis from listening on any TCP port. Before this commit, such configuration was not accepted.
      * Using `CONFIG GET bind` will always return an explicit configuration value. Before this commit, if a bind address was not specified the returned value was empty (which was an anomaly).
      
      Another behavior change is that modifying the `bind` configuration to a non-default value will NO LONGER DISABLE protected-mode implicitly.
      07b0d144
    • Evan's avatar
      modules: Add newlen == 0 handling to RM_StringTruncate (#3717) (#3718) · 1ccf2ca2
      Evan authored
      Previously, passing 0 for newlen would not truncate the string at all.
      This adds handling of this case, freeing the old string and creating a new empty string.
      
      Other changes:
      - Move `src/modules/testmodule.c` to `tests/modules/basics.c`
      - Introduce that basic test into the test suite
      - Add tests to cover StringTruncate
      - Add `test-modules` build target for the main makefile
      - Extend `distclean` build target to clean modules too
      1ccf2ca2
    • Oran Agra's avatar
      solve test timing issues in replication tests (#9121) · d0819d61
      Oran Agra authored
      # replication-3.tcl
      had a test timeout failure with valgrind on daily CI:
      ```
      *** [err]: SLAVE can reload "lua" AUX RDB fields of duplicated scripts in tests/integration/replication-3.tcl
      Replication not started.
      ```
      replication took more than 70 seconds.
      https://github.com/redis/redis/runs/2854037905?check_suite_focus=true
      
      on my machine it takes only about 30, but i can see how 50 seconds isn't enough.
      
      # replication.tcl
      loading was over too quickly in freebsd daily CI:
      ```
      *** [err]: slave fails full sync and diskless load swapdb recovers it in tests/integration/replication.tcl
      Expected '0' to be equal to '1' (context: type eval line 44 cmd {assert_equal [s -1 loading] 1} proc ::start_server)
      ```
      
      # rdb.tcl
      loading was over too quickly.
      increase the time loading takes, and decrease the amount of work we try to achieve in that time.
      d0819d61
    • Oran Agra's avatar
      Fix race in client side tracking (#9116) · 9b564b52
      Oran Agra authored
      The `Tracking gets notification of expired keys` test in tracking.tcl
      used to hung in valgrind CI quite a lot.
      
      It turns out the reason is that with valgrind and a busy machine, the
      server cron active expire cycle could easily run in the same event loop
      as the command that created `mykey`, so that when they key got expired,
      there were two change events to broadcast, one that set the key and one
      that expired it, but since we used raxTryInsert, the client that was
      associated with the "last" change was the one that created the key, so
      the NOLOOP filtered that event.
      
      This commit adds a test that reproduces the problem by using lazy expire
      in a multi-exec which makes sure the key expires in the same event loop
      as the one that added it.
      9b564b52
  17. 17 Jun, 2021 1 commit
  18. 16 Jun, 2021 3 commits
  19. 15 Jun, 2021 2 commits
    • sundb's avatar
      Fix the wrong reisze of querybuf (#9003) · e5d8a5eb
      sundb authored
      The initialize memory of `querybuf` is `PROTO_IOBUF_LEN(1024*16) * 2` (due to sdsMakeRoomFor being greedy), under `jemalloc`, the allocated memory will be 40k.
      This will most likely result in the `querybuf` being resized when call `clientsCronResizeQueryBuffer` unless the client requests it fast enough.
      
      Note that this bug existed even before #7875, since the condition for resizing includes the sds headers (32k+6).
      
      ## Changes
      1. Use non-greedy sdsMakeRoomFor when allocating the initial query buffer (of 16k).
      1. Also use non-greedy allocation when working with BIG_ARG (we won't use that extra space anyway)
      2. in case we did use a greedy allocation, read as much as we can into the buffer we got (including internal frag), to reduce system calls.
      3. introduce a dedicated constant for the shrinking (same value as before)
      3. Add test for querybuf.
      4. improve a maxmemory test by ignoring the effect of replica query buffers (can accumulate many ACKs on slow env)
      5. improve a maxmemory by disabling slowlog (it will cause slight memory growth on slow env).
      e5d8a5eb
    • Binbin's avatar
      Fix XINFO help for unexpected options. (#9075) · b1099773
      Binbin authored
      Small cleanup and consistency.
      b1099773
  20. 14 Jun, 2021 2 commits
    • Binbin's avatar
      slowlog get command supports passing in -1 to get all logs. (#9018) · 7900b48b
      Binbin authored
      This was already the case before this commit, but it wasn't clear / intended in the code, now it does.
      7900b48b
    • YaacovHazan's avatar
      cleanup around loadAppendOnlyFile (#9012) · 1677efb9
      YaacovHazan authored
      Today when we load the AOF on startup, the loadAppendOnlyFile checks if
      the file is openning for reading.
      This check is redundent (dead code) as we open the AOF file for writing at initServer,
      and the file will always be existing for the loadAppendOnlyFile.
      
      In this commit:
      - remove all the exit(1) from loadAppendOnlyFile, as it is the caller
        responsibility to decide what to do in case of failure.
      - move the opening of the AOF file for writing, to be after we loading it.
      - avoid return -ERR in DEBUG LOADAOF, when the AOF is existing but empty
      1677efb9