1. 23 Nov, 2023 6 commits
    • meiravgri's avatar
      Fix async safety in signal handlers (#12658) · 2e854bcc
      meiravgri authored
      see discussion from after https://github.com/redis/redis/pull/12453 was
      merged
      ----
      This PR replaces signals that are not considered async-signal-safe
      (AS-safe) with safe calls.
      
      #### **1. serverLog() and serverLogFromHandler()**
      `serverLog` uses unsafe calls. It was decided that we will **avoid**
      `serverLog` calls by the signal handlers when:
      * The signal is not fatal, such as SIGALRM. In these cases, we prefer
      using `serverLogFromHandler` which is the safe version of `serverLog`.
      Note they have different prompts:
      `serverLog`: `62220:M 26 Oct 2023 14:39:04.526 # <msg>`
      `serverLogFromHandler`: `62220:signal-handler (1698331136) <msg>`
      * The code was added recently. Calls to `serverLog` by the signal
      handler have been there ever since Redis exists and it hasn't caused
      problems so far. To avoid regression, from now we should use
      `serverLogFromHandler`
      
      #### **2. `snprintf` `fgets` and `strtoul`(base = 16) -------->
      `_safe_snprintf`, `fgets_async_signal_safe`, `string_to_hex`**
      The safe version of `snprintf` was taken from
      [here](https://github.com/twitter/twemcache/blob/8cfc4ca5e76ed936bd3786c8cc43ed47e7778c08/src/mc_util.c#L754)
      
      #### **3. fopen(), fgets(), fclose() --------> open(), read(), close()**
      
      #### **4. opendir(), readdir(), closedir() --------> open(),
      syscall(SYS_getdents64), close()**
      
      #### **5. Threads_mngr sync mechanisms**
      * waiting for the thread to generate stack trace: semaphore -------->
      busy-wait
      * `globals_rw_lock` was removed: as we are not using malloc and the
      semaphore anymore we don't need to protect `ThreadsManager_cleanups`.
      
      #### **6. Stacktraces buffer**
      The initial problem was that we were not able to safely call malloc
      within the signal handler.
      To solve that we created a buffer on the stack of `writeStacktraces` and
      saved it in a global pointer, assuming that under normal circumstances,
      the function `writeStacktraces` would complete before any thread
      attempted to write to it. However, **if threads lag behind, they might
      access this global pointer after it no longer belongs to the
      `writeStacktraces` stack, potentially corrupting memory.**
      To address this, various solutions were discussed
      [here](https://github.com/redis/redis/pull/12658#discussion_r1390442896)
      Eventually, we decided to **create a pipe** at server startup that will
      remain valid as long as the process is alive.
      We chose this solution due to its minimal memory usage, and since
      `write()` and `read()` are atomic operations. It ensures that stack
      traces from different threads won't mix.
      
      **The stacktraces collection process is now as  follows:**
      * Cleaning the pipe to eliminate writes of late threads from previous
      runs.
      * Each thread writes to the pipe its stacktrace
      * Waiting for all the threads to mark completion or until a timeout (2
      sec) is reached
      * Reading from the pipe to print the stacktraces.
      
      #### **7. Changes that were considered and eventually were dropped**
      * replace watchdog timer with a POSIX timer: 
      according to [settimer man](https://linux.die.net/man/2/setitimer)
      
      > POSIX.1-2008 marks getitimer() and setitimer() obsolete, recommending
      the use of the POSIX timers API
      ([timer_gettime](https://linux.die.net/man/2/timer_gettime)(2),
      [timer_settime](https://linux.die.net/man/2/timer_settime)(2), etc.)
      instead.
      
      However, although it is supposed to conform to POSIX std, POSIX timers
      API is not supported on Mac.
      You can take a look here at the Linux implementation:
      
      [here](https://github.com/redis/redis/commit/c7562ee13546e504977372fdf40d33c3f86775a5
      
      )
      To avoid messing up the code, and uncertainty regarding compatibility,
      it was decided to drop it for now.
      
      * avoid using sds (uses malloc) in logConfigDebugInfo
      It was considered to print config info instead of using sds, however
      apparently, `logConfigDebugInfo` does more than just print the sds, so
      it was decided this fix is out of this issue scope.
      
      #### **8. fix Signal mask check**
      The check `signum & sig_mask` intended to indicate whether the signal is
      blocked by the thread was incorrect. Actually, the bit position in the
      signal mask corresponds to the signal number. We fixed this by changing
      the condition to: `sig_mask & (1L << (sig_num - 1))`
      
      #### **9. Unrelated changes**
      both `fork.tcl `and `util.tcl` implemented a function called
      `count_log_message` expecting different parameters. This caused
      confusion when trying to run daily tests with additional test parameters
      to run a specific test.
      The `count_log_message` in `fork.tcl` was removed and the calls were
      replaced with calls to `count_log_message` located in `util.tcl`
      
      ---------
      Co-authored-by: default avatarOzan Tezcan <ozantezcan@gmail.com>
      Co-authored-by: default avatarOran Agra <oran@redislabs.com>
      2e854bcc
    • Binbin's avatar
      Fix misleading error message in redis.log when loglevel is invalid (#12636) · 5e403099
      Binbin authored
      We don't have any debug level, change it to log level.
      5e403099
    • Moshe Kaplan's avatar
      rdb.c: Avoid potential file handle leak (Coverity 404720) (#12795) · c9aa586b
      Moshe Kaplan authored
      `open()` can return any non-negative integer on success, including zero.
      This change modifies the check from open()'s return value to also
      include checking for a return value of zero (e.g., if stdin were closed
      and then `open()` was called).
      
      Fixes Coverity 404720
      
      Can't happen in Redis. just a cleanup.
      c9aa586b
    • Moshe Kaplan's avatar
      redis-check-aof.c: Avoid leaking file handle if file is zero bytes (#12797) · ae09d4d3
      Moshe Kaplan authored
      If fopen() is successful, but redis_fstat() determines the file is zero
      bytes, the file handle stored in fp will leak. This change closes the
      filehandle stored in fp if the file is zero bytes.
      
      An FD leak on a tool like redis-check-aof isn't an issue (it'll exit soon anyway).
      This is just a cleanup.
      ae09d4d3
    • Moshe Kaplan's avatar
      config.c: Avoid leaking file handle if redis_fstat() fails (#12796) · 1c48d3da
      Moshe Kaplan authored
      If fopen() is successful, but redis_fstat() fails, the file handle
      stored in fp will leak. This change closes the filehandle stored in fp
      if redis_fstat() fails.
      
      Fixes Coverity 390029
      1c48d3da
    • Moshe Kaplan's avatar
      util.c: Don't leak directory handle if recursive call to dirRemove fails (#12800) · 157e5d47
      Moshe Kaplan authored
      If a recursive call to dirRemove() returns -1, dirRemove() the directory
      handle stored in dir will leak. This change closes the directory handle
      stored in dir even if the recursive call to dirRemove() returns -1.
      
      Fixed Coverity 371073
      157e5d47
  2. 22 Nov, 2023 15 commits
  3. 21 Nov, 2023 4 commits
  4. 20 Nov, 2023 1 commit
  5. 19 Nov, 2023 1 commit
    • Hwang Si Yeon's avatar
      Add an explanation for URI with -u in redis-cli --help (#12751) · a1f91ffa
      Hwang Si Yeon authored
      Add documentation of the URI format in the `--help` output of
      `redis-cli` and `redis-benchmark`.
      
      In particular, it's good for users to know that they need to specify
      "default" as the username when authenticating without a username. Other
      details of the URI format are described too, like scheme and dbnum.
      
      It used to be possible to connect to Redis using an URL with an empty
      username, like `redis-cli -u redis://:PASSWORD@localhost:6379/0`. This
      was broken in 6.2 (#8048), and there was a discussion about it #9186.
      Now, users need to specify "default" as the username and it's better to
      document it.
      
      Refer to #12746 for more details.
      a1f91ffa
  6. 17 Nov, 2023 2 commits
  7. 16 Nov, 2023 1 commit
  8. 15 Nov, 2023 1 commit
    • Binbin's avatar
      Empty rehashing list in emptyDbStructure (#12764) · 4366bbaa
      Binbin authored
      This is currently harmless, since we have already cleared the dict
      before, it will reset the rehashidx to -1, and in incrementallyRehash
      we will call dictIsRehashing to check.
      
      It would be nice to empty the list to avoid meaningless attempts, and
      the code is also unified to reduce misunderstandings.
      4366bbaa
  9. 14 Nov, 2023 1 commit
    • Binbin's avatar
      Fix DB iterator not resetting pauserehash causing dict being unable to rehash (#12757) · fe363063
      Binbin authored
      When using DB iterator, it will use dictInitSafeIterator to init a old safe
      dict iterator. When dbIteratorNext is used, it will jump to the next slot db
      dict when we are done a dict. During this process, we do not have any calls to
      dictResumeRehashing, which causes the dict's pauserehash to always be > 0.
      
      And at last, it will be returned directly in dictRehashMilliseconds, which causes
      us to have slot dict in a state where rehash cannot be completed.
      
      In the "expire scan should skip dictionaries with lot's of empty buckets" test,
      adding a `keys *` can reproduce the problem stably. `keys *` will call dbIteratorNext
      to trigger a traversal of all slot dicts.
      
      Added dbReleaseIterator and dbIteratorInitNextSafeIterator methods to call dictResetIterator.
      Issue was introduced in #11695.
      fe363063
  10. 12 Nov, 2023 1 commit
    • Roshan Khatri's avatar
      Add DEBUG_ASSERTIONS option to custom assert (#12667) · 88e83e51
      Roshan Khatri authored
      This PR introduces a new macro, serverAssertWithInfoDebug, to do complex assertions only for debugging. The main intention is to allow running complex operations during tests without impacting runtime performance. This assertion is enabled when setting DEBUG_ASSERTIONS.
      
      The DEBUG_ASSERTIONS flag is set for the daily and CI variants of `test-sanitizer-address`.
      88e83e51
  11. 10 Nov, 2023 1 commit
  12. 09 Nov, 2023 1 commit
  13. 08 Nov, 2023 2 commits
    • Binbin's avatar
      Fix genClusterDebugString minor sds leaks (#12739) · 53294e53
      Binbin authored
      This function now will only be called in printCrashReport,
      so this is just a cleanup.
      53294e53
    • Meir Shpilraien (Spielrein)'s avatar
      Before evicted and before expired server events are not executed inside an execution unit. (#12733) · 0ffb9d2e
      Meir Shpilraien (Spielrein) authored
      Redis 7.2 (#9406) introduced a new modules event, `RedisModuleEvent_Key`.
      This new event allows the module to read the key data just before it is removed
      from the database (either deleted, expired, evicted, or overwritten).
      
      When the key is removed from the database, either by active expire or eviction.
      The new event was not called as part of an execution unit. This can cause an
      issue if the module registers a post notification job inside the event. This job will
      not be executed atomically with the expiration/eviction operation and will not
      replicated inside a Multi/Exec. Moreover, the post notification job will be executed
      right after the event where it is still not safe to perform any write operation, this will
      violate the promise that post notification job will be called atomically with the
      operation that triggered it and **only when it is safe to write**.
      
      This PR fixes the issue by wrapping each expiration/eviction of a key with an execution
      unit. This makes sure the entire operation will run atomically and all the post notification
      jobs will be executed at the end where it is safe to write.
      
      Tests were modified to verify the fix.
      0ffb9d2e
  14. 06 Nov, 2023 2 commits
  15. 05 Nov, 2023 1 commit
    • Wen Hui's avatar
      Fix the bug that write redis sensitive command information to redis_cli historyfile (#11489) · 28b6155b
      Wen Hui authored
      Currently, we do not write the following sensitive commands into the ~/.rediscli_history file:
      
      ACL SETUSER username [rule [rule ...]]
      AUTH [username] password
      HELLO [AUTH username password] 
      MIGRATE host port <key | ""> destination-db timeout [[AUTH password | AUTH2 username password]]
      CONFIG SET masterauth master-password
      CONFIG SET masteruser username
      CONFIG SET requirepass foobared
      
      However, we still write the following sensitive commands into the ~/.rediscli_history file:
      ACL GETUSER username
      Sentinel CONFIG set sentinel-pass password
      Sentinel CONFIG set sentinel-user username
      Sentinel set mastername auth-pass password
      Sentinel set mastername auth-user username
      
      This change adds the commands of the second list to be skipped from being written to the history file.
      
      28b6155b