1. 22 Nov, 2022 1 commit
    • Binbin's avatar
      Fix set with duplicate elements causes sdiff to hang (#11530) · 3f8756a0
      Binbin authored
      
      
      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: default avatarOran Agra <oran@redislabs.com>
      3f8756a0
  2. 20 Nov, 2022 2 commits
    • Binbin's avatar
      sanitize dump payload: fix crash with empty set with listpack encoding (#11519) · 51887e61
      Binbin authored
      The following example will create an empty set (listpack encoding):
      ```
      > RESTORE key 0
      "\x14\x25\x25\x00\x00\x00\x00\x00\x02\x01\x82\x5F\x37\x03\x06\x01\x82\x5F\x35\x03\x82\x5F\x33\x03\x00\x01\x82\x5F\x31\x03\x82\x5F\x39\x03\x04\xA9\x08\x01\xFF\x0B\x00\xA3\x26\x49\xB4\x86\xB0\x0F\x41"
      OK
      > SCARD key
      (integer) 0
      > SRANDMEMBER key
      Error: Server closed the connection
      ```
      
      In the spirit of #9297, skip empty set when loading RDB_TYPE_SET_LISTPACK.
      Introduced in #11290
      51887e61
    • Wen Hui's avatar
      Add CONFIG SET and GET loglevel feature in Sentinel (#11214) · 2f411770
      Wen Hui authored
      Till now Sentinel allowed modifying the log level in the config file, but not at runtime.
      this makes it possible to tune the log level at runtime
      2f411770
  3. 17 Nov, 2022 1 commit
    • Ping Xie's avatar
      Introduce Shard IDs to logically group nodes in cluster mode (#10536) · 203b12e4
      Ping Xie authored
      Introduce Shard IDs to logically group nodes in cluster mode.
      1. Added a new "shard_id" field to "cluster nodes" output and nodes.conf after "hostname"
      2. Added a new PING extension to propagate "shard_id"
      3. Handled upgrade from pre-7.2 releases automatically
      4. Refactored PING extension assembling/parsing logic
      
      Behavior of Shard IDs:
      
      Replicas will always follow the shards of their reported primaries. If a primary updates its shard ID, the replica will follow. (This need not follow for cluster v2) This is not an expected use case.
      203b12e4
  4. 16 Nov, 2022 2 commits
    • sundb's avatar
      Add listpack encoding for list (#11303) · 2168ccc6
      sundb authored
      Improve memory efficiency of list keys
      
      ## Description of the feature
      The new listpack encoding uses the old `list-max-listpack-size` config
      to perform the conversion, which we can think it of as a node inside a
      quicklist, but without 80 bytes overhead (internal fragmentation included)
      of quicklist and quicklistNode structs.
      For example, a list key with 5 items of 10 chars each, now takes 128 bytes
      instead of 208 it used to take.
      
      ## Conversion rules
      * Convert listpack to quicklist
        When the listpack length or size reaches the `list-max-listpack-size` limit,
        it will be converted to a quicklist.
      * Convert quicklist to listpack
        When a quicklist has only one node, and its length or size is reduced to half
        of the `list-max-listpack-size` limit, it will be converted to a listpack.
        This is done to avoid frequent conversions when we add or remove at the bounding size or length.
          
      ## Interface changes
      1. add list entry param to listTypeSetIteratorDirection
          When list encoding is listpack, `listTypeIterator->lpi` points to the next entry of current entry,
          so when changing the direction, we need to use the current node (listTypeEntry->p) to 
          update `listTypeIterator->lpi` to the next node in the reverse direction.
      
      ## Benchmark
      ### Listpack VS Quicklist with one node
      * LPUSH - roughly 0.3% improvement
      * LRANGE - roughly 13% improvement
      
      ### Both are quicklist
      * LRANGE - roughly 3% improvement
      * LRANGE without pipeline - roughly 3% improvement
      
      From the benchmark, as we can see from the results
      1. When list is quicklist encoding, LRANGE improves performance by <5%.
      2. When list is listpack encoding, LRANGE improves performance by ~13%,
         the main enhancement is brought by `addListListpackRangeReply()`.
      
      ## Memory usage
      1M lists(key:0~key:1000000) with 5 items of 10 chars ("hellohello") each.
      shows memory usage down by 35.49%, from 214MB to 138MB.
      
      ## Note
      1. Add conversion callback to support doing some work before conversion
          Since the quicklist iterator decompresses the current node when it is released, we can 
          no longer decompress the quicklist after we convert the list.
      2168ccc6
    • Madelyn Olson's avatar
      Explicitly send function commands to monitor (#11510) · d136bf28
      Madelyn Olson authored
      Both functions and eval are marked as "no-monitor", since we want to explicitly feed in the script command before the commands generated by the script. Note that we want this behavior generally, so that commands can redact arguments before being added to the monitor.
      d136bf28
  5. 14 Nov, 2022 1 commit
  6. 12 Nov, 2022 1 commit
  7. 10 Nov, 2022 1 commit
  8. 09 Nov, 2022 3 commits
    • Viktor Söderqvist's avatar
      Listpack encoding for sets (#11290) · 4e472a1a
      Viktor Söderqvist authored
      Small sets with not only integer elements are listpack encoded, by default
      up to 128 elements, max 64 bytes per element, new config `set-max-listpack-entries`
      and `set-max-listpack-value`. This saves memory for small sets compared to using a hashtable.
      
      Sets with only integers, even very small sets, are still intset encoded (up to 1G
      limit, etc.). Larger sets are hashtable encoded.
      
      This PR increments the RDB version, and has an effect on OBJECT ENCODING
      
      Possible conversions when elements are added:
      
          intset -> listpack
          listpack -> hashtable
          intset -> hashtable
      
      Note: No conversion happens when elements are deleted. If all elements are
      deleted and then added again, the set is deleted and recreated, thus implicitly
      converted to a smaller encoding.
      4e472a1a
    • Viktor Söderqvist's avatar
      Deprecate QUIT (#11439) · 07d18706
      Viktor Söderqvist authored
      Clients should not use this command.
      Instead, clients should simply close the connection when they're not used anymore.
      Terminating a connection on the client side is preferable, as it eliminates `TIME_WAIT`
      lingering sockets on the server side.
      07d18706
    • Oran Agra's avatar
      diskless master, avoid bgsave child hung when fork parent crashes (#11463) · ccaef5c9
      Oran Agra authored
      During a diskless sync, if the master main process crashes, the child would
      have hung in `write`. This fix closes the read fd on the child side, so that if the
      parent crashes, the child will get a write error and exit.
      
      This change also fixes disk-based replication, BGSAVE and AOFRW.
      In that case the child wouldn't have been hang, it would have just kept
      running until done which may be pointless.
      
      There is a certain degree of risk here. in case there's a BGSAVE child that could
      maybe succeed and the parent dies for some reason, the old code would have let
      the child keep running and maybe succeed and avoid data loss.
      On the other hand, if the parent is restarted, it would have loaded an old rdb file
      (or none), and then the child could reach the end and rename the rdb file (data
      conflicting with what the parent has), or also have a race with another BGSAVE
      child that the new parent started.
      
      Note that i removed a comment saying a write error will be ignored in the child
      and handled by the parent (this comment was very old and i don't think relevant).
      ccaef5c9
  9. 07 Nov, 2022 1 commit
  10. 06 Nov, 2022 2 commits
  11. 04 Nov, 2022 1 commit
    • Binbin's avatar
      Introduce socket shutdown into connection type, used if a fork is active (#11376) · fac188b4
      Binbin authored
      Introduce socket `shutdown()` into connection type, and use it
      on normal socket if a fork is active. This allows us to close
      client connections when there are child processes sharing the
      file descriptors.
      
      Fixes #10077. The reason is that since the `fork()` child is holding
      the file descriptors, the `close` in `unlinkClient -> connClose`
      isn't sufficient. The client will not realize that the connection is
      disconnected until the child process ends.
      
      Let's try to be conservative and only use shutdown when the fork is active.
      fac188b4
  12. 03 Nov, 2022 2 commits
    • Madelyn Olson's avatar
      Retain ACL categories used to generate ACL for displaying them later (#11224) · c337c0a8
      Madelyn Olson authored
      Retain ACL categories used to generate ACL for displaying them later
      c337c0a8
    • Binbin's avatar
      Block some specific characters in module command names (#11434) · 8764611c
      Binbin authored
      Today we don't place any specific restrictions on module command names.
      This can cause ambiguous scenarios. For example, someone might name a
      command like "module|feature" which would be incorrectly parsed by the
      ACL system as a subcommand.
      
      In this PR, we will block some chars that we know can mess things up.
      Specifically ones that can appear ok at first and cause problems in some
      cases (we rather surface the issue right away).
      
      There are these characters:
       * ` ` (space) - issues with old inline protocol.
       * `\r`, `\n` (newline) - can mess up the protocol on acl error replies.
       * `|` - sub-commands.
       * `@` - ACL categories
       * `=`, `,` - info and client list fields.
      
      note that we decided to leave `:` out as it's handled by `getSafeInfoString`
      and is more likely to already been used by existing modules.
      8764611c
  13. 02 Nov, 2022 4 commits
    • Wen Hui's avatar
      Fix XSETID with max_deleted_entry_id issue (#11444) · 7395e370
      Wen Hui authored
      Resolve an edge case where the ID of a stream is updated retroactively
      to an ID lower than the already set max_deleted_entry_id.
      
      Currently, if we have command as below:
      **xsetid mystream 1-1 MAXDELETEDID 1-2**
      Then we will get the following error:
      **(error) ERR The ID specified in XSETID is smaller than the provided max_deleted_entry_id**
      Becuase the provided MAXDELETEDID 1-2 is greated than input last-id: 1-1
      
      Then we could assume there is a similar situation:
      step 1: we add three items in the mystream
      
      **127.0.0.1:6381> xadd mystream 1-1 a 1
      "1-1"
      127.0.0.1:6381> xadd mystream 1-2 b 2
      "1-2"
      127.0.0.1:6381> xadd mystream 1-3 c 3
      "1-3"**
      
      step 2: we could check the mystream infomation as below:
      **127.0.0.1:6381> xinfo stream mystream
       1) "length"
       2) (integer) 3
       7) "last-generated-id"
       8) "1-3"
       9) "max-deleted-entry-id"
      10) "0-0"
      
      step 3: we delete the item id 1-2 and 1-3 as below:
      **127.0.0.1:6381> xdel mystream 1-2
      (integer) 1
      127.0.0.1:6381> xdel mystream 1-3
      (integer) 1**
      
      step 4: we check the mystream information:
      127.0.0.1:6381> xinfo stream mystream
       1) "length"
       2) (integer) 1
       7) "last-generated-id"
       8) "1-3"
       9) "max-deleted-entry-id"
      10) "1-3"
      
      we could notice that the **max-deleted-entry-id update to 1-3**, so right now, if we just run:
      **xsetid mystream 1-2** 
      the above command has the same effect with **xsetid mystream 1-2  MAXDELETEDID 1-3**
      
      So we should return an error to the client that **(error) ERR The ID specified in XSETID is smaller than current max_deleted_entry_id**
      7395e370
    • Wen Hui's avatar
      Fix command BITFIELD_RO and BITFIELD argument json file, add some test cases for them (#11445) · fea9bbbe
      Wen Hui authored
      According to the source code, the commands can be executed with only key name,
      and no GET/SET/INCR operation arguments.
      change the docs to reflect that by marking these arguments as optional.
      also add tests.
      fea9bbbe
    • Binbin's avatar
      Print IP and port on cluster bus message sanity check (#11443) · e632e62e
      Binbin authored
      
      
      * Print IP and port on cluster bus message sanity check
      
      Add a print statement to indicate which IP/port is sending
      the error messages. That way we can at least check to see
      if it is a node in the cluster or some other nefarious nodes.
      
      It is proposed in #11339.
      
      Unrelated changes: the return check for connAddrPeerName should
      be -1 instead of C_ERR, although the value of C_ERR is also -1.
      Co-authored-by: default avatarMadelyn Olson <34459052+madolson@users.noreply.github.com>
      e632e62e
    • Brennan's avatar
      Re-design cluster link send buffer to improve memory management (#11343) · 47c493e0
      Brennan authored
      Re-design cluster link send queue to improve memory management
      47c493e0
  14. 29 Oct, 2022 1 commit
  15. 27 Oct, 2022 2 commits
    • Moti Cohen's avatar
      Refactor and (internally) rebrand from pause-clients to pause-actions (#11098) · c0d72262
      Moti Cohen authored
      Renamed from "Pause Clients" to "Pause Actions" since the mechanism can pause
      several actions in redis, not just clients (e.g. eviction, expiration).
      
      Previously each pause purpose (which has a timeout that's tracked separately from others purposes),
      also implicitly dictated what it pauses (reads, writes, eviction, etc). Now it is explicit, and
      the actions that are paused (bit flags) are defined separately from the purpose.
      
      - Previously, when using feature pause-client it also implicitly means to make the server static:
        - Pause replica traffic
        - Pauses eviction processing
        - Pauses expire processing
      
      Making the server static is used also for failover and shutdown. This PR internally rebrand
      pause-client API to become pause-action API. It also Simplifies pauseClients structure
      by replacing pointers array with static array.
      
      The context of this PR is to add another trigger to pause-client which will activated in case
      of OOM as throttling mechanism ([see here](https://github.com/redis/redis/issues/10907)).
      In this case we want only to pause client, and eviction actions.
      c0d72262
    • Shaya Potter's avatar
      RM_Call - only enforce OOM on scripts if 'M' flag is sent (#11425) · 38028dab
      Shaya Potter authored
      
      
      RM_Call is designed to let modules call redis commands disregarding the
      OOM state (the module is responsible to declare its command flags to redis,
      or perform the necessary checks).
      The other (new) alternative is to pass the "M" flag to RM_Call so that redis can
      OOM reject commands implicitly.
      
      However, Currently, RM_Call enforces OOM on scripts (excluding scripts that
      declared `allow-oom`) in all cases, regardless of the RM_Call "M" flag being present.
      
      This PR fixes scripts to be consistent with other commands being executed by RM_Call.
      It modifies the flow in effect treats scripts as if they if they have the ALLOW_OOM script
      flag, if the "M" flag is not passed (i.e. no OOM checking is being performed by RM_Call,
      so no OOM checking should be done on script).
      Co-authored-by: default avatarOran Agra <oran@redislabs.com>
      38028dab
  16. 26 Oct, 2022 1 commit
  17. 25 Oct, 2022 1 commit
    • Wen Hui's avatar
      Fix command GEOHASH and GEOPOS argument doc, mark member as optional (#11417) · 7c3916ae
      Wen Hui authored
      These commands take a list of members, which can be empty (i.e. running
      the command with just a key name).
      this always results in an empty array reply, so it doesn't make much sense,
      but changing it is a breaking change.
      
      This PR fixes the documentation, making the member field as optional, just makes
      sure the command format documentation is consistent with the command behavior.
      
      The command format will be:
      
      127.0.0.1:6381> GEOPOS key [member [member ...]]
      127.0.0.1:6381> GEOHASH key [member [member ...]]
      7c3916ae
  18. 24 Oct, 2022 2 commits
    • guybe7's avatar
      Cleanup: Remove redundant arg from moduleCreateArgvFromUserFormat (#11426) · f8970fdb
      guybe7 authored
      We do not need to return the length of argv because it is equal to argc, which we return anyway.
      This change makes the code cleaner and adds a comment to explain something that might not be immediately clear.
      f8970fdb
    • guybe7's avatar
      Set errno in case XADD with partial ID fails (#11424) · 737a0905
      guybe7 authored
      This is a rare failure mode of a new feature of redis 7 introduced in #9217
      (when the incremental part of the ID overflows).
      Till now, the outcome of that error was undetermined (could easily result in
      `Elements are too large to be stored` wrongly, due to unset `errno`).
      737a0905
  19. 23 Oct, 2022 2 commits
    • Moti Cohen's avatar
      Fix sentinel function that compares hostnames (if failed resolve) (#11419) · bd23b15a
      Moti Cohen authored
      Funcion sentinelAddrEqualsHostname() of sentinel makes DNS resolve
      and based on it determines if two IP addresses are equal. Now, If the
      DNS resolve command fails, the function simply returns 0, even if the
      hostnames are identical.
      
      This might become an issue in case of failover such that sentinel might
      receives from Redis instance, response to regular INFO query it sent,
      and wrongly decide that the instance is pointing to is different leader
      than the one recorded because of this function, yet hostnames are
      identical. In turn sentinel disconnects the connection between sentinel
      and valid slave which leads to -failover-abort-no-good-slave.
      See issue #11241.
      
      I managed to reproduce only part of the flow in which the function
      return wrong result and trigger +fix-slave-config.
      
      The fix is even if the function failed to resolve then compare based on
      hostnames. That is our best effort as long as the server is unavailable
      for some reason. It is fine since Redis instance cannot have multiple
      hostnames for a given setup
      bd23b15a
    • qetu3790's avatar
      Fix wrong tips when the user pass wrong # of arguments to redis.set_repl(). (#11415) · 20df1424
      qetu3790 authored
      redis.set_repl() needs one arg, but the tips says two.
      20df1424
  20. 22 Oct, 2022 2 commits
    • Binbin's avatar
      Make PFMERGE source key optional in docs, add tests with one input key, add... · 9e1b879f
      Binbin authored
      Make PFMERGE source key optional in docs, add tests with one input key, add tests on missing source keys (#11205)
      
      The following two cases will create an empty destkey HLL:
      1. called with no source keys, like `pfmerge destkey`
      2. called with non-existing source keys, like `pfmerge destkey non-existing-source-key`
      
      In the first case, in `PFMERGE`, the dest key is actually one of the source keys too.
      So `PFMERGE k1 k2` is equivalent to `SUNIONSTORE k1 k1 k2`,
      and `PFMERGE k1` is equivalent to `SUNIONSTORE k1 k1`.
      So the first case is reasonable, the source key is actually optional.
      
      And the second case, `PFMERGE` on missing keys should succeed and create an empty dest.
      This is consistent with `PFCOUNT`, and also with `SUNIONSTORE`, no need to change.
      9e1b879f
    • sundb's avatar
      Fix crash due to to reuse iterator entry after list deletion in module (#11383) · 6dd21355
      sundb authored
      
      
      In the module, we will reuse the list iterator entry for RM_ListDelete, but `listTypeDelete` will only update
      `quicklistEntry->zi` but not `quicklistEntry->node`, which will result in `quicklistEntry->node` pointing to
      a freed memory address if the quicklist node is deleted. 
      
      This PR sync `key->u.list.index` and `key->u.list.entry` to list iterator after `RM_ListDelete`.
      
      This PR also optimizes the release code of the original list iterator.
      Co-authored-by: default avatarViktor Söderqvist <viktor@zuiderkwast.se>
      6dd21355
  21. 19 Oct, 2022 1 commit
  22. 18 Oct, 2022 5 commits
    • guybe7's avatar
      Blocked module clients should be aware when a key is deleted (#11310) · b57fd010
      guybe7 authored
      The use case is a module that wants to implement a blocking command on a key that
      necessarily exists and wants to unblock the client in case the key is deleted (much like
      what we implemented for XREADGROUP in #10306)
      
      New module API:
      * RedisModule_BlockClientOnKeysWithFlags
      
      Flags:
      * REDISMODULE_BLOCK_UNBLOCK_NONE
      * REDISMODULE_BLOCK_UNBLOCK_DELETED
      
      ### Detailed description of code changes
      
      blocked.c:
      1. Both module and stream functions are called whether the key exists or not, regardless of
        its type. We do that in order to allow modules/stream to unblock the client in case the key
        is no longer present or has changed type (the behavior for streams didn't change, just code
        that moved into serveClientsBlockedOnStreamKey)
      2. Make sure afterCommand is called in serveClientsBlockedOnKeyByModule, in order to propagate
        actions from moduleTryServeClientBlockedOnKey.
      3. handleClientsBlockedOnKeys: call propagatePendingCommands directly after lookupKeyReadWithFlags
        to prevent a possible lazy-expire DEL from being mixed with any command propagated by the
        preceding functions.
      4. blockForKeys: Caller can specifiy that it wants to be awakened if key is deleted.
         Minor optimizations (use dictAddRaw).
      5. signalKeyAsReady became signalKeyAsReadyLogic which can take a boolean in case the key is deleted.
        It will only signal if there's at least one client that awaits key deletion (to save calls to
        handleClientsBlockedOnKeys).
        Minor optimizations (use dictAddRaw)
      
      db.c:
      1. scanDatabaseForDeletedStreams is now scanDatabaseForDeletedKeys and will signalKeyAsReady
        for any key that was removed from the database or changed type. It is the responsibility of the code
        in blocked.c to ignore or act on deleted/type-changed keys.
      2. Use the new signalDeletedKeyAsReady where needed
      
      blockedonkey.c + tcl:
      1. Added test of new capabilities (FSL.BPOPGT now requires the key to exist in order to work)
      b57fd010
    • Meir Shpilraien (Spielrein)'s avatar
      Avoid saving module aux on RDB if no aux data was saved by the module. (#11374) · b43f2548
      Meir Shpilraien (Spielrein) authored
      ### Background
      
      The issue is that when saving an RDB with module AUX data, the module AUX metadata
      (moduleid, when, ...) is saved to the RDB even though the module did not saved any actual data.
      This prevent loading the RDB in the absence of the module (although there is no actual data in
      the RDB that requires the module to be loaded).
      
      ### Solution
      
      The solution suggested in this PR is that module AUX will be saved on the RDB only if the module
      actually saved something during `aux_save` function.
      
      To support backward compatibility, we introduce `aux_save2` callback that acts the same as
      `aux_save` with the tiny change of avoid saving the aux field if no data was actually saved by
      the module. Modules can use the new API to make sure that if they have no data to save,
      then it will be possible to load the created RDB even without the module.
      
      ### Concerns
      
      A module may register for the aux load and save hooks just in order to be notified when
      saving or loading starts or completed (there are better ways to do that, but it still possible
      that someone used it).
      
      However, if a module didn't save a single field in the save callback, it means it's not allowed
      to read in the read callback, since it has no way to distinguish between empty and non-empty
      payloads. furthermore, it means that if the module did that, it must never change it, since it'll
      break compatibility with it's old RDB files, so this is really not a valid use case.
      
      Since some modules (ones who currently save one field indicating an empty payload), need
      to know if saving an empty payload is valid, and if Redis is gonna ignore an empty payload
      or store it, we opted to add a new API (rather than change behavior of an existing API and
      expect modules to check the redis version)
      
      ### Technical Details
      
      To avoid saving AUX data on RDB, we change the code to first save the AUX metadata
      (moduleid, when, ...) into a temporary buffer. The buffer is then flushed to the rio at the first
      time the module makes a write operation inside the `aux_save` function. If the module saves
      nothing (and `aux_save2` was used), the entire temporary buffer is simply dropped and no
      data about this AUX field is saved to the RDB. This make it possible to load the RDB even in
      the absence of the module.
      
      Test was added to verify the fix.
      b43f2548
    • Shuning's avatar
      keyIsExpired checks server.loading before calling getExpire (#11393) · 20d286f7
      Shuning authored
      
      
      Seems excessive to call getExpire if we don't need it.
      This can maybe have some speedup on AOF file loading (saving a dictFind call)
      Co-authored-by: default avatarlvshuning <lvshuning@meituan.com>
      20d286f7
    • DarrenJiang13's avatar
      fix malloc macro in listpack.c (#11398) · ba1f09d3
      DarrenJiang13 authored
      fix some malloc macros in `listpack.c`.
      listpack has it's own malloc aliases, but in some places normal redis malloc calls have slipped in.
      ba1f09d3
    • Binbin's avatar
      Bump codespell from 2.2.1 to 2.2.2 in /.codespell (#11399) · a9d561af
      Binbin authored
      And fix a few newly detected typo.
      Closes #11394
      a9d561af
  23. 16 Oct, 2022 1 commit
    • Shaya Potter's avatar
      Unify ACL failure error messaging. (#11160) · 3193f086
      Shaya Potter authored
      Motivation: for applications that use RM ACL verification functions, they would
      want to return errors back to the user, in ways that are consistent with Redis.
      While investigating how we should return ACL errors to the user, we realized that
      Redis isn't consistent, and currently returns ACL error strings in 3 primary ways.
      
      [For the actual implications of this change, see the "Impact" section at the bottom]
      
      1. how it returns an error when calling a command normally
         ACL_DENIED_CMD -> "this user has no permissions to run the '%s' command"
         ACL_DENIED_KEY -> "this user has no permissions to access one of the keys used as arguments"
         ACL_DENIED_CHANNEL -> "this user has no permissions to access one of the channels used as arguments"
      
      2. how it returns an error when calling via 'acl dryrun' command
         ACL_DENIED_CMD ->  "This user has no permissions to run the '%s' command"
         ACL_DENIED_KEY -> "This user has no permissions to access the '%s' key"
         ACL_DENIED_CHANNEL -> "This user has no permissions to access the '%s' channel"
      
      3. how it returns an error via RM_Call (and scripting is similar).
         ACL_DENIED_CMD -> "can't run this command or subcommand";
         ACL_DENIED_KEY -> "can't access at least one of the keys mentioned in the command arguments";
         ACL_DENIED_CHANNEL -> "can't publish to the channel mentioned in the command";
         
         In addition, if one wants to use RM_Call's "dry run" capability instead of the RM ACL
         functions directly, one also sees a different problem than it returns ACL errors with a -ERR,
         not a -PERM, so it can't be returned directly to the caller.
      
      This PR modifies the code to generate a base message in a common manner with the ability
      to set verbose flag for acl dry run errors, and keep it unset for normal/rm_call/script cases
      
      ```c
      sds getAclErrorMessage(int acl_res, user *user, struct redisCommand *cmd, sds errored_val, int verbose) {
          switch (acl_res) {
          case ACL_DENIED_CMD:
              return sdscatfmt(sdsempty(), "User %S has no permissions to run "
                                           "the '%S' command", user->name, cmd->fullname);
          case ACL_DENIED_KEY:
              if (verbose) {
                  return sdscatfmt(sdsempty(), "User %S has no permissions to access "
                                               "the '%S' key", user->name, errored_val);
              } else {
                  return sdsnew("No permissions to access a key");
              }
          case ACL_DENIED_CHANNEL:
              if (verbose) {
                  return sdscatfmt(sdsempty(), "User %S has no permissions to access "
                                               "the '%S' channel", user->name, errored_val);
              } else {
                  return sdsnew("No permissions to access a channel");
              }
          }
      ```
      
      The caller can append/prepend the message (adding NOPERM for normal/RM_Call or indicating it's within a script).
      
      Impact:
      - Plain commands, as well as scripts and RM_Call now include the user name.
      - ACL DRYRUN remains the only one that's verbose (mentions the offending channel or key name)
      - Changes RM_Call ACL errors from being a `-ERR` to being `-NOPERM` (besides for textual changes)
        **This somewhat a breaking change, but it only affects the RM_Call with both `C` and `E`, or `D`**
      - Changes ACL errors in scripts textually from being
        `The user executing the script <old non unified text>`
        to
        `ACL failure in script: <new unified text>`
      3193f086