1. 21 Aug, 2023 4 commits
    • Binbin's avatar
      BITCOUNT and BITPOS with non-existing key and illegal arguments should return error, not 0 (#11734) · 1407ac1f
      Binbin authored
      BITCOUNT and BITPOS with non-existing key will return 0 even the
      arguments are error, before this commit:
      ```
      > flushall
      OK
      > bitcount s 0
      (integer) 0
      > bitpos s 0 0 1 hello
      (integer) 0
      
      > set s 1
      OK
      > bitcount s 0
      (error) ERR syntax error
      > bitpos s 0 0 1 hello
      (error) ERR syntax error
      ```
      
      The reason is that we judged non-existing before parameter checking and
      returned. This PR fixes it, and after this commit:
      ```
      > flushall
      OK
      > bitcount s 0
      (error) ERR syntax error
      > bitpos s 0 0 1 hello
      (error) ERR syntax error
      ```
      
      Also BITPOS made the same fix as #12394, check for wrong argument, before
      checking for key.
      ```
      > lpush mylist a b c
      (integer) 3                                                                                    
      > bitpos mylist 1 a b
      (error) WRONGTYPE Operation against a key holding the wrong kind of value
      ```
      1407ac1f
    • Wen Hui's avatar
      BITCOUNT: check for argument, before checking for key (#12394) · 45d33106
      Wen Hui authored
      Generally, In any command we first check for  the argument and then check if key exist.
      
      Some of the examples are
      
      ```
      127.0.0.1:6379> getrange no-key invalid1 invalid2
      (error) ERR value is not an integer or out of range
      127.0.0.1:6379> setbit no-key 1 invalid
      (error) ERR bit is not an integer or out of range
      127.0.0.1:6379> xrange no-key invalid1 invalid2
      (error) ERR Invalid stream ID specified as stream command argument
      ```
      
      **Before change** 
      ```
      bitcount no-key invalid1 invalid2
      0
      ```
      
      **After change**
      ```
      bitcount no-key invalid1 invalid2
      (error) ERR value is not an integer or out of range
      ```
      45d33106
    • Binbin's avatar
      Fix LREM count LONG_MIN overflow minor issue (#12465) · c98a28a8
      Binbin authored
      Limit the range of LREM count to -LONG_MAX ~ LONG_MAX.
      Before the fix, passing -LONG_MAX would cause an overflow
      and would effectively be the same as passing 0. (Because
      this condition `toremove && removed == toremove `can never
      be satisfied).
      
      This is a minor fix as it shouldn't really affect users,
      more like a cleanup.
      c98a28a8
    • Yves LeBras's avatar
      config.memkeys init for consistency (#12505) · 16988208
      Yves LeBras authored
      Initializing `memkeys` to 0 for consistency and clarity.
      the config struct is anyway zeroed, but other fields are explicitly initialized.
      16988208
  2. 20 Aug, 2023 1 commit
    • meiravgri's avatar
      Signal handler attributes (#12426) · fe47c202
      meiravgri authored
      This PR purpose is to make the crash report process thread safe.
      main changes include:
      
      1. `setupSigSegvHandler()` is introduced to initialize the signal handler.
      This function first initializes the signal handler mutex (if not initialized yet)
      and then registers the process to the signal handler. 
      
      2. **sigsegvHandler** flags :
      SA_NODEFER - don't add the signal to the process signal mask. We use this
      flag because we want to be able to handle a second call to the signal manually.
      removed SA_RESETHAND: this flag resets the signal handler function upon the first
      entrance to the registered function. The reason to use this flag is to protect from
      recursively entering the signal handler by the same thread. But, it also means
      that if a second thread crashes while handling a signal, the process will be
      terminated immediately and we won't get the crash report.
      In this PR we discard this flag. The signal handler guard described below purpose
      is to solve the above issues.
      
      3. Add a **signal handler lock** with ERRORCHECK attributes. 
      The lock's purpose is to ensure that only one thread generates a crash report.
      Once a second thread enters the signal handler it will be blocked.
      We use the ERRORCHECK lock in order to protect from possible deadlock in
      case the thread handling the crash gets a signal. In the latest scenario, we log
      what we have collected until the handler crashed.
      
      At the end of the crash report we reset the signal handler SIG_DFL, with no flags, and
      rethrow the signal to generate a core dump (if enabled) and exit the process.
      
      During the work on this PR we wanted to understand the historical reasons for
      how crash is handled.
      With respect to the choice of the flag, we believe the **SA_RESETHAND** was not
      added for any specific purpose.
      **SA_ONSTACK** which is removed here from bugReportEnd(), was originally also
      set in the initial registration to signal handler, but removed in 3ada43e7. In addition,
      it was removed from another location in deee2c1e with the following description,
      which is also relevant to why it should be removed from bugReportEnd:
      
      > it seems to be some valgrind bug with SA_ONSTACK.
      > SA_ONSTACK seems unneeded since WD is not recursive (SA_NODEFER was removed),
      > also, not sure if it's even valid without a call to sigaltstack()
      fe47c202
  3. 16 Aug, 2023 4 commits
  4. 15 Aug, 2023 1 commit
  5. 10 Aug, 2023 1 commit
    • Madelyn Olson's avatar
      Fixed a bug where sequential matching ACL rules weren't compressed (#12472) · 7c179f9b
      Madelyn Olson authored
      When adding a new ACL rule was added, an attempt was made to remove
      any "overlapping" rules. However, there when a match was found, the search
      was not resumed at the right location, but instead after the original position of
      the original command.
      
      For example, if the current rules were `-config +config|get` and a rule `+config`
      was added. It would identify that `-config` was matched, but it would skip over
      `+config|get`, leaving the compacted rule `-config +config`. This would be evaluated
      safely, but looks weird.
      
      This bug can only be triggered with subcommands, since that is the only way to
      have sequential matching rules. Resolves #12470. This is also only present in 7.2.
      I think there was also a minor risk of removing another valid rule, since it would start
      the search of the next command at an arbitrary point. I couldn't find a valid offset that
      would have cause a match using any of the existing commands that have subcommands
      with another command. 
      7c179f9b
  6. 05 Aug, 2023 3 commits
    • zhaozhao.zz's avatar
      optimize the check of kill pubsub clients after modifying ACL rules (#12457) · 1b6bdff4
      zhaozhao.zz authored
      if there are no subscribers, we can ignore the operation
      1b6bdff4
    • zhaozhao.zz's avatar
      do not call handleClientsBlockedOnKeys inside yielding command (#12459) · 8226f39f
      zhaozhao.zz authored
      
      
      Fix the assertion when a busy script (timeout) signal ready keys (like LPUSH),
      and then an arbitrary client's `allow-busy` command steps into `handleClientsBlockedOnKeys`
      try wake up clients blocked on keys (like BLPOP).
      
      Reproduction process:
      1. start a redis with aof
          `./redis-server --appendonly yes`
      2. exec blpop
          `127.0.0.1:6379> blpop a 0`
      3. use another client call a busy script and this script push the blocked key
          `127.0.0.1:6379> eval "redis.call('lpush','a','b') while(1) do end" 0`
      4. user a new client call an allow-busy command like auth
          `127.0.0.1:6379> auth a`
      
      BTW, this issue also break the atomicity of script.
      
      This bug has been around for many years, the old versions only have the
      atomic problem, only 7.0/7.2 has the assertion problem.
      Co-authored-by: default avatarOran Agra <oran@redislabs.com>
      8226f39f
    • Binbin's avatar
      Fix GEOHASH / GEODIST / GEOPOS time complexity, should be O(1) (#12445) · 7af9f4b3
      Binbin authored
      GEOHASH / GEODIST / GEOPOS use zsetScore to get the score, in skiplist encoding,
      we use dictFind to get the score, which is O(1), same as ZSCORE command.
      It is not clear why these commands had O(Log(N)), and O(N) until now.
      7af9f4b3
  7. 02 Aug, 2023 2 commits
    • Meir Shpilraien (Spielrein)'s avatar
      Ensure that the function load timeout is disabled during loading from RDB/AOF... · 2ee1bbb5
      Meir Shpilraien (Spielrein) authored
      Ensure that the function load timeout is disabled during loading from RDB/AOF and on replicas. (#12451)
      
      When loading a function from either RDB/AOF or a replica, it is essential not to
      fail on timeout errors. The loading time may vary due to various factors, such as
      hardware specifications or the system's workload during the loading process.
      Once a function has been successfully loaded, it should be allowed to load from
      persistence or on replicas without encountering a timeout failure.
      
      To maintain a clear separation between the engine and Redis internals, the
      implementation refrains from directly checking the state of Redis within the
      engine itself. Instead, the engine receives the desired timeout as part of the
      library creation and duly respects this timeout value. If Redis wishes to disable
      any timeout, it can simply send a value of 0.
      2ee1bbb5
    • zhaozhao.zz's avatar
      fix false success and a memory leak for ACL selector with bad parenthesis combination (#12452) · 90ab91f0
      zhaozhao.zz authored
      
      
      When doing merge selector, we should check whether the merge
      has started (i.e., whether open_bracket_start is -1) every time.
      Otherwise, encountering an illegal selector pattern could succeed
      and also cause memory leaks, for example:
      
      ```
      acl setuser test1 (+PING (+SELECT (+DEL )
      ```
      
      The above would leak memory and succeed with only DEL being applied,
      and would now error after the fix.
      Co-authored-by: default avatarOran Agra <oran@redislabs.com>
      90ab91f0
  8. 25 Jul, 2023 2 commits
    • zhaozhao.zz's avatar
      update monitor client's memory and evict correctly (#12420) · 01eb939a
      zhaozhao.zz authored
      A bug introduced in #11657 (7.2 RC1), causes client-eviction (#8687)
      and INFO to have inaccurate memory usage metrics of MONITOR clients.
      
      Because the type in `c->type` and the type in `getClientType()` are confusing
      (in the later, `CLIENT_TYPE_NORMAL` not `CLIENT_TYPE_SLAVE`), the comment
      we wrote in `updateClientMemUsageAndBucket` was wrong, and in fact that function
      didn't skip monitor clients.
      And since it doesn't skip monitor clients, it was wrong to delete the call for it from
      `replicationFeedMonitors` (it wasn't a NOP).
      That deletion could mean that the monitor client memory usage is not always up to
      date (updated less frequently, but still a candidate for client eviction).
      01eb939a
    • nihohit's avatar
      Update request/response policies. (#12417) · 9f512017
      nihohit authored
      changing the response and request policy of a few commands,
      see https://redis.io/docs/reference/command-tips
      
      
      
      1. RANDOMKEY used to have no response policy, which means
        that when sent to multiple shards, the responses should be aggregated.
        this normally applies to commands that return arrays, but since RANDOMKEY
        replies with a simple string, it actually requires a SPECIAL response policy
        (for the client to select just one)
      2. SCAN used to have no response policy, but although the key names part of
        the response can be aggregated, the cursor part certainly can't.
      3. MSETNX had a request policy of MULTI_SHARD and response policy of AGG_MIN,
        but in fact the contract with MSETNX is that when one key exists, it returns 0
        and doesn't set any key, routing it to multiple shards would mean that if one failed
        and another succeeded, it's atomicity is broken and it's impossible to return a valid
        response to the caller.
      Co-authored-by: default avatarShachar Langbeheim <shachlan@amazon.com>
      Co-authored-by: default avatarOran Agra <oran@redislabs.com>
      9f512017
  9. 20 Jul, 2023 1 commit
    • Makdon's avatar
      redis-cli: use previous hostip when not provided by redis cluster server (#12273) · 2495b90a
      Makdon authored
      
      
      When the redis server cluster running on cluster-preferred-endpoint-type unknown-endpoint mode, and receive a request that should be redirected to another redis server node, it does not reply the hostip, but a empty host like MOVED 3999 :6381.
      
      The redis-cli would try to connect to an address without a host, which cause the issue:
      ```
      127.0.0.1:7002> set bar bar
      -> Redirected to slot [5061] located at :7000
      Could not connect to Redis at :7000: No address associated with hostname
      Could not connect to Redis at :7000: No address associated with hostname
      not connected> exit
      ```
      
      In this case, the redis-cli should use the previous hostip when there's no host provided by the server.
      
      ---------
      Co-authored-by: default avatarViktor Söderqvist <viktor.soderqvist@est.tech>
      Co-authored-by: default avatarMadelyn Olson <madelynolson@gmail.com>
      2495b90a
  10. 16 Jul, 2023 2 commits
    • George Guimares's avatar
      Replaced comment with excessive warning. · 9a3b99cb
      George Guimares authored
      The data structures in the comment are not in sync and don't need to be.
      Referring to function that handles conversion.
      9a3b99cb
    • Chen Tianjie's avatar
      Hide the comma after cport when there is no hostname. (#12411) · 91011100
      Chen Tianjie authored
      According to the format shown in https://redis.io/commands/cluster-nodes/
      ```
      <ip:port@cport[,hostname[,auxiliary_field=value]*]>
      ```
      when there is no hostname, and the auxiliary fields are hidden, the cluster topology should be
      ```
      <ip:port@cport>
      ```
      However in the code we always print the hostname even when it is an empty string, leaving an unnecessary comma tailing after cport, which is weird and conflicts with the doc.
      ```
      94ca2f6cf85228a49fde7b738ee1209de7bee325 127.0.0.1:6379@16379, myself,master - 0 0 0 connected 0-16383
      ```
      91011100
  11. 07 Jul, 2023 1 commit
    • Binbin's avatar
      Initialize cluster owner_not_claiming_slot to avoid warning (#12391) · 14f802b3
      Binbin authored
      valgrind report a Uninitialised warning:
      ```
      ==25508==  Uninitialised value was created by a heap allocation
      ==25508==    at 0x4848899: malloc (in
      /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
      ==25508==    by 0x1A35A1: ztrymalloc_usable_internal (zmalloc.c:117)
      ==25508==    by 0x1A368D: zmalloc (zmalloc.c:145)
      ==25508==    by 0x21FDEA: clusterInit (cluster.c:973)
      ==25508==    by 0x19DC09: main (server.c:7306)
      ```
      
      Introduced in #12344
      14f802b3
  12. 06 Jul, 2023 2 commits
    • zhaozhao.zz's avatar
      add an assertion to make sure numkeys is 0 in getKeysUsingKeySpecs (#12389) · aefdc57f
      zhaozhao.zz authored
      This is an addition to #12380, to prevent potential bugs when collecting
      keys from multiple commands in the future.
      Note that this function also resets numkeys in some cases.
      aefdc57f
    • Sankar's avatar
      Process loss of slot ownership in cluster bus (#12344) · 1190f25c
      Sankar authored
      Process loss of slot ownership in cluster bus
      
      When a node no longer owns a slot, it clears the bit corresponding
      to the slot in the cluster bus messages. The receiving nodes
      currently don't record the fact that the sender stopped claiming
      a slot until some other node in the cluster starts claiming the slot.
      This can cause a slot to go missing during slot migration when subjected
      to inopportune race with addition of new shards or a failover.
      This fix forces the receiving nodes to process the loss of ownership
      to avoid spreading wrong information.
      1190f25c
  13. 05 Jul, 2023 1 commit
  14. 03 Jul, 2023 1 commit
    • Lior Lahav's avatar
      Fix possible crash in command getkeys (#12380) · b7559d9f
      Lior Lahav authored
      
      
      When getKeysUsingKeySpecs processes a command with more than one key-spec,
      and called with a total of more than 256 keys, it'll call getKeysPrepareResult again,
      but since numkeys isn't updated, getKeysPrepareResult will not bother to copy key
      names from the old result (leaving these slots uninitialized). Furthermore, it did not
      consider the keys it already found when allocating more space.
      Co-authored-by: default avatarOran Agra <oran@redislabs.com>
      b7559d9f
  15. 02 Jul, 2023 1 commit
  16. 01 Jul, 2023 1 commit
  17. 29 Jun, 2023 1 commit
  18. 27 Jun, 2023 4 commits
    • judeng's avatar
      improve performance for scan command when matching pattern or data type (#12209) · 07ed0eaf
      judeng authored
      
      
      Optimized the performance of the SCAN command in a few ways:
      1. Move the key filtering (by MATCH pattern) in the scan callback,
        so as to avoid collecting them for later filtering.
      2. Reduce a many memory allocations and copying (use a reference
        to the original sds, instead of creating an robj, an excessive 2 mallocs
        and one string duplication)
      3. Compare TYPE filter directly (as integers), instead of inefficient string
        compare per key.
      4. fixed a small bug: when scan zset and hash types, maxiterations uses
        a more accurate number to avoid wrong double maxiterations.
      
      Changes **postponed** for a later version (8.0):
      1. Prepare to move the TYPE filtering to the scan callback as well. this was
        put on hold since it has side effects that can be considered a breaking
        change, which is that we will not attempt to do lazy expire (delete) a key
        that was filtered by not matching the TYPE (changing it would mean TYPE filter
        starts behaving the same as MATCH filter already does in that respect). 
      2. when the specified key TYPE filter is an unknown type, server will reply a error
        immediately instead of doing a full scan that comes back empty handed. 
      
      Benchmark result:
      For different scenarios, we obtained about 30% or more performance improvement.
      Co-authored-by: default avatarOran Agra <oran@redislabs.com>
      07ed0eaf
    • Maria Markova's avatar
      Adding of O3 on linking stage for OPTIMIZATION=-O3 cases (#12339) · b2cdf6bc
      Maria Markova authored
      Added missing O3 flag to linking stage in default option "-O3 -flto". 
      Flags doesn't lead to significant changes in performance:
      - +0.21% in geomean for all benchmarks on ICX bare-metal (256 cpus)
      - +0.33% in geomean for all benchmarks on m6i.2xlarge (16 cpus) 
      Checked on redis from Mar'30 (commit 1f76bb17 ). Comparison file is attached. 
      b2cdf6bc
    • michalbiesek's avatar
      Add RISC-V debug support (#12349) · ef4bb4e3
      michalbiesek authored
      - Add support for `getAndSetMcontextEip`
      - Add support for `logRegisters`
      ef4bb4e3
    • Binbin's avatar
      Set HIDDEN_CONFIG flag on aof-disable-auto-gc (#12355) · f58fd9e6
      Binbin authored
      aof-disable-auto-gc was created for testing purposes,
      to check if certain AOF files were actually generated
      and if they were deletedcorrectly during testing.
      
      So hiding it, see #12249 for more discussion.
      f58fd9e6
  19. 26 Jun, 2023 1 commit
    • Chen Tianjie's avatar
      Support TLS service when "tls-cluster" is not enabled and persist both plain... · 22a29935
      Chen Tianjie authored
      Support TLS service when "tls-cluster" is not enabled and persist both plain and TLS port in nodes.conf (#12233)
      
      Originally, when "tls-cluster" is enabled, `port` is set to TLS port. In order to support non-TLS clients, `pport` is used to propagate TCP port across cluster nodes. However when "tls-cluster" is disabled, `port` is set to TCP port, and `pport` is not used, which means the cluster cannot provide TLS service unless "tls-cluster" is on.
      ```
      typedef struct {
          // ...
          uint16_t port;  /* Latest known clients port (TLS or plain). */
          uint16_t pport; /* Latest known clients plaintext port. Only used if the main clients port is for TLS. */
          // ...
      } clusterNode;
      ```
      ```
      typedef struct {
          // ...
          uint16_t port;   /* TCP base port number. */
          uint16_t pport;  /* Sender TCP plaintext port, if base port is TLS */
          // ...
      } clusterMsg;
      ```
      This PR renames `port` and `pport` in `clusterNode` to `tcp_port` and `tls_port`, to record both ports no matter "tls-cluster" is enabled or disabled.
      
      This allows to provide TLS service to clients when "tls-cluster" is disabled: when displaying cluster topology, or giving `MOVED` error, server can provide TLS or TCP port according to client's connection type, no matter what type of connection cluster bus is using.
      
      For backwards compatibility, `port` and `pport` in `clusterMsg` are preserved, when "tls-cluster" is enabled, `port` is set to TLS port and `pport` is set to TCP port, when "tls-cluster" is disabled, `port` is set to TCP port and `pport` is set to TLS port (instead of 0).
      
      Also, in the nodes.conf file, a new aux field displaying an extra port is added to complete the persisted info. We may have `tls_port=xxxxx` or `tcp_port=xxxxx` in the aux field, to complete the cluster topology, while the other port is stored in the normal `<ip>:<port>` field. The format is shown below.
      ```
      <node-id> <ip>:<tcp_port>@<cport>,<hostname>,shard-id=...,tls-port=6379 myself,master - 0 0 0 connected 0-1000
      ```
      Or we can switch the position of two ports, both can be correctly resolved.
      ```
      <node-id> <ip>:<tls_port>@<cport>,<hostname>,shard-id=...,tcp-port=6379 myself,master - 0 0 0 connected 0-1000
      ```
      22a29935
  20. 25 Jun, 2023 2 commits
    • Binbin's avatar
      Move clusterBeforeSleep before blockedBeforeSleep (#12343) · 9600553e
      Binbin authored
      The new blockedBeforeSleep was added in #12337, it breaks the order in 2ecb5edf.
      
      This may be related to #2288, quoted from comment in #2288:
      ```
      Moreover the clusterBeforeSleep() call was misplaced at the end of the chain of the
      beforeSleep() call in redis.c. It should be at the top, before processing un blocking
      clients. This is exactly the reason in the specific instance of the bug as reported,
      why the state was not updated in time before clients served.
      ```
      9600553e
    • Meir Shpilraien (Spielrein)'s avatar
      Fix use after free on blocking RM_Call. (#12342) · 153f8f08
      Meir Shpilraien (Spielrein) authored
      blocking RM_Call was introduced on: #11568, It allows a module to perform
      blocking commands and get the reply asynchronously.If the command gets
      block, a special promise CallReply is returned that allow to set the unblock
      handler. The unblock handler will be called when the command invocation
      finish and it gets, as input, the command real reply.
      
      The issue was that the real CallReply was created using a stack allocated
      RedisModuleCtx which is no longer available after the unblock handler finishes.
      So if the module keeps the CallReply after the unblock handler finished, the
      CallReply holds a pointer to invalid memory and will try to access it when the
      CallReply will be released.
      
      The solution is to create the CallReply with a NULL context to make it totally
      detached and can be freed freely when the module wants.
      
      Test was added to cover this case, running the test with valgrind before the
      fix shows the use after free error. With the fix, there are no valgrind errors.
      
      unrelated: adding a missing `$rd close` in many tests in that file.
      153f8f08
  21. 22 Jun, 2023 2 commits
    • guybe7's avatar
      Modules: Unblock from within a timer coverage (#12337) · 32301999
      guybe7 authored
      Apart from adding the missing coverage, this PR also adds `blockedBeforeSleep`
      that gathers all block-related functions from `beforeSleep`
      
      The order inside `blockedBeforeSleep` is different: now `handleClientsBlockedOnKeys`
      (which may unblock clients) is called before `processUnblockedClients` (which handles
      unblocked clients).
      It makes sense to have this order.
      
      There are no visible effects of the wrong ordering, except some cleanups of the now-unblocked
      client would have  happen in the next `beforeSleep` (will now happen in the current one)
      
      The reason we even got into it is because i triggers an assertion in logresreq.c (breaking
      the assumption that `unblockClient` is called **before** actually flushing the reply to the socket):
      `handleClientsBlockedOnKeys` is called, then it calls `moduleUnblockClientOnKey`, which calls
      `moduleUnblockClient`, which adds the client to `moduleUnblockedClients` back to `beforeSleep`,
      we call `handleClientsWithPendingWritesUsingThreads`, it writes the data of buf to the client, so
      `client->bufpos` became 0
      On the next `beforeSleep`, we call `moduleHandleBlockedClients`, which calls `unblockClient`,
      which calls `reqresAppendResponse`, triggering the assert. (because the `bufpos` is 0) - see https://github.com/redis/redis/pull/12301#discussion_r1226386716
      32301999
    • Gabi Ganam's avatar
      Fix typos in comments (#12338) · 9e5f45f2
      Gabi Ganam authored
      9e5f45f2
  22. 21 Jun, 2023 2 commits