1. 19 Apr, 2023 2 commits
    • Oran Agra's avatar
      Move startup system check to before daeomniztion and modules init (#12067) · e5882da9
      Oran Agra authored
      1. it's a bad idea to print these errors and exit after daemonization (if we'll exit,
        the failure may not be detected)
      2. it's not nice to exit (or even do the check that uses `fork`) after modules already
        started (could create threads, or do some changes)
      e5882da9
    • Wen Hui's avatar
      Updating reply_schema for sentinal commands (#12018) · b220e6b4
      Wen Hui authored
      Some sentinel subcommands are missing the reply_schema in the json file,
      so add the proper reply_schema part in json file as sentinel replicas commands.
      
      The schema validator was skipping coverage test for sentinel commands, this was initially
      done just in order to focus on redis commands and leave sentinel coverage for later,
      so this check is now removed.
      
      sentinel commands that were missing reply schema:
      * sentinel masters
      * sentinel myid
      * sentinel sentinels <master-name>
      * sentinel slaves (deprecated)  <master-name> 
      b220e6b4
  2. 18 Apr, 2023 2 commits
    • Pengfei Han's avatar
      Update dict.c dict_can_resize comment (#12059) · 528b0e69
      Pengfei Han authored
      The comment for dict_can_resize in dict.c should be updated. Currently 0 means
      DICT_RESIZE_ENABLE, but should actually be DICT_RESIZE_AVOID or 1.
      528b0e69
    • sundb's avatar
      Fix some compile warnings and errors when building with gcc-12 or clang (#12035) · 42c8c618
      sundb authored
      This PR is to fix the compilation warnings and errors generated by the latest
      complier toolchain, and to add a new runner of the latest toolchain for daily CI.
      
      ## Fix various compilation warnings and errors
      
      1) jemalloc.c
      
      COMPILER: clang-14 with FORTIFY_SOURCE
      
      WARNING:
      ```
      src/jemalloc.c:1028:7: warning: suspicious concatenation of string literals in an array initialization; did you mean to separate the elements with a comma? [-Wstring-concatenation]
                          "/etc/malloc.conf",
                          ^
      src/jemalloc.c:1027:3: note: place parentheses around the string literal to silence warning
                      "\"name\" of the file referenced by the symbolic link named "
                      ^
      ```
      
      REASON:  the compiler to alert developers to potential issues with string concatenation
      that may miss a comma,
      just like #9534 which misses a comma.
      
      SOLUTION: use `()` to tell the compiler that these two line strings are continuous.
      
      2) config.h
      
      COMPILER: clang-14 with FORTIFY_SOURCE
      
      WARNING:
      ```
      In file included from quicklist.c:36:
      ./config.h:319:76: warning: attribute declaration must precede definition [-Wignored-attributes]
      char *strcat(char *restrict dest, const char *restrict src) __attribute__((deprecated("please avoid use of unsafe C functions. prefer use of redis_strlcat instead")));
      ```
      
      REASON: Enabling _FORTIFY_SOURCE will cause the compiler to use `strcpy()` with check,
      it results in a deprecated attribute declaration after including <features.h>.
      
      SOLUTION: move the deprecated attribute declaration from config.h to fmacro.h before "#include <features.h>".
      
      3) networking.c
      
      COMPILER: GCC-12
      
      WARNING: 
      ```
      networking.c: In function ‘addReplyDouble.part.0’:
      networking.c:876:21: warning: writing 1 byte into a region of size 0 [-Wstringop-overflow=]
        876 |         dbuf[start] = '$';
            |                     ^
      networking.c:868:14: note: at offset -5 into destination object ‘dbuf’ of size 5152
        868 |         char dbuf[MAX_LONG_DOUBLE_CHARS+32];
            |              ^
      networking.c:876:21: warning: writing 1 byte into a region of size 0 [-Wstringop-overflow=]
        876 |         dbuf[start] = '$';
            |                     ^
      networking.c:868:14: note: at offset -6 into destination object ‘dbuf’ of size 5152
        868 |         char dbuf[MAX_LONG_DOUBLE_CHARS+32];
      ```
      
      REASON: GCC-12 predicts that digits10() may return 9 or 10 through `return 9 + (v >= 1000000000UL)`.
      
      SOLUTION: add an assert to let the compiler know the possible length;
      
      4) redis-cli.c & redis-benchmark.c
      
      COMPILER: clang-14 with FORTIFY_SOURCE
      
      WARNING:
      ```
      redis-benchmark.c:1621:2: warning: embedding a directive within macro arguments has undefined behavior [-Wembedded-directive] #ifdef USE_OPENSSL
      redis-cli.c:3015:2: warning: embedding a directive within macro arguments has undefined behavior [-Wembedded-directive] #ifdef USE_OPENSSL
      ```
      
      REASON: when _FORTIFY_SOURCE is enabled, the compiler will use the print() with
      check, which is a macro. this may result in the use of directives within the macro, which
      is undefined behavior.
      
      SOLUTION: move the directives-related code out of `print()`.
      
      5) server.c
      
      COMPILER: gcc-13 with FORTIFY_SOURCE
      
      WARNING:
      ```
      In function 'lookupCommandLogic',
          inlined from 'lookupCommandBySdsLogic' at server.c:3139:32:
      server.c:3102:66: error: '*(robj **)argv' may be used uninitialized [-Werror=maybe-uninitialized]
       3102 |     struct redisCommand *base_cmd = dictFetchValue(commands, argv[0]->ptr);
            |                                                              ~~~~^~~
      ```
      
      REASON: The compiler thinks that the `argc` returned by `sdssplitlen()` could be 0,
      resulting in an empty array of size 0 being passed to lookupCommandLogic.
      this should be a false positive, `argc` can't be 0 when strings are not NULL.
      
      SOLUTION: add an assert to let the compiler know that `argc` is positive.
      
      6) sha1.c
      
      COMPILER: gcc-12
      
      WARNING:
      ```
      In function ‘SHA1Update’,
          inlined from ‘SHA1Final’ at sha1.c:195:5:
      sha1.c:152:13: warning: ‘SHA1Transform’ reading 64 bytes from a region of size 0 [-Wstringop-overread]
        152 |             SHA1Transform(context->state, &data[i]);
            |             ^
      sha1.c:152:13: note: referencing argument 2 of type ‘const unsigned char[64]’
      sha1.c: In function ‘SHA1Final’:
      sha1.c:56:6: note: in a call to function ‘SHA1Transform’
         56 | void SHA1Transform(uint32_t state[5], const unsigned char buffer[64])
            |      ^
      In function ‘SHA1Update’,
          inlined from ‘SHA1Final’ at sha1.c:198:9:
      sha1.c:152:13: warning: ‘SHA1Transform’ reading 64 bytes from a region of size 0 [-Wstringop-overread]
        152 |             SHA1Transform(context->state, &data[i]);
            |             ^
      sha1.c:152:13: note: referencing argument 2 of type ‘const unsigned char[64]’
      sha1.c: In function ‘SHA1Final’:
      sha1.c:56:6: note: in a call to function ‘SHA1Transform’
         56 | void SHA1Transform(uint32_t state[5], const unsigned char buffer[64])
      ```
      
      REASON: due to the bug[https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80922], when
      enable LTO, gcc-12 will not see `diagnostic ignored "-Wstringop-overread"`, resulting in a warning.
      
      SOLUTION: temporarily set SHA1Update to noinline to avoid compiler warnings due
      to LTO being enabled until the above gcc bug is fixed.
      
      7) zmalloc.h
      
      COMPILER: GCC-12
      
      WARNING: 
      ```
      In function ‘memset’,
          inlined from ‘moduleCreateContext’ at module.c:877:5,
          inlined from ‘RM_GetDetachedThreadSafeContext’ at module.c:8410:5:
      /usr/include/x86_64-linux-gnu/bits/string_fortified.h:59:10: warning: ‘__builtin_memset’ writing 104 bytes into a region of size 0 overflows the destination [-Wstringop-overflow=]
         59 |   return __builtin___memset_chk (__dest, __ch, __len,
      ```
      
      REASON: due to the GCC-12 bug [https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96503],
      GCC-12 cannot see alloc_size, which causes GCC to think that the actual size of memory
      is 0 when checking with __glibc_objsize0().
      
      SOLUTION: temporarily set malloc-related interfaces to `noinline` to avoid compiler warnings
      due to LTO being enabled until the above gcc bug is fixed.
      
      ## Other changes
      1) Fixed `ps -p [pid]`  doesn't output `<defunct>` when using procps 4.x causing `replication
        child dies when parent is killed - diskless` test to fail.
      2) Add a new fortify CI with GCC-13 and ubuntu-lunar docker image.
      42c8c618
  3. 17 Apr, 2023 1 commit
    • Joe Hu's avatar
      Fix RDB check regression caused by PR 12022 (#12051) · 644d9455
      Joe Hu authored
      The nightly tests showed that the recent PR #12022 caused random failures
      in aof.tcl on checking RDB preamble inside an AOF file.
      
      Root cause:
      When checking RDB preamble in an AOF file, what's passed into redis_check_rdb is
      aof_filename, not aof_filepath. The newly introduced isFifo function does not check return
      status of the stat call and hence uses the uninitailized stat_p object.
      
      Fix:
      1. Fix isFifo by checking stat call's return code.
      2. Pass aof_filepath instead of aof_filename to redis_check_rdb.
      3. move the FIFO check to rdb.c since the limitation is the re-opening of the file, and not
        anything specific about redis-check-rdb.
      644d9455
  4. 16 Apr, 2023 2 commits
    • judeng's avatar
      avoid incorrect shrinking of querybuf when client is reading a big argv (#12000) · e7f18432
      judeng authored
      this pr fix two wrongs:
      1. When client’s querybuf is pre-allocated for a fat argv, we need to update the
        querybuf_peak of the client immediately to completely avoid the unexpected
        shrinking of querybuf in the next clientCron (before data arrives to set the peak).
      2. the protocol's bulklen does not include `\r\n`, but the allocation and the data we
        read does. so in `clientsCronResizeQueryBuffer`, the `resize` or `querybuf_peak`
        should add these 2 bytes.
      
      the first bug is likely to hit us on large payloads over slow connections, in which case
      transferring the payload can take longer and a cron event will be triggered (specifically
      if there are not a lot of clients)
      e7f18432
    • judeng's avatar
      improve performance of keys command by using static objects (#12036) · 1222b608
      judeng authored
      Improve performance by avoiding redundancy memory malloc/free
      1222b608
  5. 14 Apr, 2023 1 commit
    • Joe Hu's avatar
      Fix redis_check_rdb() hang when rdb is FIFO (#12022) · d5d56d0d
      Joe Hu authored
      When loading RDB over the named piped, redis_check_rdb() is hung at fopen, because fopen blocks until another process opens the FIFO for writing. The fix is to check if RDB is FIFO. If yes, return an error.
      d5d56d0d
  6. 13 Apr, 2023 2 commits
  7. 12 Apr, 2023 1 commit
    • Binbin's avatar
      Add RM_ReplyWithErrorFormat that can support format (#11923) · bfec2d70
      Binbin authored
      * Add RM_ReplyWithErrorFormat that can support format
      
      Reply with the error create from a printf format and arguments.
      
      If the error code is already passed in the string 'fmt', the error
      code provided is used, otherwise the string "-ERR " for the generic
      error code is automatically added.
      
      The usage is, for example:
          RedisModule_ReplyWithErrorFormat(ctx, "An error: %s", "foo");
          RedisModule_ReplyWithErrorFormat(ctx, "-WRONGTYPE Wrong Type: %s", "foo");
      
      The function always returns REDISMODULE_OK.
      bfec2d70
  8. 10 Apr, 2023 1 commit
    • sundb's avatar
      Use dummy allocator to make accesses defined as per standard (#11982) · e0b378d2
      sundb authored
      
      
      ## Issue
      When we use GCC-12 later or clang 9.0 later to build with `-D_FORTIFY_SOURCE=3`,
      we can see the following buffer overflow:
      ```
      === REDIS BUG REPORT START: Cut & paste starting from here ===
      6263:M 06 Apr 2023 08:59:12.915 # Redis 255.255.255 crashed by signal: 6, si_code: -6
      6263:M 06 Apr 2023 08:59:12.915 # Crashed running the instruction at: 0x7f03d59efa7c
      
      ------ STACK TRACE ------
      EIP:
      /lib/x86_64-linux-gnu/libc.so.6(pthread_kill+0x12c)[0x7f03d59efa7c]
      
      Backtrace:
      /lib/x86_64-linux-gnu/libc.so.6(+0x42520)[0x7f03d599b520]
      /lib/x86_64-linux-gnu/libc.so.6(pthread_kill+0x12c)[0x7f03d59efa7c]
      /lib/x86_64-linux-gnu/libc.so.6(raise+0x16)[0x7f03d599b476]
      /lib/x86_64-linux-gnu/libc.so.6(abort+0xd3)[0x7f03d59817f3]
      /lib/x86_64-linux-gnu/libc.so.6(+0x896f6)[0x7f03d59e26f6]
      /lib/x86_64-linux-gnu/libc.so.6(__fortify_fail+0x2a)[0x7f03d5a8f76a]
      /lib/x86_64-linux-gnu/libc.so.6(+0x1350c6)[0x7f03d5a8e0c6]
      src/redis-server 127.0.0.1:25111(+0xd5e80)[0x557cddd3be80]
      src/redis-server 127.0.0.1:25111(feedReplicationBufferWithObject+0x78)[0x557cddd3c768]
      src/redis-server 127.0.0.1:25111(replicationFeedSlaves+0x1a4)[0x557cddd3cbc4]
      src/redis-server 127.0.0.1:25111(+0x8721a)[0x557cddced21a]
      src/redis-server 127.0.0.1:25111(call+0x47a)[0x557cddcf38ea]
      src/redis-server 127.0.0.1:25111(processCommand+0xbf4)[0x557cddcf4aa4]
      src/redis-server 127.0.0.1:25111(processInputBuffer+0xe6)[0x557cddd22216]
      src/redis-server 127.0.0.1:25111(readQueryFromClient+0x3a8)[0x557cddd22898]
      src/redis-server 127.0.0.1:25111(+0x1b9134)[0x557cdde1f134]
      src/redis-server 127.0.0.1:25111(aeMain+0x119)[0x557cddce5349]
      src/redis-server 127.0.0.1:25111(main+0x466)[0x557cddcd6716]
      /lib/x86_64-linux-gnu/libc.so.6(+0x29d90)[0x7f03d5982d90]
      /lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0x80)[0x7f03d5982e40]
      src/redis-server 127.0.0.1:25111(_start+0x25)[0x557cddcd7025]
      ```
      
      The main reason is that when FORTIFY_SOURCE is enabled, GCC or clang will enhance some
      common functions, such as `strcpy`, `memcpy`, `fgets`, etc, so that they can detect buffer
      overflow errors and stop program execution, thus improving the safety of the program.
      We use `zmalloc_usable_size()` everywhere to use memory blocks, but that is an abuse since the
      malloc_usable_size() isn't meant for this kind of use, it is for diagnostics only. That is also why the
      behavior is flaky when built with _FORTIFY_SOURCE, the compiler can sense that we reach outside
      the allocated block and SIGABRT.
      
      ### Solution
      If we need to use the additional memory we got, we need to use a dummy realloc with `alloc_size` attribute
      and no inlining, (see `extend_to_usable`) to let the compiler see the large of memory we need to use.
      This can either be an implicit call inside `z*usable` that returns the size, so that the caller doesn't have any
      other worry, or it can be a normal zmalloc call which means that if the caller wants to use
      zmalloc_usable_size it must also use extend_to_usable.
      
      ### Changes
      
      This PR does the following:
      1) rename the current z[try]malloc_usable family to z[try]malloc_internal and don't expose them to users outside zmalloc.c,
      2) expose a new set of `z[*]_usable` family that use z[*]_internal and `extend_to_usable()` implicitly, the caller gets the
        size of the allocation and it is safe to use.
      3) go over all the users of `zmalloc_usable_size` and convert them to use the `z[*]_usable` family if possible.
      4) in the places where the caller can't use `z[*]_usable` and store the real size, and must still rely on zmalloc_usable_size,
        we still make sure that the allocation used `z[*]_usable` (which has a call to `extend_to_usable()`) and ignores the
        returning size, this way a later call to `zmalloc_usable_size` is still safe.
      
      [4] was done for module.c and listpack.c, all the others places (sds, reply proto list, replication backlog, client->buf)
      are using [3].
      Co-authored-by: default avatarOran Agra <oran@redislabs.com>
      e0b378d2
  9. 09 Apr, 2023 1 commit
    • Ozan Tezcan's avatar
      Add RM_RdbLoad and RM_RdbSave module API functions (#11852) · e55568ed
      Ozan Tezcan authored
      Add `RM_RdbLoad()` and `RM_RdbSave()` to load/save RDB files from the module API. 
      
      In our use case, we have our clustering implementation as a module. As part of this
      implementation, the module needs to trigger RDB save operation at specific points.
      Also, this module delivers RDB files to other nodes (not using Redis' replication).
      When a node receives an RDB file, it should be able to load the RDB. Currently,
      there is no module API to save/load RDB files. 
      
      
      This PR adds four new APIs:
      ```c
      RedisModuleRdbStream *RM_RdbStreamCreateFromFile(const char *filename);
      void RM_RdbStreamFree(RedisModuleRdbStream *stream);
      
      int RM_RdbLoad(RedisModuleCtx *ctx, RedisModuleRdbStream *stream, int flags);
      int RM_RdbSave(RedisModuleCtx *ctx, RedisModuleRdbStream *stream, int flags);
      ```
      
      The first step is to create a `RedisModuleRdbStream` object. This PR provides a function to
      create RedisModuleRdbStream from the filename. (You can load/save RDB with the filename).
      In the future, this API can be extended if needed: 
      e.g., `RM_RdbStreamCreateFromFd()`, `RM_RdbStreamCreateFromSocket()` to save/load
      RDB from an `fd` or a `socket`. 
      
      
      Usage:
      ```c
      /* Save RDB */
      RedisModuleRdbStream *stream = RedisModule_RdbStreamCreateFromFile("example.rdb");
      RedisModule_RdbSave(ctx, stream, 0);
      RedisModule_RdbStreamFree(stream);
      
      /* Load RDB */
      RedisModuleRdbStream *stream = RedisModule_RdbStreamCreateFromFile("example.rdb");
      RedisModule_RdbLoad(ctx, stream, 0);
      RedisModule_RdbStreamFree(stream);
      ```
      e55568ed
  10. 04 Apr, 2023 4 commits
    • bodong.ybd's avatar
      Add help message for client setinfo (#11995) · ccc86a91
      bodong.ybd authored
      
      
      The new sub-command was missing from CLIENT HELP
      Co-authored-by: default avatarBinbin <binloveplay1314@qq.com>
      ccc86a91
    • Subhi Al Hasan's avatar
      check for known-slave in sentinel rewrite config (#11775) · 74b29985
      Subhi Al Hasan authored
      Fix the following config file error
      
      ```
      *** FATAL CONFIG FILE ERROR (Redis 6.2.7) ***
      Reading the configuration file, at line 152
      >>> 'sentinel known-replica XXXX 127.0.0.1 5001'
      Duplicate hostname and port for replica.
      ```
      
      
      that is happening when a user uses the legacy key "known-slave" in
      the config file and a config rewrite occurs. The config rewrite logic won't
      replace the old  line "sentinel known-slave XXXX 127.0.0.1 5001" and
      would add a new line with "sentinel known-replica XXXX 127.0.0.1 5001"
      which results in the error above "Duplicate hostname and port for replica."
      
      example:
      
      Current sentinal.conf
      ```
      ...
      
      sentinel known-slave XXXX 127.0.0.1 5001
      sentinel example-random-option X
      ...
      ```
      after the config rewrite logic runs:
      ```
      ....
      sentinel known-slave XXXX 127.0.0.1 5001
      sentinel example-random-option X
      
      # Generated by CONFIG REWRITE
      sentinel known-replica XXXX 127.0.0.1 5001
      ```
      
      This bug only exists in Redis versions >=6.2 because prior to that it was hidden
      by the effects of this bug https://github.com/redis/redis/issues/5388 that was fixed
      in https://github.com/redis/redis/pull/8271 and was released in versions >=6.2
      74b29985
    • gx's avatar
      Fix local clients detection (#11664) · e1da7241
      gx authored
      Match 127.0.0.0/8 instead of just `127.0.0.1` to detect the local clients.
      e1da7241
    • Binbin's avatar
      Changed activeExpireCycle server.masterhost check to iAmMaster in beforeSleep (#11997) · aee8d1ff
      Binbin authored
      In cluster mode, when a node restart as a replica, it doesn't immediately
      sync with the master, replication is enabled in clusterCron. It means that
      sometime server.masterhost is NULL and we wrongly judge it in beforeSleep.
      
      In this case, we may trigger a fast activeExpireCycle in beforeSleep, but the
      node's flag is actually a replica, that can lead to data inconsistency.  In this
      PR, we use iAmMaster to replace the `server.masterhost == NULL`
      
      This is an overlook in #7001, and more discussion in #11783.
      aee8d1ff
  11. 02 Apr, 2023 2 commits
    • Wen Hui's avatar
      redis-cli - handle sensitive command redaction for variadic CONFIG SET (#11975) · a4a0eab5
      Wen Hui authored
      In the Redis 7.0 and newer version,
      config set command support multiply `<parameter> <value>` pairs, thus the previous
      sensitive command condition does not apply anymore
      
      For example:
      
      The command:
      **config set maxmemory 1GB masteruser aa** will be written to redis_cli historyfile
      
      In this PR, we update the condition for these sensitive commands
      config set masteruser <username>
      config set masterauth <master-password>
      config set requirepass foobared
      a4a0eab5
    • Slava Koyfman's avatar
      Disconnect pub-sub subscribers when revoking `allchannels` permission (#11992) · f38aa6bf
      Slava Koyfman authored
      The existing logic for killing pub-sub clients did not handle the `allchannels`
      permission correctly. For example, if you:
      
          ACL SETUSER foo allchannels
      
      Have a client authenticate as the user `foo` and subscribe to a channel, and then:
      
          ACL SETUSER foo resetchannels
      
      The subscribed client would not be disconnected, though new clients under that user
      would be blocked from subscribing to any channels.
      
      This was caused by an incomplete optimization in `ACLKillPubsubClientsIfNeeded`
      checking whether the new channel permissions were a strict superset of the old ones.
      f38aa6bf
  12. 30 Mar, 2023 2 commits
    • Jason Elbaum's avatar
      Reimplement cli hints based on command arg docs (#10515) · 1f76bb17
      Jason Elbaum authored
      
      
      Now that the command argument specs are available at runtime (#9656), this PR addresses
      #8084 by implementing a complete solution for command-line hinting in `redis-cli`.
      
      It correctly handles nearly every case in Redis's complex command argument definitions, including
      `BLOCK` and `ONEOF` arguments, reordering of optional arguments, and repeated arguments
      (even when followed by mandatory arguments). It also validates numerically-typed arguments.
      It may not correctly handle all possible combinations of those, but overall it is quite robust.
      
      Arguments are only matched after the space bar is typed, so partial word matching is not
      supported - that proved to be more confusing than helpful. When the user's current input
      cannot be matched against the argument specs, hinting is disabled.
      
      Partial support has been implemented for legacy (pre-7.0) servers that do not support
      `COMMAND DOCS`, by falling back to a statically-compiled command argument table.
      On startup, if the server does not support `COMMAND DOCS`, `redis-cli` will now issue
      an `INFO SERVER` command to retrieve the server version (unless `HELLO` has already
      been sent, in which case the server version will be extracted from the reply to `HELLO`).
      The server version will be used to filter the commands and arguments in the command table,
      removing those not supported by that version of the server. However, the static table only
      includes core Redis commands, so with a legacy server hinting will not be supported for
      module commands. The auto generated help.h and the scripts that generates it are gone.
      
      Command and argument tables for the server and CLI use different structs, due primarily
      to the need to support different runtime data. In order to generate code for both, macros
      have been added to `commands.def` (previously `commands.c`) to make it possible to
      configure the code generation differently for different use cases (one linked with redis-server,
      and one with redis-cli).
      
      Also adding a basic testing framework for the command hints based on new (undocumented)
      command line options to `redis-cli`: `--test_hint 'INPUT'` prints out the command-line hint for
      a given input string, and `--test_hint_file <filename>` runs a suite of test cases for the hinting
      mechanism. The test suite is in `tests/assets/test_cli_hint_suite.txt`, and it is run from
      `tests/integration/redis-cli.tcl`.
      Co-authored-by: default avatarOran Agra <oran@redislabs.com>
      Co-authored-by: default avatarViktor Söderqvist <viktor.soderqvist@est.tech>
      1f76bb17
    • Madelyn Olson's avatar
      Fixed tracking of command duration for multi/eval/module/wait (#11970) · 971b177f
      Madelyn Olson authored
      In #11012, we changed the way command durations were computed to handle the same command being executed multiple times. This commit fixes some misses from that commit.
      
      * Wait commands were not correctly reporting their duration if the timeout was reached.
      * Multi/scripts/and modules with RM_Call were not properly resetting the duration between inner calls, leading to them reporting cumulative duration.
      * When a blocked client is freed, the call and duration are always discarded.
      
      This commit also adds an assert if the duration is not properly reset, potentially indicating that a report to call statistics was missed. The assert potentially be removed in the future, as it's mainly intended to detect misses in tests.
      971b177f
  13. 29 Mar, 2023 2 commits
    • Itamar Haber's avatar
      Overhauls command summaries and man pages. (#11942) · 0c3b8b7e
      Itamar Haber authored
      This is an attempt to normalize/formalize command summaries.
      
      Main actions performed:
      
      * Starts with the continuation of the phrase "The XXXX command, when called, ..." for user commands.
      * Starts with "An internal command...", "A container command...", etc... when applicable.
      * Always uses periods.
      * Refrains from referring to other commands. If this is needed, backquotes should be used for command names.
      * Tries to be very clear about the data type when applicable.
      * Tries to mention additional effects, e.g. "The key is created if it doesn't exist" and "The set is deleted if the last member is removed."
      * Prefers being terse over verbose.
      * Tries to be consistent.
      0c3b8b7e
    • Binbin's avatar
      Fix fork done handler wrongly update fsync metrics and enhance AOF_ FSYNC_ALWAYS (#11973) · cb171786
      Binbin authored
      This PR fix several unrelated bugs that were discovered by the same set of tests
      (WAITAOF tests in #11713), could make the `WAITAOF` test hang. 
      
      The change in `backgroundRewriteDoneHandler` is about MP-AOF.
      That leftover / old code assumes that we started a new AOF file just now
      (when we have a new base into which we're gonna incrementally write), but
      the fact is that with MP-AOF, the fork done handler doesn't really affect the
      incremental file being maintained by the parent process, there's no reason to
      re-issue `SELECT`, and no reason to update any of the fsync variables in that flow.
      This should have been deleted with MP-AOF (introduced in #9788, 7.0).
      The damage is that the update to `aof_fsync_offset` will cause us to miss an fsync
      in `flushAppendOnlyFile`, that happens if we stop write commands in `AOF_FSYNC_EVERYSEC`
      while an AOFRW is in progress. This caused a new `WAITAOF` test to sometime hang forever.
      
      Also because of MP-AOF, we needed to change `aof_fsync_offset` to `aof_last_incr_fsync_offset`
      and match it to `aof_last_incr_size` in `flushAppendOnlyFile`. This is because in the past we compared
      `aof_fsync_offset` and `aof_current_size`, but with MP-AOF it could be the total AOF file will be
      smaller after AOFRW, and the (already existing) incr file still has data that needs to be fsynced.
      
      The change in `flushAppendOnlyFile`, about the `AOF_FSYNC_ALWAYS`, it is follow #6053
      (the details is in #5985), we also check `AOF_FSYNC_ALWAYS` to handle a case where
      appendfsync is changed from everysec to always while there is data that's written but not yet fsynced.
      cb171786
  14. 27 Mar, 2023 1 commit
    • Rafi Einstein's avatar
      Clang: fix for -flto argument (#11961) · 557ca05d
      Rafi Einstein authored
      Starting with the recent #11926 Makefile specifies `-flto=auto` which is unsupported on clang.
      Additionally, detecting clang correctly requires actually running it, since on MacOS gcc can be an alias for clang.
      557ca05d
  15. 22 Mar, 2023 5 commits
  16. 21 Mar, 2023 2 commits
    • Roshan Khatri's avatar
      Module commands to have ACL categories. (#11708) · 6948daca
      Roshan Khatri authored
      
      
      This allows modules to register commands to existing ACL categories and blocks the creation of [sub]commands, datatypes and registering the configs outside of the OnLoad function.
      
      For allowing modules to register commands to existing ACL categories,
      This PR implements a new API int RM_SetCommandACLCategories() which takes a pointer to a RedisModuleCommand and a C string aclflags containing the set of space separated ACL categories.
      Example, 'write slow' marks the command as part of the write and slow ACL categories.
      
      The C string aclflags is tokenized by implementing a helper function categoryFlagsFromString(). Theses tokens are matched and the corresponding ACL categories flags are set by a helper function matchAclCategoriesFlags. The helper function categoryFlagsFromString() returns the corresponding categories_flags or returns -1 if some token not processed correctly.
      
      If the module contains commands which are registered to existing ACL categories, the number of [sub]commands are tracked by num_commands_with_acl_categories in struct RedisModule. Further, the allowed command bit-map of the existing users are recomputed from the command_rules list, by implementing a function called ACLRecomputeCommandBitsFromCommandRulesAllUsers() for the existing users to have access to the module commands on runtime.
      
      ## Breaking change
      This change requires that registering commands and subcommands only occur during a modules "OnLoad" function, in order to allow efficient recompilation of ACL bits. We also chose to block registering configs and types, since we believe it's only valid for those to be created during onLoad. We check for this onload flag in struct RedisModule to check if the call is made from the OnLoad function.
      Co-authored-by: default avatarMadelyn Olson <madelyneolson@gmail.com>
      6948daca
    • Binbin's avatar
      Add missing master_reboot flag in sentinel instance info (#11888) · 1cb4b1ad
      Binbin authored
      SRI_MASTER_REBOOT flag was added in #9438
      1cb4b1ad
  17. 20 Mar, 2023 3 commits
  18. 19 Mar, 2023 2 commits
    • Viktor Söderqvist's avatar
      redis-cli: Accept commands in subscribed mode (#11873) · bbf364a4
      Viktor Söderqvist authored
      The message "Reading messages... (press Ctrl-C to quit)" is replaced by
      "Reading messages... (press Ctrl-C to quit or any key to type command)".
      
      This allows users to subscribe to more channels, to try out UNSUBSCRIBE and to
      combine pubsub with other features such as push messages from client tracking.
      
      The "Reading messages" info message is displayed in the bottom of the output in a
      distinct style and moves downward as more messages appear. When any key is pressed,
      the info message is replaced by the prompt with for entering commands.
      After entering a command and the reply is displayed, the "Reading messages" info
      messages appears again. This is added to the repl loop in redis-cli and in the
      corresponding place for non-interactive mode.
      
      An indication "(subscribed mode)" is included in the prompt when entering commands
      in subscribed mode.
      
      Also:
      * Fixes a problem that UNSUBSCRIBE hanged when used with RESP3 and push callback,
        without first entering subscribe mode. It hanged because UNSUBSCRIBE gets one or
        more push replies but no in-band reply.
      * Exit subscribed mode after RESET.
      bbf364a4
    • Wang Yuan's avatar
      Remove unnecessary `fsync` when sentinel flushs config file (#11910) · c9466b24
      Wang Yuan authored
      `rewriteConfig` already calls `fsync` to make sure changes are committed to disk.
      so it is no need to call `fsync` again here.
      this was added here when rewriteConfigOverwriteFile used the ftruncate approach and didn't fsync
      c9466b24
  19. 17 Mar, 2023 1 commit
    • Rong Tao's avatar
      Fix compile lto-wrapper warning for aarch64 (#11926) · d6910983
      Rong Tao authored
      
      
      Use -flto=auto to use GNU make's job server, if available, or otherwise fall
      back to autodetection of the number of CPU threads present in your system.
      
        Warnings:
      
        lto-wrapper: warning: using serial compilation of 2 LTRANS jobs
        lto-wrapper: note: see the ‘-flto’ option documentation for more information
        lto-wrapper: warning: using serial compilation of 4 LTRANS jobs
        lto-wrapper: note: see the ‘-flto’ option documentation for more information
        lto-wrapper: warning: using serial compilation of 31 LTRANS jobs
        lto-wrapper: note: see the ‘-flto’ option documentation for more information
      Signed-off-by: default avatarRong Tao <rongtao@cestc.cn>
      d6910983
  20. 16 Mar, 2023 2 commits
    • Meir Shpilraien (Spielrein)'s avatar
      Support for RM_Call on blocking commands (#11568) · d0da0a6a
      Meir Shpilraien (Spielrein) authored
      Allow running blocking commands from within a module using `RM_Call`.
      
      Today, when `RM_Call` is used, the fake client that is used to run command
      is marked with `CLIENT_DENY_BLOCKING` flag. This flag tells the command
      that it is not allowed to block the client and in case it needs to block, it must
      fallback to some alternative (either return error or perform some default behavior).
      For example, `BLPOP` fallback to simple `LPOP` if it is not allowed to block.
      
      All the commands must respect the `CLIENT_DENY_BLOCKING` flag (including
      module commands). When the command invocation finished, Redis asserts that
      the client was not blocked.
      
      This PR introduces the ability to call blocking command using `RM_Call` by
      passing a callback that will be called when the client will get unblocked.
      In order to do that, the user must explicitly say that he allow to perform blocking
      command by passing a new format specifier argument, `K`, to the `RM_Call`
      function. This new flag will tell Redis that it is allow to run blocking command
      and block the client. In case the command got blocked, Redis will return a new
      type of call reply (`REDISMODULE_REPLY_PROMISE`). This call reply indicates
      that the command got blocked and the user can set the on_unblocked handler using
      `RM_CallReplyPromiseSetUnblockHandler`.
      
      When clients gets unblocked, it eventually reaches `processUnblockedClients` function.
      This is where we check if the client is a fake module client and if it is, we call the unblock
      callback instead of performing the usual unblock operations.
      
      **Notice**: `RM_CallReplyPromiseSetUnblockHandler` must be called atomically
      along side the command invocation (without releasing the Redis lock in between).
      In addition, unlike other CallReply types, the promise call reply must be released
      by the module when the Redis GIL is acquired.
      
      The module can abort the execution on the blocking command (if it was not yet
      executed) using `RM_CallReplyPromiseAbort`. the API will return `REDISMODULE_OK`
      on success and `REDISMODULE_ERR` if the operation is already executed.
      **Notice** that in case of misbehave module, Abort might finished successfully but the
      operation will not really be aborted. This can only happened if the module do not respect
      the disconnect callback of the blocked client. 
      For pure Redis commands this can not happened.
      
      ### Atomicity Guarantees
      
      The API promise that the unblock handler will run atomically as an execution unit.
      This means that all the operation performed on the unblock handler will be wrapped
      with a multi exec transaction when replicated to the replica and AOF.
      The API **do not** grantee any other atomicity properties such as when the unblock
      handler will be called. This gives us the flexibility to strengthen the grantees (or not)
      in the future if we will decide that we need a better guarantees.
      
      That said, the implementation **does** provide a better guarantees when performing
      pure Redis blocking command like `BLPOP`. In this case the unblock handler will run
      atomically with the operation that got unblocked (for example, in case of `BLPOP`, the
      unblock handler will run atomically with the `LPOP` operation that run when the command
      got unblocked). This is an implementation detail that might be change in the future and the
      module writer should not count on that.
      
      ### Calling blocking commands while running on script mode (`S`)
      
      `RM_Call` script mode (`S`) was introduced on #0372. It is used for usecases where the
      command that was invoked on `RM_Call` comes from a user input and we want to make
      sure the user will not run dangerous commands like `shutdown`. Some command, such
      as `BLPOP`, are marked with `NO_SCRIPT` flag, which means they will not be allowed on
      script mode. Those commands are marked with  `NO_SCRIPT` just because they are
      blocking commands and not because they are dangerous. Now that we can run blocking
      commands on RM_Call, there is no real reason not to allow such commands on script mode.
      
      The underline problem is that the `NO_SCRIPT` flag is abused to also mark some of the
      blocking commands (notice that those commands know not to block the client if it is not
      allowed to do so, and have a fallback logic to such cases. So even if those commands
      were not marked with `NO_SCRIPT` flag, it would not harm Redis, and today we can
      already run those commands within multi exec).
      
      In addition, not all blocking commands are marked with `NO_SCRIPT` flag, for example
      `blmpop` are not marked and can run from within a script.
      
      Those facts shows that there are some ambiguity about the meaning of the `NO_SCRIPT`
      flag, and its not fully clear where it should be use.
      
      The PR suggest that blocking commands should not be marked with `NO_SCRIPT` flag,
      those commands should handle `CLIENT_DENY_BLOCKING` flag and only block when
      it's safe (like they already does today). To achieve that, the PR removes the `NO_SCRIPT`
      flag from the following commands:
      * `blmove`
      * `blpop`
      * `brpop`
      * `brpoplpush`
      * `bzpopmax`
      * `bzpopmin`
      * `wait`
      
      This might be considered a breaking change as now, on scripts, instead of getting
      `command is not allowed from script` error, the user will get some fallback behavior
      base on the command implementation. That said, the change matches the behavior
      of scripts and multi exec with respect to those commands and allow running them on
      `RM_Call` even when script mode is used.
      
      ### Additional RedisModule API and changes
      
      * `RM_BlockClientSetPrivateData` - Set private data on the blocked client without the
        need to unblock the client. This allows up to set the promise CallReply as the private
        data of the blocked client and abort it if the client gets disconnected.
      * `RM_BlockClientGetPrivateData` - Return the current private data set on a blocked client.
        We need it so we will have access to this private data on the disconnect callback.
      * On RM_Call, the returned reply will be added to the auto memory context only if auto
        memory is enabled, this allows us to keep the call reply for longer time then the context
        lifetime and does not force an unneeded borrow relationship between the CallReply and
        the RedisModuleContext.
      d0da0a6a
    • Binbin's avatar
      Bump codespell to 2.2.4, fix typos and outupdated comments (#11911) · 0b159b34
      Binbin authored
      Fix some seen typos and wrong comments.
      0b159b34
  21. 15 Mar, 2023 1 commit
    • KarthikSubbarao's avatar
      Custom authentication for Modules (#11659) · f8a5a4f7
      KarthikSubbarao authored
      
      
      This change adds new module callbacks that can override the default password based authentication associated with ACLs. With this, Modules can register auth callbacks through which they can implement their own Authentication logic. When `AUTH` and `HELLO AUTH ...` commands are used, Module based authentication is attempted and then normal password based authentication is attempted if needed.
      The new Module APIs added in this PR are - `RM_RegisterCustomAuthCallback` and `RM_BlockClientOnAuth` and `RedisModule_ACLAddLogEntryByUserName `.
      
      Module based authentication will be attempted for all Redis users (created through the ACL SETUSER cmd or through Module APIs) even if the Redis user does not exist at the time of the command. This gives a chance for the Module to create the RedisModule user and then authenticate via the RedisModule API - from the custom auth callback.
      
      For the AUTH command, we will support both variations - `AUTH <username> <password>` and `AUTH <password>`. In case of the `AUTH <password>` variation, the custom auth callbacks are triggered with “default” as the username and password as what is provided.
      
      
      ### RedisModule_RegisterCustomAuthCallback
      ```
      void RM_RegisterCustomAuthCallback(RedisModuleCtx *ctx, RedisModuleCustomAuthCallback cb) {
      ```
      This API registers a callback to execute to prior to normal password based authentication. Multiple callbacks can be registered across different modules. These callbacks are responsible for either handling the authentication, each authenticating the user or explicitly denying, or deferring it to other authentication mechanisms. Callbacks are triggered in the order they were registered. When a Module is unloaded, all the auth callbacks registered by it are unregistered. The callbacks are attempted, in the order of most recently registered callbacks, when the AUTH/HELLO (with AUTH field is provided) commands are called. The callbacks will be called with a module context along with a username and a password, and are expected to take one of the following actions:
      
       (1) Authenticate - Use the RM_Authenticate* API successfully and return `REDISMODULE_AUTH_HANDLED`. This will immediately end the auth chain as successful and add the OK reply.
      (2) Block a client on authentication - Use the `RM_BlockClientOnAuth` API and return `REDISMODULE_AUTH_HANDLED`. Here, the client will be blocked until the `RM_UnblockClient `API is used which will trigger the auth reply callback (provided earlier through the `RM_BlockClientOnAuth`). In this reply callback, the Module should authenticate, deny or skip handling authentication.
      (3) Deny Authentication - Return `REDISMODULE_AUTH_HANDLED` without authenticating or blocking the client. Optionally, `err` can be set to a custom error message. This will immediately end the auth chain as unsuccessful and add the ERR reply.
      (4) Skip handling Authentication - Return `REDISMODULE_AUTH_NOT_HANDLED` without blocking the client. This will allow the engine to attempt the next custom auth callback.
      
      If none of the callbacks authenticate or deny auth, then password based auth is attempted and will authenticate or add failure logs and reply to the clients accordingly.
      
      ### RedisModule_BlockClientOnAuth
      ```
      RedisModuleBlockedClient *RM_BlockClientOnAuth(RedisModuleCtx *ctx, RedisModuleCustomAuthCallback reply_callback,
                                                     void (*free_privdata)(RedisModuleCtx*,void*))
      ```
      This API can only be used from a Module from the custom auth callback. If a client is not in the middle of custom module based authentication, ERROR is returned. Otherwise, the client is blocked and the `RedisModule_BlockedClient` is returned similar to the `RedisModule_BlockClient` API.
      
      ### RedisModule_ACLAddLogEntryByUserName
      ```
      int RM_ACLAddLogEntryByUserName(RedisModuleCtx *ctx, RedisModuleString *username, RedisModuleString *object, RedisModuleACLLogEntryReason reason)
      ```
      Adds a new entry in the ACL log with the `username` RedisModuleString provided. This simplifies the Module usage because now, developers do not need to create a Module User just to add an error ACL Log entry. Aside from accepting username (RedisModuleString) instead of a RedisModuleUser, it is the same as the existing `RedisModule_ACLAddLogEntry` API.
      
      
      ### Breaking changes
      - HELLO command - Clients can now only set the client name and RESP protocol from the `HELLO` command if they are authenticated. Also, we now finish command arg validation first and return early with a ERR reply if any arg is invalid. This is to avoid mutating the client name / RESP from a command that would have failed on invalid arguments.
      
      ### Notable behaviors
      - Module unblocking - Now, we will not allow Modules to block the client from inside the context of a reply callback (triggered from the Module unblock flow `moduleHandleBlockedClients`).
      
      ---------
      Co-authored-by: default avatarMadelyn Olson <34459052+madolson@users.noreply.github.com>
      f8a5a4f7