1. 27 Apr, 2022 10 commits
    • Madelyn Olson's avatar
      Redact ACL SETUSER arguments if the user has spaces (#9935) · 066b6832
      Madelyn Olson authored
      
      (cherry picked from commit c40d23b8)
      066b6832
    • Meir Shpilraien (Spielrein)'s avatar
      Clean Lua stack before parsing call reply to avoid crash on a call with many arguments (#9809) · 93c1d31d
      Meir Shpilraien (Spielrein) authored
      This commit 0f8b634c (CVE-2021-32626 released in 6.2.6, 6.0.16, 5.0.14)
      fixes an invalid memory write issue by using `lua_checkstack` API to make
      sure the Lua stack is not overflow. This fix was added on 3 places:
      1. `luaReplyToRedisReply`
      2. `ldbRedis`
      3. `redisProtocolToLuaType`
      
      On the first 2 functions, `lua_checkstack` is handled gracefully while the
      last is handled with an assert and a statement that this situation can
      not happened (only with misbehave module):
      
      > the Redis reply might be deep enough to explode the LUA stack (notice
      that currently there is no such command in Redis that returns such a nested
      reply, but modules might do it)
      
      The issue that was discovered is that user arguments is also considered part
      of the stack, and so the following script (for example) make the assertion reachable:
      ```
      local a = {}
      for i=1,7999 do
          a[i] = 1
      end
      return redis.call("lpush", "l", unpack(a))
      ```
      
      This is a regression because such a script would have worked before and now
      its crashing Redis. The solution is to clear the function arguments from the Lua
      stack which makes the original assumption true and the assertion unreachable.
      
      (cherry picked from commit 6b0b04f1)
      93c1d31d
    • Binbin's avatar
      Add tests to cover EXPIRE overflow fix (#9839) · 8fca090e
      Binbin authored
      In #8287, some overflow checks have been added. But when
      `when *= 1000` overflows, it will become a positive number.
      And the check not able to catch it. The key will be added with
      a short expiration time and will deleted a few seconds later.
      
      In #9601, will check the overflow after `*=` and return an
      error first, and avoiding this situation.
      
      In this commit, added some tests to cover those code paths.
      Found it in #9825, and close it.
      
      (cherry picked from commit 9273d09d)
      8fca090e
    • Itamar Haber's avatar
      Fixes LPOP/RPOP wrong replies when count is 0 (#9692) · 968cd2b9
      Itamar Haber authored
      Introduced in #8179, this fixes the command's replies in the 0 count edge case.
      [BREAKING] changes the reply type when count is 0 to an empty array (instead of nil)
      Moves LPOP ... 0 fast exit path after type check to reply with WRONGTYPE
      
      (cherry picked from commit 06dd202a)
      968cd2b9
    • Oran Agra's avatar
      fix new cluster tests issues (#9657) · 3ba7c6ac
      Oran Agra authored
      Following #9483 the daily CI exposed a few problems.
      
      * The cluster creation code (uses redis-cli) is complicated to test with TLS enabled.
        for now i'm just skipping them since the tests we run there don't really need that kind of coverage
      * cluster port binding failures
        note that `find_available_port` already looks for a free cluster port
        but the code in `wait_server_started` couldn't detect the failure of binding
        (the text it greps for wasn't found in the log)
      
      (cherry picked from commit 7d6744c7)
      3ba7c6ac
    • qetu3790's avatar
      Release clients blocked on module commands in cluster resharding and down state (#9483) · 936ee017
      qetu3790 authored
      
      
      Prevent clients from being blocked forever in cluster when they block with their own module command
      and the hash slot is migrated to another master at the same time.
      These will get a redirection message when unblocked.
      Also, release clients blocked on module commands when cluster is down (same as other blocked clients)
      
      This commit adds basic tests for the main (non-cluster) redis test infra that test the cluster.
      This was done because the cluster test infra can't handle some common test features,
      but most importantly we only build the test modules with the non-cluster test suite.
      
      note that rather than really supporting cluster operations by the test infra, it was added (as dup code)
      in two files, one for module tests and one for non-modules tests, maybe in the future we'll refactor that.
      Co-authored-by: default avatarOran Agra <oran@redislabs.com>
      (cherry picked from commit 4962c552)
      936ee017
    • Huang Zhw's avatar
      Make tracking invalidation messages always after command's reply (#9422) · 0fb96d55
      Huang Zhw authored
      Tracking invalidation messages were sometimes sent in inconsistent order,
      before the command's reply rather than after.
      In addition to that, they were sometimes embedded inside other commands
      responses, like MULTI-EXEC and MGET.
      
      (cherry picked from commit fd135f3e)
      0fb96d55
    • Binbin's avatar
      Add missing pause tcl test to test_helper.tcl (#9158) · f5d8a369
      Binbin authored
      * Add keyname tags to avoid CROSSSLOT errors in external server CI
      * Use new wait_for_blocked_clients_count in pause.tcl
      
      (cherry picked from commit 5dddf496)
      f5d8a369
    • meir's avatar
      Protect any table which is reachable from globals and added globals allow list. · 11b602fb
      meir authored
      The allow list is done by setting a metatable on the global table before initializing
      any library. The metatable set the `__newindex` field to a function that check
      the allow list before adding the field to the table. Fields which is not on the
      allow list are simply ignored.
      
      After initialization phase is done we protect the global table and each table
      that might be reachable from the global table. For each table we also protect
      the table metatable if exists.
      11b602fb
    • meir's avatar
      Protect globals of evals scripts. · b2ce3719
      meir authored
      Use the new `lua_enablereadonlytable` Lua API to protect the global tables of
      evals scripts. The implemetation is easy, we simply call `lua_enablereadonlytable`
      on the global table to turn it into a readonly table.
      b2ce3719
  2. 04 Oct, 2021 7 commits
    • zhaozhao.zz's avatar
      Fix wrong offset when replica pause (#9448) · 9b25484a
      zhaozhao.zz authored
      When a replica paused, it would not apply any commands event the command comes from master, if we feed the non-applied command to replication stream, the replication offset would be wrong, and data would be lost after failover(since replica's `master_repl_offset` grows but command is not applied).
      
      To fix it, here are the changes:
      * Don't update replica's replication offset or propagate commands to sub-replicas when it's paused in `commandProcessed`.
      * Show `slave_read_repl_offset` in info reply.
      * Add an assert to make sure master client should never be blocked unless pause or module (some modules may use block way to do background (parallel) processing and forward original block module command to the replica, it's not a good way but it can work, so the assert excludes module now, but someday in future all modules should rewrite block command to propagate like what `BLPOP` does).
      
      (cherry picked from commit 1b83353d)
      9b25484a
    • Madelyn Olson's avatar
      Add test verifying PUBSUB NUMPAT behavior (#9209) · 49f8f438
      Madelyn Olson authored
      
      (cherry picked from commit 8b8f05c8)
      49f8f438
    • DarrenJiang13's avatar
      [BUGFIX] Add some missed error statistics (#9328) · 1ed0f049
      DarrenJiang13 authored
      add error counting for some missed behaviors.
      
      (cherry picked from commit 43eb0ce3)
      1ed0f049
    • Binbin's avatar
      GEO* STORE with empty src key delete the dest key and return 0, not empty array (#9271) · 530c70b0
      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>
      (cherry picked from commit 86555ae0)
      530c70b0
    • Oran Agra's avatar
      Fix ziplist and listpack overflows and truncations (CVE-2021-32627, CVE-2021-32628) · 2775a352
      Oran Agra authored
      - fix possible heap corruption in ziplist and listpack resulting by trying to
        allocate more than the maximum size of 4GB.
      - prevent ziplist (hash and zset) from reaching size of above 1GB, will be
        converted to HT encoding, that's not a useful size.
      - prevent listpack (stream) from reaching size of above 1GB.
      - XADD will start a new listpack if the new record may cause the previous
        listpack to grow over 1GB.
      - XADD will respond with an error if a single stream record is over 1GB
      - List type (ziplist in quicklist) was truncating strings that were over 4GB,
        now it'll respond with an error.
      2775a352
    • meir@redislabs.com's avatar
      Fix protocol parsing on 'ldbReplParseCommand' (CVE-2021-32672) · 3e09be56
      meir@redislabs.com authored
      The protocol parsing on 'ldbReplParseCommand' (LUA debugging)
      Assumed protocol correctness. This means that if the following
      is given:
      *1
      $100
      test
      The parser will try to read additional 94 unallocated bytes after
      the client buffer.
      This commit fixes this issue by validating that there are actually enough
      bytes to read. It also limits the amount of data that can be sent by
      the debugger client to 1M so the client will not be able to explode
      the memory.
      3e09be56
    • Oran Agra's avatar
      Prevent unauthenticated client from easily consuming lots of memory (CVE-2021-32675) · 757f8f77
      Oran Agra authored
      This change sets a low limit for multibulk and bulk length in the
      protocol for unauthenticated connections, so that they can't easily
      cause redis to allocate massive amounts of memory by sending just a few
      characters on the network.
      The new limits are 10 arguments of 16kb each (instead of 1m of 512mb)
      757f8f77
  3. 21 Jul, 2021 16 commits
    • Huang Zhw's avatar
      On 32 bit platform, the bit position of GETBIT/SETBIT/BITFIELD/BITCOUNT,BITPOS... · 835d15b5
      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 reason is bit pos 8 times of byte pos. When proto-max-bulk-len is very larger, bit pos may overflow.
      But at 64bit platform, we don't have so long string. So this bug may never happen.
      
      Additionally this commit add a test cost `512MB` memory which is tag as `large-memory`. Make freebsd ci and valgrind ci ignore this test.
      
      (cherry picked from commit 71d45287)
      835d15b5
    • Oran Agra's avatar
      Fix failing basics moduleapi test on 32bit CI (#9140) · 1d7c0e59
      Oran Agra authored
      
      (cherry picked from commit 5ffdbae1)
      1d7c0e59
    • Binbin's avatar
      SMOVE only notify dstset when the addition is successful. (#9244) · b6225371
      Binbin authored
      in case dest key already contains the member, the dest key isn't modified, so the command shouldn't invalidate watch.
      
      (cherry picked from commit 11dc4e59)
      b6225371
    • Yossi Gottlieb's avatar
      Fix CLIENT UNBLOCK crashing modules. (#9167) · 79fa5618
      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.
      
      (cherry picked from commit aa139e2f)
      79fa5618
    • Binbin's avatar
      Fix accidental deletion of sinterstore command when we meet wrong type error. (#9032) · 88655019
      Binbin authored
      SINTERSTORE would have deleted the dest key right away,
      even when later on it is bound to fail on an (WRONGTYPE) error.
      
      With this change it first picks up all the input keys, and only later
      delete the dest key if one is empty.
      
      Also add more tests for some commands.
      Mainly focus on
      - `wrong type error`:
      	expand test case (base on sinter bug) in non-store variant
      	add tests for store variant (although it exists in non-store variant, i think it would be better to have same tests)
      - the dstkey result when we meet `non-exist key (empty set)` in *store
      
      sdiff:
      - improve test case about wrong type error (the one we found in sinter, although it is safe in sdiff)
      - add test about using non-exist key (treat it like an empty set)
      sdiffstore:
      - according to sdiff test case, also add some tests about `wrong type error` and `non-exist key`
      - the different is that in sdiffstore, we will consider the `dstkey` result
      
      sunion/sunionstore add more tests (same as above)
      
      sinter/sinterstore also same as above ...
      
      (cherry picked from commit b8a5da80)
      88655019
    • Jason Elbaum's avatar
      Change return value type for ZPOPMAX/MIN in RESP3 (#8981) · fad44611
      Jason Elbaum authored
      When using RESP3, ZPOPMAX/ZPOPMIN should return nested arrays for consistency
      with other commands (e.g. ZRANGE).
      
      We do that only when COUNT argument is present (similarly to how LPOP behaves).
      for reasoning see https://github.com/redis/redis/issues/8824#issuecomment-855427955
      
      This is a breaking change only when RESP3 is used, and COUNT argument is present!
      
      (cherry picked from commit 7f342020)
      fad44611
    • Oran Agra's avatar
      Test infra, handle RESP3 attributes and big-numbers and bools (#9235) · 6cd84b64
      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.
      
      (cherry picked from commit 6a5bac30)
      6cd84b64
    • Binbin's avatar
      hrandfield and zrandmember with count should return emptyarray when key does not exist. (#9178) · c6b3966d
      Binbin authored
      
      
      due to a copy-paste bug, it used to reply with null response rather than empty array.
      this commit includes new tests that are looking at the RESP response directly in
      order to be able to tell the difference between them.
      Co-authored-by: default avatarOran Agra <oran@redislabs.com>
      (cherry picked from commit a418a2d3)
      c6b3966d
    • Oran Agra's avatar
      Tests: add a way to read raw RESP protocol reponses (#9193) · 432e0566
      Oran Agra authored
      This makes it possible to distinguish between null response and an empty
      array (currently the tests infra translates both to an empty string/list)
      
      (cherry picked from commit 7103367a)
      432e0566
    • Leibale Eidelman's avatar
      fix ZRANGESTORE - should return 0 when src points to an empty key (#9089) · 095a6e59
      Leibale Eidelman authored
      
      
      mistakenly it used to return an empty array rather than 0.
      Co-authored-by: default avatarOran Agra <oran@redislabs.com>
      (cherry picked from commit 95274f1f)
      095a6e59
    • Binbin's avatar
      ZRANDMEMBER WITHSCORES with negative COUNT may return bad score (#9162) · 11235c0e
      Binbin authored
      Return a bad score when used with negative count (or count of 1), and non-ziplist encoded zset.
      Also add test to validate the return value and cover the issue.
      
      (cherry picked from commit 4bc5a832)
      11235c0e
    • Evan's avatar
      modules: Add newlen == 0 handling to RM_StringTruncate (#3717) (#3718) · 406caa5f
      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
      
      (cherry picked from commit 1ccf2ca2)
      406caa5f
    • Huang Zhw's avatar
      Fix XTRIM or XADD with LIMIT may delete more entries than Count. (#9048) · e6df2f62
      Huang Zhw authored
      The decision to stop trimming due to LIMIT in XADD and XTRIM was after the limit was reached.
      i.e. the code was deleting **at least** that count of records (from the LIMIT argument's perspective, not the MAXLEN),
      instead of **up to** that count of records.
      see #9046
      
      (cherry picked from commit eaa7a7bb)
      e6df2f62
    • YaacovHazan's avatar
      stabilize tests that involved with load handlers (#8967) · ff272176
      YaacovHazan authored
      When test stop 'load handler' by killing the process that generating the load,
      some commands that already in the input buffer, still might be processed by the server.
      This may cause some instability in tests, that count on that no more commands
      processed after we stop the `load handler'
      
      In this commit, new proc 'wait_load_handlers_disconnected' added, to verify that no more
      cammands from any 'load handler' prossesed, by checking that the clients who
      genreate the load is disconnceted.
      
      Also, replacing check of dbsize with wait_for_ofs_sync before comparing debug digest, as
      it would fail in case the last key the workload wrote was an overridden key (not a new one).
      
      Affected tests
      Race fix:
      - failover command to specific replica works
      - Connect multiple replicas at the same time (issue #141), master diskless=$mdl, replica diskless=$sdl
      - AOF rewrite during write load: RDB preamble=$rdbpre
      
      Cleanup and speedup:
      - Test replication with blocking lists and sorted sets operations
      - Test replication with parallel clients writing in different DBs
      - Test replication partial resync: $descr (diskless: $mdl, $sdl, reconnect: $reconnect
      
      (cherry picked from commit 32a2584e)
      ff272176
    • perryitay's avatar
      Fail EXEC command in case a watched key is expired (#9194) · 3f4f9b63
      perryitay authored
      
      
      There are two issues fixed in this commit: 
      1. we want to fail the EXEC command in case there is a watched key that's logically
         expired but not yet deleted by active expire or lazy expire.
      2. we saw that currently cache time is update in every `call()` (including nested calls),
         this time is being also being use for the isKeyExpired comparison, we want to update
         the cache time only in the first call (execCommand)
      Co-authored-by: default avatarOran Agra <oran@redislabs.com>
      (cherry picked from commit ac8b1df8)
      3f4f9b63
    • Oran Agra's avatar
      Fix race in client side tracking (#9116) · abd44c83
      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.
      
      (cherry picked from commit 9b564b52)
      abd44c83
  4. 01 Jun, 2021 2 commits
  5. 03 May, 2021 2 commits
  6. 19 Apr, 2021 3 commits
    • Hanna Fadida's avatar
      Modules: adding a module type for key space notification (#8759) · 53a4d6c3
      Hanna Fadida authored
      Adding a new type mask ​for key space notification, REDISMODULE_NOTIFY_MODULE, to enable unique notifications from commands on REDISMODULE_KEYTYPE_MODULE type keys (which is currently unsupported).
      
      Modules can subscribe to a module key keyspace notification by RM_SubscribeToKeyspaceEvents,
      and clients by notify-keyspace-events of redis.conf or via the CONFIG SET, with the characters 'd' or 'A' 
      (REDISMODULE_NOTIFY_MODULE type mask is part of the '**A**ll' notation for key space notifications).
      
      Refactor: move some pubsub test infra from pubsub.tcl to util.tcl to be re-used by other tests.
      53a4d6c3
    • guybe7's avatar
      Modules: Replicate lazy-expire even if replication is not allowed (#8816) · f40ca9cb
      guybe7 authored
      Before this commit using RM_Call without "!" could cause the master
      to lazy-expire a key (delete it) but without replicating to replicas.
      This could cause the replica's memory usage to gradually grow and
      could also cause consistency issues if the master and replica have
      a clock diff.
      This bug was introduced in #8617
      
      Added a test which demonstrates that scenario.
      f40ca9cb
    • Harkrishn Patro's avatar
      ACL channels permission handling for save/load scenario. (#8794) · 7a3d1487
      Harkrishn Patro authored
      
      
      In the initial release of Redis 6.2 setting a user to only allow pubsub access to
      a specific channel, and doing ACL SAVE, resulted in an assertion when
      ACL LOAD was used. This was later changed by #8723 (not yet released),
      but still not properly resolved (now it errors instead of crash).
      
      The problem is that the server that generates an ACL file, doesn't know what
      would be the setting of the acl-pubsub-default config in the server that will load it.
      so ACL SAVE needs to always start with resetchannels directive.
      
      This should still be compatible with old acl files (from redis 6.0), and ones from earlier
      versions of 6.2 that didn't mess with channels.
      Co-authored-by: default avatarHarkrishn Patro <harkrisp@amazon.com>
      Co-authored-by: default avatarOran Agra <oran@redislabs.com>
      7a3d1487