1. 12 Dec, 2022 16 commits
    • Madelyn Olson's avatar
      Explicitly send function commands to monitor (#11510) · a26ac7eb
      Madelyn Olson authored
      Both functions and eval are marked as "no-monitor", since we want to explicitly feed in the script command before the commands generated by the script. Note that we want this behavior generally, so that commands can redact arguments before being added to the monitor.
      
      (cherry picked from commit d136bf28)
      a26ac7eb
    • uriyage's avatar
      Module CLIENT_CHANGE, Fix crash on free blocked client with DB!=0 (#11500) · 7ad786db
      uriyage authored
      
      
      In moduleFireServerEvent we change the real client DB to 0 on freeClient in case the event is REDISMODULE_EVENT_CLIENT_CHANGE.
      It results in a crash if the client is blocked on a key on other than DB 0.
      
      The DB change is not necessary even for module-client, as we set its DB to 0 on either createClient or moduleReleaseTempClient.
      Co-authored-by: default avatarMadelyn Olson <34459052+madolson@users.noreply.github.com>
      Co-authored-by: default avatarBinbin <binloveplay1314@qq.com>
      (cherry picked from commit e4eb18b3)
      7ad786db
    • Oran Agra's avatar
      fixes for fork child exit and test: #11463 (#11499) · abf83bc6
      Oran Agra authored
      
      
      Fix a few issues with the recent #11463
      * use exitFromChild instead of exit
      * test should ignore defunct process since that's what we expect to
        happen for thees child processes when the parent dies.
      * fix typo
      Co-authored-by: default avatarBinbin <binloveplay1314@qq.com>
      (cherry picked from commit 4c54528f)
      abf83bc6
    • Oran Agra's avatar
      diskless master, avoid bgsave child hung when fork parent crashes (#11463) · 7c956d5c
      Oran Agra authored
      During a diskless sync, if the master main process crashes, the child would
      have hung in `write`. This fix closes the read fd on the child side, so that if the
      parent crashes, the child will get a write error and exit.
      
      This change also fixes disk-based replication, BGSAVE and AOFRW.
      In that case the child wouldn't have been hang, it would have just kept
      running until done which may be pointless.
      
      There is a certain degree of risk here. in case there's a BGSAVE child that could
      maybe succeed and the parent dies for some reason, the old code would have let
      the child keep running and maybe succeed and avoid data loss.
      On the other hand, if the parent is restarted, it would have loaded an old rdb file
      (or none), and then the child could reach the end and rename the rdb file (data
      conflicting with what the parent has), or also have a race with another BGSAVE
      child that the new parent started.
      
      Note that i removed a comment saying a write error will be ignored in the child
      and handled by the parent (this comment was very old and i don't think relevant).
      
      (cherry picked from commit ccaef5c9)
      7c956d5c
    • Moti Cohen's avatar
      Fix sentinel function that compares hostnames (if failed resolve) (#11419) · 19d01b62
      Moti Cohen authored
      Funcion sentinelAddrEqualsHostname() of sentinel makes DNS resolve
      and based on it determines if two IP addresses are equal. Now, If the
      DNS resolve command fails, the function simply returns 0, even if the
      hostnames are identical.
      
      This might become an issue in case of failover such that sentinel might
      receives from Redis instance, response to regular INFO query it sent,
      and wrongly decide that the instance is pointing to is different leader
      than the one recorded because of this function, yet hostnames are
      identical. In turn sentinel disconnects the connection between sentinel
      and valid slave which leads to -failover-abort-no-good-slave.
      See issue #11241.
      
      I managed to reproduce only part of the flow in which the function
      return wrong result and trigger +fix-slave-config.
      
      The fix is even if the function failed to resolve then compare based on
      hostnames. That is our best effort as long as the server is unavailable
      for some reason. It is fine since Redis instance cannot have multiple
      hostnames for a given setup
      
      (cherry picked from commit bd23b15a)
      19d01b62
    • sundb's avatar
      Fix crash due to to reuse iterator entry after list deletion in module (#11383) · 9fc20f4f
      sundb authored
      
      
      In the module, we will reuse the list iterator entry for RM_ListDelete, but `listTypeDelete` will only update
      `quicklistEntry->zi` but not `quicklistEntry->node`, which will result in `quicklistEntry->node` pointing to
      a freed memory address if the quicklist node is deleted.
      
      This PR sync `key->u.list.index` and `key->u.list.entry` to list iterator after `RM_ListDelete`.
      
      This PR also optimizes the release code of the original list iterator.
      Co-authored-by: default avatarViktor Söderqvist <viktor@zuiderkwast.se>
      (cherry picked from commit 6dd21355)
      9fc20f4f
    • C Charles's avatar
      MIGTATE with AUTH that contains "keys" is getting wrong key names in... · f95af778
      C Charles authored
      MIGTATE with AUTH that contains "keys" is getting wrong key names in migrateGetKeys, leads to ACL errors (#11253)
      
      When using the MIGRATE, with a destination Redis that has the user name or password set to the string "keys",
      Redis would have determine the wrong set of key names the command is gonna access.
      This lead to ACL returning wrong authentication result.
      
      Destination instance:
      ```
      127.0.0.1:6380> acl setuser default >keys
      OK
      127.0.0.1:6380> acl setuser keys on nopass ~* &* +@all
      OK
      ```
      
      Source instance:
      ```
      127.0.0.1:6379> set a 123
      OK
      127.0.0.1:6379> acl setuser cc on nopass ~a* +@all
      OK
      127.0.0.1:6379> auth cc 1
      OK
      127.0.0.1:6379> migrate 127.0.0.1 6380 "" 0 1000 auth keys keys a
      (error) NOPERM this user has no permissions to access one of the keys used as arguments
      127.0.0.1:6379> migrate 127.0.0.1 6380 "" 0 1000 auth2 keys pswd keys a
      (error) NOPERM this user has no permissions to access one of the keys used as arguments
      ```
      
      Using `acl dryrun` we know that the parameters of `auth` and `auth2` are mistaken for the `keys` option.
      ```
      127.0.0.1:6379> acl dryrun cc migrate whatever whatever "" 0 1000 auth keys keys a
      "This user has no permissions to access the 'keys' key"
      127.0.0.1:6379> acl dryrun cc migrate whatever whatever "" 0 1000 auth2 keys pswd keys a
      "This user has no permissions to access the 'pswd' key"
      ```
      
      Fix the bug by editing db.c/migrateGetKeys function, which finds the `keys` option and all the keys following.
      
      (cherry picked from commit 9ab873d9)
      f95af778
    • Oran Agra's avatar
      Improve linux overcommit check and warning (#11357) · 92ad0b5c
      Oran Agra authored
      1. show the overcommit warning when overcommit is disabled (2),
         not just when it is set to heuristic (0).
      2. improve warning text to mention the issue with jemalloc causing VM
         mapping fragmentation when set to 2.
      
      (cherry picked from commit dd60c6c8)
      92ad0b5c
    • Meir Shpilraien (Spielrein)'s avatar
      `RedisModule_ResetDataset` should not clear the functions. (#11268) · 6cf24fa4
      Meir Shpilraien (Spielrein) authored
      As mentioned on docs, `RM_ResetDataset` Performs similar operation to FLUSHALL.
      As FLUSHALL do not clean the function, `RM_ResetDataset` should not clean the functions
      as well.
      
      (cherry picked from commit d2ad01ab)
      6cf24fa4
    • Meir Shpilraien (Spielrein)'s avatar
      Avoid crash on crash report when a bad function pointer was called (#11298) · b1ede212
      Meir Shpilraien (Spielrein) authored
      If Redis crashes due to calling an invalid function pointer,
      the `backtrace` function will try to dereference this invalid pointer
      which will cause a crash inside the crash report and will kill
      the processes without having all the crash report information.
      
      Example:
      
      ```
      === REDIS BUG REPORT START: Cut & paste starting from here ===
      198672:M 19 Sep 2022 18:06:12.936 # Redis 255.255.255 crashed by signal: 11, si_code: 1
      198672:M 19 Sep 2022 18:06:12.936 # Accessing address: 0x1
      198672:M 19 Sep 2022 18:06:12.936 # Crashed running the instruction at: 0x1
      // here the processes is crashing
      ```
      
      This PR tries to fix this crash be:
      1. Identify the issue when it happened.
      2. Replace the invalid pointer with a pointer to some dummy function
         so that `backtrace` will not crash.
      
      I identification is done by comparing `eip` to `info->si_addr`, if they
      are the same we know that the crash happened on the same address it tries to
      accesses and we can conclude that it tries to call and invalid function pointer.
      
      To replace the invalid pointer we introduce a new function, `setMcontextEip`,
      which is very similar to `getMcontextEip` and it knows to set the Eip for the
      different supported OS's. After printing the trace we retrieve the old `Eip` value.
      
      (cherry picked from commit 0bf90d94)
      b1ede212
    • guybe7's avatar
      RM_CreateCommand should not set CMD_KEY_VARIABLE_FLAGS automatically (#11320) · 5b2119c6
      guybe7 authored
      The original idea behind auto-setting the default (first,last,step) spec was to use
      the most "open" flags when the user didn't provide any key-spec flags information.
      
      While the above idea is a good approach, it really makes no sense to set
      CMD_KEY_VARIABLE_FLAGS if the user didn't provide the getkeys-api flag:
      in this case there's not way to retrieve these variable flags, so what's the point?
      
      Internally in redis there was code to ignore this already, so this fix doesn't change
      redis's behavior, it only affects the output of COMMAND command.
      
      (cherry picked from commit 3330ea18)
      5b2119c6
    • Steffen Moser's avatar
      Fixing compilation by removing flock() when compiling on Solaris (#11327) · 929ab58a
      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
      
      (cherry picked from commit 6aab4cb7)
      929ab58a
    • Binbin's avatar
      Fix CLUSTER SHARDS showing empty hostname (#11297) · 79414d45
      Binbin authored
      * Fix CLUSTER SHARDS showing empty hostname
      
      In #10290, we changed clusterNode hostname from `char*`
      to `sds`, and the old `node->hostname` was changed to
      `sdslen(node->hostname)!=0`.
      
      But in `addNodeDetailsToShardReply` it is missing.
      It results in the return of an empty string hostname
      in CLUSTER SHARDS command if it unavailable.
      
      Like this (note that we listed it as optional in the doc):
      ```
       9) "hostname"
      10) ""
      ```
      
      (cherry picked from commit 1de675b3)
      79414d45
    • Shaya Potter's avatar
      Add RM_SetContextUser to support acl validation in RM_Call (and scripts) (#10966) · b8fcd322
      Shaya Potter authored
      Adds a number of user management/ACL validaiton/command execution functions to improve a
      Redis module's ability to enforce ACLs correctly and easily.
      
      * RM_SetContextUser - sets a RedisModuleUser on the context, which RM_Call will use to both
        validate ACLs (if requested and set) as well as assign to the client so that scripts executed via
        RM_Call will have proper ACL validation.
      * RM_SetModuleUserACLString - Enables one to pass an entire ACL string, not just a single OP
        and have it applied to the user
      * RM_GetModuleUserACLString - returns a stringified version of the user's ACL (same format as dump
        and list).  Contains an optimization to cache the stringified version until the underlying ACL is modified.
      * Slightly re-purpose the "C" flag to RM_Call from just being about ACL check before calling the
        command, to actually running the command with the right user, so that it also affects commands
        inside EVAL scripts. see #11231
      
      (cherry picked from commit 6e993a5d)
      b8fcd322
    • Valentino Geron's avatar
      Replica that asks for rdb only should be closed right after the rdb part (#11296) · d279ec25
      Valentino Geron authored
      The bug is that the the server keeps on sending newlines to the client.
      As a result, the receiver might not find the EOF marker since it searches
      for it only on the end of each payload it reads from the socket.
      The but only affects `CLIENT_REPL_RDBONLY`.
      This affects `redis-cli --rdb` (depending on timing)
      
      The fixed consist of two steps:
      1. The `CLIENT_REPL_RDBONLY` should be closed ASAP (we cannot
         always call to `freeClient` so we use `freeClientAsync`)
      2. Add new replication state `SLAVE_STATE_RDB_TRANSMITTED`
      
      (cherry picked from commit e53bf652)
      d279ec25
    • Ariel Shtul's avatar
      [PERF] use snprintf once in addReplyDouble (#11093) · a221fc85
      Ariel Shtul authored
      The previous implementation calls `snprintf` twice, the second time used to
      'memcpy' the output of the first, which could be a very large string.
      The new implementation reserves space for the protocol header ahead
      of the formatted double, and then prepends the string length ahead of it.
      
      Measured improvement of simple ZADD of some 25%.
      
      (cherry picked from commit 90223759)
      a221fc85
  2. 21 Sep, 2022 24 commits