1. 12 Oct, 2022 1 commit
    • Meir Shpilraien (Spielrein)'s avatar
      Fix crash on RM_Call inside module load (#11346) · eb6accad
      Meir Shpilraien (Spielrein) authored
      PR #9320 introduces initialization order changes. Now cluster is initialized after modules.
      This changes causes a crash if the module uses RM_Call inside the load function
      on cluster mode (the code will try to access `server.cluster` which at this point is NULL).
      
      To solve it, separate cluster initialization into 2 phases:
      1. Structure initialization that happened before the modules initialization
      2. Listener initialization that happened after.
      
      Test was added to verify the fix.
      eb6accad
  2. 09 Oct, 2022 2 commits
    • Binbin's avatar
      Fix TIME command microseconds overflow under 32-bits (#11368) · 1cc511d7
      Binbin authored
      The old `server.unixtime*1000000` will overflow in 32-bits.
      This was introduced in #10300 (not released).
      1cc511d7
    • Binbin's avatar
      Freeze time sampling during command execution, and scripts (#10300) · 35b3fbd9
      Binbin authored
      Freeze time during execution of scripts and all other commands.
      This means that a key is either expired or not, and doesn't change
      state during a script execution. resolves #10182
      
      This PR try to add a new `commandTimeSnapshot` function.
      The function logic is extracted from `keyIsExpired`, but the related
      calls to `fixed_time_expire` and `mstime()` are removed, see below.
      
      In commands, we will avoid calling `mstime()` multiple times
      and just use the one that sampled in call. The background is,
      e.g. using `PEXPIRE 1` with valgrind sometimes result in the key
      being deleted rather than expired. The reason is that both `PEXPIRE`
      command and `checkAlreadyExpired` call `mstime()` separately.
      
      There are other more important changes in this PR:
      1. Eliminate `fixed_time_expire`, it is no longer needed. 
         When we want to sample time we should always use a time snapshot. 
         We will use `in_nested_call` instead to update the cached time in `call`.
      2. Move the call for `updateCachedTime` from `serverCron` to `afterSleep`.
          Now `commandTimeSnapshot` will always return the sample time, the
          `lookupKeyReadWithFlags` call in `getNodeByQuery` will get a outdated
          cached time (because `processCommand` is out of the `call` context).
          We put the call to `updateCachedTime` in `aftersleep`.
      3. Cache the time each time the module lock Redis.
          Call `updateCachedTime` in `moduleGILAfterLock`, affecting `RM_ThreadSafeContextLock`
          and `RM_ThreadSafeContextTryLock`
      
      Currently the commandTimeSnapshot change affects the following TTL commands:
      - SET EX / SET PX
      - EXPIRE / PEXPIRE
      - SETEX / PSETEX
      - GETEX EX / GETEX PX
      - TTL / PTTL
      - EXPIRETIME / PEXPIRETIME
      - RESTORE key TTL
      
      And other commands just use the cached mstime (including TIME).
      
      This is considered to be a breaking change since it can break a script
      that uses a loop to wait for a key to expire.
      35b3fbd9
  3. 07 Oct, 2022 1 commit
  4. 27 Sep, 2022 1 commit
    • Steffen Moser's avatar
      Fixing compilation by removing flock() when compiling on Solaris (#11327) · 6aab4cb7
      Steffen Moser authored
      SunOS/Solaris and its relatives don't support the flock() function.
      While "redis" has been excluding setting up the lock using flock() on the cluster
      configuration file when compiling under Solaris, it was still using flock() in the
      unlock call while shutting down. 
      
      This pull request eliminates the flock() call also in the unlocking stage
      for Oracle Solaris and its relatives.
      
      Fix compilation regression from #10912
      6aab4cb7
  5. 21 Sep, 2022 1 commit
  6. 28 Aug, 2022 1 commit
    • Shaya Potter's avatar
      Improve cmd_flags for script/functions in RM_Call (#11159) · bed6d759
      Shaya Potter authored
      When RM_Call was used with `M` (reject OOM), `W` (reject writes),
      as well as `S` (rejecting stale or write commands in "Script mode"),
      it would have only checked the command flags, but not the declared
      script flag in case it's a command that runs a script.
      
      Refactoring: extracts out similar code in server.c's processCommand
      to be usable in RM_Call as well.
      bed6d759
  7. 26 Aug, 2022 1 commit
    • Moti Cohen's avatar
      Removing old redundant code from bio.c (#11136) · 246f44d7
      Moti Cohen authored
      * Remove redundant array bio_pending[]. Value at index i identically reflects the
      length of list bio_jobs[i]. Better use listLength() instead and discard this array.
      (no critical section issues to concern about).
      
      changed returned value of bioPendingJobsOfType() from "long long" to "long".
      
      Remove unused API. Maybe we will use this API later.
      246f44d7
  8. 24 Aug, 2022 1 commit
    • Meir Shpilraien (Spielrein)'s avatar
      Reverts most of the changes of #10969 (#11178) · c1bd61a4
      Meir Shpilraien (Spielrein) authored
      The PR reverts the changes made on #10969.
      The reason for revert was trigger because of occasional test failure
      that started after the PR was merged.
      
      The issue is that if there is a lazy expire during the command invocation,
      the `del` command is added to the replication stream after the command
      placeholder. So the logical order on the primary is:
      
      * Delete the key (lazy expiration)
      * Command invocation
      
      But the replication stream gets it the other way around:
      
      * Command invocation (because the command is written into the placeholder)
      * Delete the key (lazy expiration)
      
      So if the command write to the key that was just lazy expired we will get
      inconsistency between primary and replica.
      
      One solution we considered is to add another lazy expire replication stream
      and write all the lazy expire there. Then when replicating, we will replicate the
      lazy expire replication stream first. This will solve this specific test failure but
      we realize that the issues does not ends here and the more we dig the more
      problems we find.One of the example we thought about (that can actually
      crashes Redis) is as follow:
      
      * User perform SINTERSTORE
      * When Redis tries to fetch the second input key it triggers lazy expire
      * The lazy expire trigger a module logic that deletes the first input key
      * Now Redis hold the robj of the first input key that was actually freed
      
      We believe we took the wrong approach and we will come up with another
      PR that solve the problem differently, for now we revert the changes so we
      will not have the tests failure.
      
      Notice that not the entire code was revert, some parts of the PR are changes
      that we would like to keep. The changes that **was** reverted are:
      
      * Saving a placeholder for replication at the beginning of the command (`call` function)
      * Order of the replication stream on active expire and eviction (we will decide how
        to handle it correctly on follow up PR)
      * `Spop` changes are no longer needed (because we reverted the placeholder code)
      
      Changes that **was not** reverted:
      
      * On expire/eviction, wrap the `del` and the notification effect in a multi exec.
      * `PropagateNow` function can still accept a special dbid, -1, indicating not to replicate select.
      * Keep optimisation for reusing the `alsoPropagate` array instead of allocating it each time.
      
      Tests:
      
      * All tests was kept and only few tests was modify to work correctly with the changes
      * Test was added to verify that the revert fixes the issues.
      c1bd61a4
  9. 23 Aug, 2022 1 commit
    • Oran Agra's avatar
      Build TLS as a loadable module · 4faddf18
      Oran Agra authored
      * Support BUILD_TLS=module to be loaded as a module via config file or
        command line. e.g. redis-server --loadmodule redis-tls.so
      * Updates to redismodule.h to allow it to be used side by side with
        server.h by defining REDISMODULE_CORE_MODULE
      * Changes to server.h, redismodule.h and module.c to avoid repeated
        type declarations (gcc 4.8 doesn't like these)
      * Add a mechanism for non-ABI neutral modules (ones who include
        server.h) to refuse loading if they detect not being built together with
        redis (release.c)
      * Fix wrong signature of RedisModuleDefragFunc, this could break
        compilation of a module, but not the ABI
      * Move initialization of listeners in server.c to be after loading
        the modules
      * Config TLS after initialization of listeners
      * Init cluster after initialization of listeners
      * Add TLS module to CI
      * Fix a test suite race conditions:
        Now that the listeners are initialized later, it's not sufficient to
        wait for the PID message in the log, we need to wa...
      4faddf18
  10. 22 Aug, 2022 9 commits
    • zhenwei pi's avatar
      Add listeners info string for 'INFO' command · 0c4d2fcc
      zhenwei pi authored
      
      
      Suggested by Oran, add necessary listeners information in 'INFO'
      command. It would be helpful for debug.
      
      Example of this:
      127.0.0.1:6379> INFO SERVER
      redis_version:255.255.255
      ...
      listener0:name=tcp,bind=127.0.0.1,port=6380
      listener1:name=unix,bind=/run/redis.sock
      listener2:name=tls,bind=127.0.0.1,port=6379
      ...
      Signed-off-by: default avatarzhenwei pi <pizhenwei@bytedance.com>
      0c4d2fcc
    • zhenwei pi's avatar
      Introduce .listen into connection type · 0b27cfe3
      zhenwei pi authored
      
      
      Introduce listen method into connection type, this allows no hard code
      of listen logic. Originally, we initialize server during startup like
      this:
          if (server.port)
              listenToPort(server.port,&server.ipfd);
          if (server.tls_port)
              listenToPort(server.port,&server.tlsfd);
          if (server.unixsocket)
              anetUnixServer(...server.unixsocket...);
      
          ...
          if (createSocketAcceptHandler(&server.ipfd, acceptTcpHandler) != C_OK)
          if (createSocketAcceptHandler(&server.tlsfd, acceptTcpHandler) != C_OK)
          if (createSocketAcceptHandler(&server.sofd, acceptTcpHandler) != C_OK)
          ...
      
      If a new connection type gets supported, we have to add more hard code
      to setup listener.
      
      Introduce .listen and refactor listener, and Unix socket supports this.
      this allows to setup listener arguments and create listener in a loop.
      
      What's more, '.listen' is defined in connection.h, so we should include
      server.h to import 'struct socketFds', but server.h has already include
      'connection.h'. To avoid including loop(also to make code reasonable),
      define 'struct connListener' in connection.h instead of 'struct socketFds'
      in server.h. This leads this commit to get more changes.
      
      There are more fields in 'struct connListener', hence it's possible to
      simplify changeBindAddr & applyTLSPort() & updatePort() into a single
      logic: update the listener config from the server.xxx, and re-create
      the listener.
      
      Because of the new field 'priv' in struct connListener, we expect to pass
      this to the accept handler(even it's not used currently), this may be used
      in the future.
      Signed-off-by: default avatarzhenwei pi <pizhenwei@bytedance.com>
      0b27cfe3
    • zhenwei pi's avatar
      Use connection name of string · 45617385
      zhenwei pi authored
      
      
      Suggested by Oran, use an array to store all the connection types
      instead of a linked list, and use connection name of string. The index
      of a connection is dynamically allocated.
      
      Currently we support max 8 connection types, include:
      - tcp
      - unix socket
      - tls
      
      and RDMA is in the plan, then we have another 4 types to support, it
      should be enough in a long time.
      
      Introduce 3 functions to get connection type by a fast path:
      - connectionTypeTcp()
      - connectionTypeTls()
      - connectionTypeUnix()
      
      Note that connectionByType() is designed to use only in unlikely code path.
      Signed-off-by: default avatarzhenwei pi <pizhenwei@bytedance.com>
      45617385
    • zhenwei pi's avatar
      Introduce unix socket connection type · eb94d6d3
      zhenwei pi authored
      
      
      Unix socket uses different accept handler/create listener from TCP,
      to hide these difference to avoid hard code, use a new unix socket
      connection type. Also move 'acceptUnixHandler' into unix.c.
      
      Currently, the connection framework becomes like following:
      
                         uplayer
                            |
                     connection layer
                       /    |     \
                     TCP   Unix   TLS
      
      It's possible to build Unix socket support as a shared library, and
      load it dynamically. Because TCP and Unix socket don't require any
      heavy dependencies or overheads, we build them into Redis statically.
      Signed-off-by: default avatarzhenwei pi <pizhenwei@bytedance.com>
      eb94d6d3
    • zhenwei pi's avatar
      Abstract accept handler · 0ae02ce9
      zhenwei pi authored
      
      
      Abstract accept handler for socket&TLS, and add helper function
      'connAcceptHandler' to get accept handler by specified type.
      
      Also move acceptTcpHandler into socket.c, and move
      acceptTLSHandler into tls.c.
      Signed-off-by: default avatarzhenwei pi <pizhenwei@bytedance.com>
      0ae02ce9
    • zhenwei pi's avatar
      Use socketFds for unix · 41fff55d
      zhenwei pi authored
      
      
      socketFds is also suitable for Unix socket, then we can use
      'createSocketAcceptHandler' to create accept handler.
      And then, we can abstract accept handler in the future.
      Signed-off-by: default avatarzhenwei pi <pizhenwei@bytedance.com>
      41fff55d
    • zhenwei pi's avatar
      Introduce pending data for connection type · 709b55b0
      zhenwei pi authored
      
      
      Introduce .has_pending_data and .process_pending_data for connection
      type, and hide tlsHasPendingData() and tlsProcessPendingData(). Also
      set .has_pending_data and .process_pending_data as NULL explicitly in
      socket.c.
      Signed-off-by: default avatarzhenwei pi <pizhenwei@bytedance.com>
      709b55b0
    • zhenwei pi's avatar
      Introduce connection layer framework · 8234a512
      zhenwei pi authored
      
      
      Use connTypeRegister() to register a connection type into redis, and
      query connection by connectionByType() via type.
      
      With this change, we can hide TLS specified methods into connection
      type:
      - void tlsInit(void);
      - void tlsCleanup(void);
      - int tlsConfigure(redisTLSContextConfig *ctx_config);
      - int isTlsConfigured(void);
      
      Merge isTlsConfigured & tlsConfigure, use an argument *reconfigure*
      to distinguish:
         tlsConfigure(&server.tls_ctx_config)
      -> onnTypeConfigure(CONN_TYPE_TLS, &server.tls_ctx_config, 1)
      
         isTlsConfigured() && tlsConfigure(&server.tls_ctx_config)
      -> connTypeConfigure(CONN_TYPE_TLS, &server.tls_ctx_config, 0)
      
      Finally, we can remove USE_OPENSSL from config.c. If redis is built
      without TLS, and still run redis with TLS, then redis reports:
       # Missing implement of connection type 1
       # Failed to configure TLS. Check logs for more info.
      
      The log can be optimised, let's leave it in the future. Maybe we can
      use connection type as a string.
      
      Although uninitialized fields of a static struct are zero, we still
      set them as NULL explicitly in socket.c, let them clear to read & maintain:
          .init = NULL,
          .cleanup = NULL,
          .configure = NULL,
      Signed-off-by: default avatarzhenwei pi <pizhenwei@bytedance.com>
      8234a512
    • zhenwei pi's avatar
      Introduce connAddr · bff7ecc7
      zhenwei pi authored
      
      
      Originally, connPeerToString is designed to get the address info from
      socket only(for both TCP & TLS), and the API 'connPeerToString' is
      oriented to operate a FD like:
      int connPeerToString(connection *conn, char *ip, size_t ip_len, int *port) {
          return anetFdToString(conn ? conn->fd : -1, ip, ip_len, port, FD_TO_PEER_NAME);
      }
      
      Introduce connAddr and implement .addr method for socket and TLS,
      thus the API 'connAddr' and 'connFormatAddr' become oriented to a
      connection like:
      static inline int connAddr(connection *conn, char *ip, size_t ip_len, int *port, int remote) {
          if (conn && conn->type->addr) {
              return conn->type->addr(conn, ip, ip_len, port, remote);
          }
      
          return -1;
      }
      
      Also remove 'FD_TO_PEER_NAME' & 'FD_TO_SOCK_NAME', use a boolean type
      'remote' to get local/remote address of a connection.
      
      With these changes, it's possible to support the other connection
      types which does not use socket(Ex, RDMA).
      
      Thanks to Oran for suggestions!
      Signed-off-by: default avatarzhenwei pi <pizhenwei@bytedance.com>
      bff7ecc7
  11. 21 Aug, 2022 1 commit
    • yourtree's avatar
      Support setlocale via CONFIG operation. (#11059) · ca6aeadf
      yourtree authored
      
      
      Till now Redis officially supported tuning it via environment variable see #1074.
      But we had other requests to allow changing it at runtime, see #799, and #11041.
      
      Note that `strcoll()` is used as Lua comparison function and also for comparison of
      certain string objects in Redis, which leads to a problem that, in different regions,
      for some characters, the result may be different. Below is an example.
      ```
      127.0.0.1:6333> SORT test alpha
      1) "<"
      2) ">"
      3) ","
      4) "*"
      127.0.0.1:6333> CONFIG GET locale-collate
      1) "locale-collate"
      2) ""
      127.0.0.1:6333> CONFIG SET locale-collate 1
      (error) ERR CONFIG SET failed (possibly related to argument 'locale')
      127.0.0.1:6333> CONFIG SET locale-collate C
      OK
      127.0.0.1:6333> SORT test alpha
      1) "*"
      2) ","
      3) "<"
      4) ">"
      ```
      That will cause accidental code compatibility issues for Lua scripts and some
      Redis commands. This commit creates a new config parameter to control the
      local environment which only affects `Collate` category. Above shows how it
      affects `SORT` command, and below shows the influence on Lua scripts.
      ```
      127.0.0.1:6333> CONFIG GET locale-collate
      1) " locale-collate"
      2) "C"
      127.0.0.1:6333> EVAL "return ',' < '*'" 0
      (nil)
      127.0.0.1:6333> CONFIG SET locale-collate ""
      OK
      127.0.0.1:6333> EVAL "return ',' < '*'" 0
      (integer) 1
      ```
      Co-authored-by: default avatarcalvincjli <calvincjli@tencent.com>
      Co-authored-by: default avatarOran Agra <oran@redislabs.com>
      ca6aeadf
  12. 18 Aug, 2022 2 commits
    • guybe7's avatar
      Repurpose redisCommandArg's name as the unique ID (#11051) · 223046ec
      guybe7 authored
      This PR makes sure that "name" is unique for all arguments in the same
      level (i.e. all args of a command and all args within a block/oneof).
      This means several argument with identical meaning can be referred to together,
      but also if someone needs to refer to a specific one, they can use its full path.
      
      In addition, the "display_text" field has been added, to be used by redis.io
      in order to render the syntax of the command (for the vast majority it is
      identical to "name" but sometimes we want to use a different string
      that is not "name")
      The "display" field is exposed via COMMAND DOCS and will be present
      for every argument, except "oneof" and "block" (which are container
      arguments)
      
      Other changes:
      1. Make sure we do not have any container arguments ("oneof" or "block")
         that contain less than two sub-args (otherwise it doesn't make sense)
      2. migrate.json: both AUTH and AUTH2 should not be "optional"
      3. arg names cannot contain underscores, and force the usage of hyphens
        (most of these were a result of the script that generated the initial json files
        from redis.io commands.json). 
      223046ec
    • Meir Shpilraien (Spielrein)'s avatar
      Fix replication inconsistency on modules that uses key space notifications (#10969) · 508a1388
      Meir Shpilraien (Spielrein) authored
      Fix replication inconsistency on modules that uses key space notifications.
      
      ### The Problem
      
      In general, key space notifications are invoked after the command logic was
      executed (this is not always the case, we will discuss later about specific
      command that do not follow this rules). For example, the `set x 1` will trigger
      a `set` notification that will be invoked after the `set` logic was performed, so
      if the notification logic will try to fetch `x`, it will see the new data that was written.
      Consider the scenario on which the notification logic performs some write
      commands. for example, the notification logic increase some counter,
      `incr x{counter}`, indicating how many times `x` was changed.
      The logical order by which the logic was executed is has follow:
      
      ```
      set x 1
      incr x{counter}
      ```
      
      The issue is that the `set x 1` command is added to the replication buffer
      at the end of the command invocation (specifically after the key space
      notification logic was invoked and performed the `incr` command).
      The replication/aof sees the commands in the wrong order:
      
      ```
      incr x{counter}
      set x 1
      ```
      
      In this specific example the order is less important.
      But if, for example, the notification would have deleted `x` then we would
      end up with primary-replica inconsistency.
      
      ### The Solution
      
      Put the command that cause the notification in its rightful place. In the
      above example, the `set x 1` command logic was executed before the
      notification logic, so it should be added to the replication buffer before
      the commands that is invoked by the notification logic. To achieve this,
      without a major code refactoring, we save a placeholder in the replication
      buffer, when finishing invoking the command logic we check if the command
      need to be replicated, and if it does, we use the placeholder to add it to the
      replication buffer instead of appending it to the end.
      
      To be efficient and not allocating memory on each command to save the
      placeholder, the replication buffer array was modified to reuse memory
      (instead of allocating it each time we want to replicate commands).
      Also, to avoid saving a placeholder when not needed, we do it only for
      WRITE or MAY_REPLICATE commands.
      
      #### Additional Fixes
      
      * Expire and Eviction notifications:
        * Expire/Eviction logical order was to first perform the Expire/Eviction
          and then the notification logic. The replication buffer got this in the
          other way around (first notification effect and then the `del` command).
          The PR fixes this issue.
        * The notification effect and the `del` command was not wrap with
          `multi-exec` (if needed). The PR also fix this issue.
      * SPOP command:
        * On spop, the `spop` notification was fired before the command logic
          was executed. The change in this PR would have cause the replication
          order to be change (first `spop` command and then notification `logic`)
          although the logical order is first the notification logic and then the
          `spop` logic. The right fix would have been to move the notification to
          be fired after the command was executed (like all the other commands),
          but this can be considered a breaking change. To overcome this, the PR
          keeps the current behavior and changes the `spop` code to keep the right
          logical order when pushing commands to the replication buffer. Another PR
          will follow to fix the SPOP properly and match it to the other command (we
          split it to 2 separate PR's so it will be easy to cherry-pick this PR to 7.0 if
          we chose to).
      
      #### Unhanded Known Limitations
      
      * key miss event:
        * On key miss event, if a module performed some write command on the
          event (using `RM_Call`), the `dirty` counter would increase and the read
          command that cause the key miss event would be replicated to the replication
          and aof. This problem can also happened on a write command that open
          some keys but eventually decides not to perform any action. We decided
          not to handle this problem on this PR because the solution is complex
          and will cause additional risks in case we will want to cherry-pick this PR.
          We should decide if we want to handle it in future PR's. For now, modules
          writers is advice not to perform any write commands on key miss event.
      
      #### Testing
      
      * We already have tests to cover cases where a notification is invoking write
        commands that are also added to the replication buffer, the tests was modified
        to verify that the replica gets the command in the correct logical order.
      * Test was added to verify that `spop` behavior was kept unchanged.
      * Test was added to verify key miss event behave as expected.
      * Test was added to verify the changes do not break lazy expiration.
      
      #### Additional Changes
      
      * `propagateNow` function can accept a special dbid, -1, indicating not
        to replicate `select`. We use this to replicate `multi/exec` on `propagatePendingCommands`
        function. The side effect of this change is that now the `select` command
        will appear inside the `multi/exec` block on the replication stream (instead of
        outside of the `multi/exec` block). Tests was modified to match this new behavior.
      508a1388
  13. 09 Aug, 2022 1 commit
  14. 04 Aug, 2022 2 commits
    • Binbin's avatar
      errno cleanup around rdbLoad (#11042) · 4505eb18
      Binbin authored
      This is an addition to #11039, which cleans up rdbLoad* related errno. Remove the
      errno print from the outer message (may be invalid since errno may have been overwritten).
      
      Our aim should be the code that detects the error and knows which system call
      triggered it, is the one to print errno, and not the code way up above (in some cases
      a result of a logical error and not a system one).
      
      Remove the code to update errno in rdbLoadRioWithLoadingCtx, signature check
      and the rdb version check, in these cases, we do print the error message.
      The caller dose not have the specific logic for handling EINVAL.
      
      Small fix around rdb-preamble AOF: A truncated RDB is considered a failure,
      not handled the same as a truncated AOF file.
      4505eb18
    • filipe oliveira's avatar
      Avoid the sdslen() on shared.crlf given we know its size beforehand. Improve... · 6686c6d7
      filipe oliveira authored
      Avoid the sdslen() on shared.crlf given we know its size beforehand. Improve ~3-4% of cpu cycles to lrange logic (#10987)
      
      * Avoid the sdslen() on shared.crlf given we know its size beforehand
      * Removed shared.crlf from sharedObjects
      6686c6d7
  15. 26 Jul, 2022 1 commit
    • Binbin's avatar
      Change the return value of rdbLoad function to enums (#11039) · 00097bf4
      Binbin authored
      The reason we do this is because in #11036, we added error
      log message when failing to open RDB file for reading.
      In loadDdataFromDisk we call rdbLoad and also check errno,
      now the logging corrupts errno (reported in alpine daily).
      
      It is not safe to rely on errno as we do today, so we change
      the return value of rdbLoad function to enums, like we have
      when loading an AOF.
      00097bf4
  16. 18 Jul, 2022 1 commit
    • ranshid's avatar
      Avoid using unsafe C functions (#10932) · eacca729
      ranshid authored
      replace use of:
      sprintf --> snprintf
      strcpy/strncpy  --> redis_strlcpy
      strcat/strncat  --> redis_strlcat
      
      **why are we making this change?**
      Much of the code uses some unsafe variants or deprecated buffer handling
      functions.
      While most cases are probably not presenting any issue on the known path
      programming errors and unterminated strings might lead to potential
      buffer overflows which are not covered by tests.
      
      **As part of this PR we change**
      1. added implementation for redis_strlcpy and redis_strlcat based on the strl implementation: https://linux.die.net/man/3/strl
      2. change all occurrences of use of sprintf with use of snprintf
      3. change occurrences of use of  strcpy/strncpy with redis_strlcpy
      4. change occurrences of use of strcat/strncat with redis_strlcat
      5. change the behavior of ll2string/ull2string/ld2string so that it will always place null
        termination ('\0') on the output buffer in the first index. this was done in order to make
        the use of these functions more safe in cases were the user will not check the output
        returned by them (for example in rdbRemoveTempFile)
      6. we added a compiler directive to issue a deprecation error in case a use of
        sprintf/strcpy/strcat is found during compilation which will result in error during compile time.
        However keep in mind that since the deprecation attribute is not supported on all compilers,
        this is expected to fail during push workflows.
      
      
      **NOTE:** while this is only an initial milestone. We might also consider
      using the *_s implementation provided by the C11 Extensions (however not
      yet widly supported). I would also suggest to start
      looking at static code analyzers to track unsafe use cases.
      For example LLVM clang checker supports security.insecureAPI.DeprecatedOrUnsafeBufferHandling
      which can help locate unsafe function usage.
      https://clang.llvm.org/docs/analyzer/checkers.html#security-insecureapi-deprecatedorunsafebufferhandling-c
      The main reason not to onboard it at this stage is that the alternative
      excepted by clang is to use the C11 extensions which are not always
      supported by stdlib.
      eacca729
  17. 11 Jul, 2022 1 commit
  18. 07 Jul, 2022 1 commit
  19. 06 Jul, 2022 1 commit
  20. 04 Jul, 2022 1 commit
    • Qu Chen's avatar
      Unlock cluster config file upon server shutdown. (#10912) · 33b7ff38
      Qu Chen authored
      Currently in cluster mode, Redis process locks the cluster config file when
      starting up and holds the lock for the entire lifetime of the process.
      When the server shuts down, it doesn't explicitly release the lock on the
      cluster config file. We noticed a problem with restart testing that if you shut down
      a very large redis-server process (i.e. with several hundred GB of data stored),
      it takes the OS a while to free the resources and unlock the cluster config file.
      So if we immediately try to restart the redis server process, it might fail to acquire
      the lock on the cluster config file and fail to come up.
      
      This fix explicitly releases the lock on the cluster config file upon a shutdown rather
      than relying on the OS to release the lock, which is a cleaner and safer approach to
      free up resources acquired. 
      33b7ff38
  21. 30 Jun, 2022 1 commit
    • Binbin's avatar
      Add SENTINEL command flag to CLIENT/COMMANDS subcommands (#10904) · 35e836c2
      Binbin authored
      This was harmless because we marked the parent command
      with SENTINEL flag. So the populateCommandTable was ok.
      And we also don't show the flag (SENTINEL and ONLY-SENTNEL)
      in COMMAND INFO.
      
      In this PR, we also add the same CMD_SENTINEL and CMD_ONLY_SENTINEL
      flags check when populating the sub-commands.
      so that in the future it'll be possible to add some sub-commands to sentinel or sentinel-only but not others.
      35e836c2
  22. 26 Jun, 2022 1 commit
    • Binbin's avatar
      redis-server command line arguments allow passing config name and value in the same arg (#10866) · d443e312
      Binbin authored
      This commit has two topics.
      
      ## Passing config name and value in the same arg
      In #10660 (Redis 7.0.1), when we supported the config values that can start with `--` prefix (one of the two topics of that PR),
      we broke another pattern: `redis-server redis.config "name value"`, passing both config name
      and it's value in the same arg, see #10865
      
      This wasn't a intended change (i.e we didn't realize this pattern used to work).
      Although this is a wrong usage, we still like to fix it.
      
      Now we support something like:
      ```
      src/redis-server redis.conf "--maxmemory '700mb'" "--maxmemory-policy volatile-lru" --proc-title-template --my--title--template --loglevel verbose
      ```
      
      ## Changes around --save
      Also in this PR, we undo the breaking change we made in #10660 on purpose.
      1. `redis-server redis.conf --save --loglevel verbose` (missing `save` argument before anotehr argument).
          In 7.0.1, it was throwing an wrong arg error.
          Now it will work and reset the save, similar to how it used to be in 7.0.0 and 6.2.x.
      3. `redis-server redis.conf --loglevel verbose --save` (missing `save` argument as last argument).
          In 6.2, it did not reset the save, which was a bug (inconsistent with the previous bullet).
          Now we will make it work and reset the save as well (a bug fix).
      d443e312
  23. 23 Jun, 2022 1 commit
  24. 12 Jun, 2022 2 commits
    • XiongDa's avatar
      Fix 3 comments in server.c (#10844) · abb2ea7e
      XiongDa authored
      abb2ea7e
    • Binbin's avatar
      Fixed SET and BITFIELD commands being wrongly marked movablekeys (#10837) · 92fb4f4f
      Binbin authored
      
      
      The SET and BITFIELD command were added `get_keys_function` in #10148, causing
      them to be wrongly marked movablekeys in `populateCommandMovableKeys`.
      
      This was an unintended side effect introduced in #10148 (7.0 RC1)
      which could cause some clients an extra round trip for these commands in cluster mode.
      
      Since we define movablekeys as a way to determine if the legacy range [first, last, step]
      doesn't find all keys, then we need a completely different approach.
      
      The right approach should be to check if the legacy range covers all key-specs,
      and if none of the key-specs have the INCOMPLETE flag. 
      This way, we don't need to look at getkeys_proc of VARIABLE_FLAG at all.
      Probably with the exception of modules, who may still not be using key-specs.
      
      In this PR, we removed `populateCommandMovableKeys` and put its logic in
      `populateCommandLegacyRangeSpec`.
      In order to properly serve both old and new modules, we must probably keep relying
      CMD_MODULE_GETKEYS, but do that only for modules that don't declare key-specs. 
      For ones that do, we need to take the same approach we take with native redis commands.
      
      This approach was proposed by Oran. Fixes #10833
      Co-authored-by: default avatarOran Agra <oran@redislabs.com>
      92fb4f4f
  25. 11 Jun, 2022 1 commit
    • Binbin's avatar
      Fix crash when overcommit_memory is inaccessible (#10848) · 62ac1ab0
      Binbin authored
      When `/proc/sys/vm/overcommit_memory` is inaccessible, fp is NULL.
      `checkOvercommit` will return -1 without setting err_msg, and then
      the err_msg is used to print the log, crash the server.
      Set the err_msg variables to Null when declaring it, seems safer.
      
      And the overcommit_memory error log will print two "WARNING",
      like `WARNING WARNING overcommit_memory is set to 0!`, this PR
      also removes the second WARNING in `checkOvercommit`.
      
      Reported in #10846. Fixes #10846. Introduced in #10636 (7.0.1)
      62ac1ab0
  26. 06 Jun, 2022 1 commit
    • DarrenJiang13's avatar
      Split instantaneous_repl_total_kbps to instantaneous_input_repl_kbps and... · f5585834
      DarrenJiang13 authored
      Split instantaneous_repl_total_kbps to instantaneous_input_repl_kbps and instantaneous_output_repl_kbps. (#10810)
      
      A supplement to https://github.com/redis/redis/pull/10062
      Split `instantaneous_repl_total_kbps` to `instantaneous_input_repl_kbps` and `instantaneous_output_repl_kbps`. 
      ## Work:
      This PR:
      - delete 1 info field:
          - `instantaneous_repl_total_kbps`
      - add 2 info fields:
          - `instantaneous_input_repl_kbps / instantaneous_output_repl_kbps`
      ## Result:
      - master
      ```
      total_net_input_bytes:26633673
      total_net_output_bytes:21716596
      total_net_repl_input_bytes:0
      total_net_repl_output_bytes:18433052
      instantaneous_input_kbps:0.02
      instantaneous_output_kbps:0.00
      instantaneous_input_repl_kbps:0.00
      instantaneous_output_repl_kbps:0.00
      ```
      - slave
      ```
      total_net_input_bytes:18433212
      total_net_output_bytes:94790
      total_net_repl_input_bytes:18433052
      total_net_repl_output_bytes:0
      instantaneous_input_kbps:0.00
      instantaneous_output_kbps:0.05
      instantaneous_input_repl_kbps:0.00
      instantaneous_output_repl_kbps:0.00
      ```
      f5585834
  27. 02 Jun, 2022 1 commit
  28. 01 Jun, 2022 1 commit
    • Oran Agra's avatar
      Expose script flags to processCommand for better handling (#10744) · df558618
      Oran Agra authored
      The important part is that read-only scripts (not just EVAL_RO
      and FCALL_RO, but also ones with `no-writes` executed by normal EVAL or
      FCALL), will now be permitted to run during CLIENT PAUSE WRITE (unlike
      before where only the _RO commands would be processed).
      
      Other than that, some errors like OOM, READONLY, MASTERDOWN are now
      handled by processCommand, rather than the command itself affects the
      error string (and even error code in some cases), and command stats.
      
      Besides that, now the `may-replicate` commands, PFCOUNT and PUBLISH, will
      be considered `write` commands in scripts and will be blocked in all
      read-only scripts just like other write commands.
      They'll also be blocked in EVAL_RO (i.e. even for scripts without the
      `no-writes` shebang flag.
      
      This commit also hides the `may_replicate` flag from the COMMAND command
      output. this is a **breaking change**.
      
      background about may_replicate:
      We don't want to expose a no-may-replicate flag or alike to scripts, since we
      consider the may-replicate thing an internal concern of redis, that we may
      some day get rid of.
      In fact, the may-replicate flag was initially introduced to flag EVAL: since
      we didn't know what it's gonna do ahead of execution, before function-flags
      existed). PUBLISH and PFCOUNT, both of which because they have side effects
      which may some day be fixed differently.
      
      code changes:
      The changes in eval.c are mostly code re-ordering:
      - evalCalcFunctionName is extracted out of evalGenericCommand
      - evalExtractShebangFlags is extracted luaCreateFunction
      - evalGetCommandFlags is new code
      df558618