1. 16 Jan, 2023 9 commits
    • Oran Agra's avatar
      Avoid integer overflows in SETRANGE and SORT (CVE-2022-35977) · 6c25c6b7
      Oran Agra authored
      Authenticated users issuing specially crafted SETRANGE and SORT(_RO)
      commands can trigger an integer overflow, resulting with Redis attempting
      to allocate impossible amounts of memory and abort with an OOM panic.
      6c25c6b7
    • Oran Agra's avatar
      Obuf limit, exit during loop in *RAND* commands and KEYS · 4537830e
      Oran Agra authored
      Related to the hang reported in #11671
      Currently, redis can disconnect a client due to reaching output buffer limit,
      it'll also avoid feeding that output buffer with more data, but it will keep
      running the loop in the command (despite the client already being marked for
      disconnection)
      
      This PR is an attempt to mitigate the problem, specifically for commands that
      are easy to abuse, specifically: KEYS, HRANDFIELD, SRANDMEMBER, ZRANDMEMBER.
      The RAND family of commands can take a negative COUNT argument (which is not
      bound to the number of elements in the key), so it's enough to create a key
      with one field, and then these commands can be used to hang redis.
      For KEYS the caller can use the existing keyspace in redis (if big enough).
      4537830e
    • knggk's avatar
      Add minimum version information to new xsetid arguments (#11694) · 5fa7d9a2
      knggk authored
      the metadata for the new arguments of XSETID,
      entries-added and max-deleted-id, which have been added
      in Redis 7.0 was missing.
      
      (cherry picked from commit 44c67703)
      5fa7d9a2
    • Oran Agra's avatar
      Make sure that fork child doesn't do incremental rehashing (#11692) · 3e82bdf7
      Oran Agra authored
      Turns out that a fork child calling getExpire while persisting keys (and
      possibly also a result of some module fork tasks) could cause dictFind
      to do incremental rehashing in the child process, which is both a waste
      of time, and also causes COW harm.
      
      (cherry picked from commit 2bec254d)
      3e82bdf7
    • Gabi Ganam's avatar
      Blocking command with a 0.001 seconds timeout blocks indefinitely (#11688) · 574a49b9
      Gabi Ganam authored
      Any value in the range of [0-1) turns to 0 when being cast from double to long long. This change rounds up instead of down for values that can't be stored precisely as long doubles.
      
      (cherry picked from commit eef29b68)
      574a49b9
    • Oran Agra's avatar
      Fix potential issue with Lua argv caching, module command filter and libc realloc (#11652) · 61a1d454
      Oran Agra authored
      TLDR: solve a problem introduced in Redis 7.0.6 (#11541) with
      RM_CommandFilterArgInsert being called from scripts, which can
      lead to memory corruption.
      
      Libc realloc can return the same pointer even if the size was changed. The code in
      freeLuaRedisArgv had an assumption that if the pointer didn't change, then the
      allocation didn't change, and the cache can still be reused.
      However, if rewriteClientCommandArgument or RM_CommandFilterArgInsert were
      used, it could be that we realloced the argv array, and the pointer didn't change, then
      a consecutive command being executed from Lua can use that argv cache reaching
      beyond its size.
      This was actually only possible with modules, since the decision to realloc was based
      on argc, rather than argv_len.
      
      (cherry picked from commit c8052122)
      61a1d454
    • judeng's avatar
      Optimize the performance of msetnx command by call lookupkey only once (#11594) · f9f48ef6
      judeng authored
      This is a small addition to #9640
      It improves performance by avoiding double lookup of the the key.
      
      (cherry picked from commit 884ca601)
      f9f48ef6
    • sundb's avatar
      Remove unnecessary updateClientMemUsageAndBucket() when feeding monitors (#11657) · b7b78a2d
      sundb authored
      This call is introduced in #8687, but became irrelevant in #11348, and is currently a no-op.
      The fact is that #11348 an unintended side effect, which is that even if the client eviction config
      is enabled, there are certain types of clients for which memory consumption is not accurately
      tracked, and so unlike normal clients, their memory isn't reported correctly in INFO.
      
      (cherry picked from commit af0a4fe2)
      b7b78a2d
    • Moti Cohen's avatar
      Fix sentinel issue if replica changes IP (#11590) · fcfb046d
      Moti Cohen authored
      
      
      As Sentinel supports dynamic IP only when using hostnames, there
      are few leftover addess comparison logic that doesn't take into
      account that the IP might get change.
      Co-authored-by: default avatarmoticless <moticless@github.com>
      (cherry picked from commit 4a27aa48)
      fcfb046d
  2. 16 Dec, 2022 2 commits
  3. 12 Dec, 2022 29 commits
    • Oran Agra's avatar
      Redis 7.0.6 · c0924a83
      Oran Agra authored
      c0924a83
    • Binbin's avatar
      Fix replication on expired key test timing issue, give it more chances (#11548) · e2665c6f
      Binbin authored
      In replica, the key expired before master's `INCR` was arrived, so INCR
      creates a new key in the replica and the test failed.
      ```
      *** [err]: Replication of an expired key does not delete the expired key in tests/integration/replication-4.tcl
      Expected '0' to be equal to '1' (context: type eval line 13 cmd {assert_equal 0 [$slave exists k]} proc ::test)
      ```
      
      This test is very likely to do a false positive if the `wait_for_ofs_sync`
      takes longer than the expiration time, so give it a few more chances.
      
      The test was introduced in #9572.
      
      (cherry picked from commit 06b577aa)
      e2665c6f
    • Binbin's avatar
      Fix redis-cli cluster add-node race in cli.tcl (#11349) · 3c525fab
      Binbin authored
      There is a race condition in the test:
      ```
      *** [err]: redis-cli --cluster add-node with cluster-port in tests/unit/cluster/cli.tcl
      Expected '5' to be equal to '4' {assert_equal 5 [CI 0 cluster_known_nodes]} proc ::test)
      ```
      
      When using cli to add node, there can potentially be a race condition
      in which all nodes presenting cluster state o.k even though the added
      node did not yet meet all cluster nodes.
      
      This comment and the fix were taken from #11221. Also apply it in several
      other similar places.
      
      (cherry picked from commit a549b78c)
      3c525fab
    • ranshid's avatar
      fix test Migrate the last slot away from a node using redis-cli (#11221) · e1557e6c
      ranshid authored
      When using cli to add node, there can potentially be a race condition in
      which all nodes presenting cluster state o.k even though the added node
      did not yet meet all cluster nodes.
      this adds another utility function to wait until all cluster nodes see the same cluster size
      
      (cherry picked from commit c0ce97fa)
      e1557e6c
    • Binbin's avatar
      Fix timing issue in cluster test (#11008) · b5784fea
      Binbin authored
      A timing issue like this was reported in freebsd daily CI:
      ```
      *** [err]: Sanity test push cmd after resharding in tests/unit/cluster/cli.tcl
      Expected 'CLUSTERDOWN The cluster is down' to match '*MOVED*'
      ```
      
      We additionally wait for each node to reach a consensus on the cluster
      state in wait_for_condition to avoid the cluster down error.
      
      The fix just like #10495, quoting madolson's comment:
      Cluster check just verifies the the config state is self-consistent,
      waiting for cluster_state to be okay is an independent check that all
      the nodes actually believe each other are healthy.
      
      At the same time i noticed that unit/moduleapi/cluster.tcl has an exact
      same test, may have the same problem, also modified it.
      
      (cherry picked from commit 5ce64ab0)
      b5784fea
    • Binbin's avatar
      Fix CLUSTERDOWN issue in cluster reshard unblock test (#11139) · a43c51b2
      Binbin authored
      change the cluster-node-timeout from 1 to 1000
      
      (cherry picked from commit 3a16ad30)
      a43c51b2
    • David CARLIER's avatar
      Fixes build warning when CACHE_LINE_SIZE is already defined. (#11389) · d86408b7
      David CARLIER authored
      * Fixes build warning when CACHE_LINE_SIZE is already defined
      * Fixes wrong CACHE_LINE_SIZE on some FreeBSD systems where it could be set to 128 (e.g. on MIPS)
      * Fixes wrong CACHE_LINE_SIZE on Apple M1 (use 128 instead of 64)
      
      Wrong cache line size in that case can some false sharing of array elements between threads, see #10892
      
      (cherry picked from commit 871cc200)
      d86408b7
    • Oran Agra's avatar
      Avoid ASAN test errors on crash report tests · dca63da4
      Oran Agra authored
      Clang Address Sanitizer tests started reporting unknown-crash on these
      tests due to the memcheck, disable the memcheck to avoid that noise.
      
      (cherry picked from commit 18ff6a3269a34b9bfe549b34bda9d83c3eae7e2a)
      dca63da4
    • Oran Agra's avatar
      Try to fix a race in psync2 test (#11553) · 4a5aba16
      Oran Agra authored
      This test sets the master ping interval to 1 hour, in order to avoid
      pings in the replicatoin stream incrementing the replication offset,
      however, it didn't increase the repl-timeout so on slow machines
      where the test took more than 60 seconds, the replicas would drop
      and reconnect.
      
      ```
      *** [err]: PSYNC2: Partial resync after restart using RDB aux fields in tests/integration/psync2.tcl
      Replica didn't partial sync
      ```
      
      The test would detect 4 additional partial syncs where it expects
      only one.
      
      (cherry picked from commit b0250b45)
      4a5aba16
    • Binbin's avatar
      Bump vmactions/freebsd-vm to 0.3.0 to fix FreeBSD daily (#11476) · 72759356
      Binbin authored
      Our FreeBSD daily has been failing recently:
      ```
        Config file: freebsd-13.1.conf
        cd: /Users/runner/work/redis/redis: No such file or directory
        gmake: *** No targets specified and no makefile found.  Stop.
      ```
      
      Upgrade vmactions/freebsd-vm to the latest version (0.3.0) can work.
      I've tested it, but don't know why, but first let's fix it.
      
      (cherry picked from commit 5246bf45)
      72759356
    • Binbin's avatar
      Fix bgsaveerr issue in psync wrong offset test (#11043) · 98410d0d
      Binbin authored
      The kill above is sometimes successful and sometimes already too late.
      The PING in pysnc wrong offset test got rejected by bgsaveerr because
      lastbgsave_status is C_ERR.
      
      In theory, using diskless can avoid PING being affected, because when
      the replica is dropped, we will kill the child with SIGUSR1, and this
      will not affect lastbgsave_status.
      
      Anyway, this kill is not particularly needed here, dropping the kill
      is the best one, since we do have the waitForBgsave, so just let it
      take care of the bgsave. No need for fast termination.
      
      (cherry picked from commit e7144693)
      98410d0d
    • filipe oliveira's avatar
      Reduce rewriteClientCommandVector usage on EXPIRE command (#11602) · 9ccb8dfd
      filipe oliveira authored
      
      
      There is overhead on Redis 7.0 EXPIRE command that is not present on 6.2.7.
      
      We could see that on the unstable profile there are around 7% of CPU cycles
      spent on rewriteClientCommandVector that are not present on 6.2.7.
      This was introduced in #8474.
      This PR reduces the overhead by using 2X rewriteClientCommandArgument instead of
      rewriteClientCommandVector. In this scenario rewriteClientCommandVector creates 4 arguments.
      the above usage of rewriteClientCommandArgument reduces the overhead in half.
      
      This PR should also improve PEXPIREAT performance by avoiding at all
      rewriteClientCommandArgument usage.
      Co-authored-by: default avatarOran Agra <oran@redislabs.com>
      (cherry picked from commit c3fb48da)
      9ccb8dfd
    • Harkrishn Patro's avatar
      Optimize client memory usage tracking operation while client eviction is disabled (#11348) · 05c9378b
      Harkrishn Patro authored
      
      
      ## Issue
      During the client input/output buffer processing, the memory usage is
      incrementally updated to keep track of clients going beyond a certain
      threshold `maxmemory-clients` to be evicted. However, this additional
      tracking activity leads to unnecessary CPU cycles wasted when no
      client-eviction is required. It is applicable in two cases.
      
      * `maxmemory-clients` is set to `0` which equates to no client eviction
        (applicable to all clients)
      * `CLIENT NO-EVICT` flag is set to `ON` which equates to a particular
        client not applicable for eviction.
      
      ## Solution
      * Disable client memory usage tracking during the read/write flow when
        `maxmemory-clients` is set to `0` or `client no-evict` is `on`.
        The memory usage is tracked only during the `clientCron` i.e. it gets
        periodically updated.
      * Cleanup the clients from the memory usage bucket when client eviction
        is disabled.
      * When the maxmemory-clients config is enabled or disabled at runtime,
        we immediately update the memory usage buckets for all clients (tested
        scanning 80000 took some 20ms)
      
      Benchmark shown that this can improve performance by about 5% in
      certain situations.
      Co-authored-by: default avatarOran Agra <oran@redislabs.com>
      (cherry picked from commit c0267b3f)
      05c9378b
    • Binbin's avatar
      Fix command line startup --sentinel problem (#11591) · b322a77e
      Binbin authored
      There is a issue with --sentinel:
      ```
      [root]# src/redis-server sentinel.conf --sentinel --loglevel verbose
      
      *** FATAL CONFIG FILE ERROR (Redis 255.255.255) ***
      Reading the configuration file, at line 352
      >>> 'sentinel "--loglevel" "verbose"'
      Unrecognized sentinel configuration statement
      ```
      
      This is because in #10660 (Redis 7.0.1), `--` prefix change break it.
      In this PR, we will handle `--sentinel` the same as we did for `--save`
      in #10866. i.e. it's a pseudo config option with no value.
      
      (cherry picked from commit 8f13ac10)
      b322a77e
    • filipe oliveira's avatar
      GEOSEARCH BYBOX: Simplified haversine distance formula when longitude diff is 0 (#11579) · 5e02338e
      filipe oliveira authored
      This is take 2 of `GEOSEARCH BYBOX` optimizations based on haversine
      distance formula when longitude diff is 0.
      The first one was in #11535 .
      
      - Given longitude diff is 0 the asin(sqrt(a)) on the haversine is asin(sin(abs(u))).
      - arcsin(sin(x)) equal to x when x ∈[−𝜋/2,𝜋/2].
      - Given latitude is between [−𝜋/2,𝜋/2] we can simplifiy arcsin(sin(x)) to x.
      
      On the sample dataset with 60M datapoints, we've measured 55% increase
      in the achievable ops/sec.
      
      (cherry picked from commit e48ac075)
      5e02338e
    • filipe oliveira's avatar
      Reintroduce lua argument cache in luaRedisGenericCommand removed in v7.0 (#11541) · 6a6d8806
      filipe oliveira authored
      This mechanism aims to reduce calls to malloc and free when
      preparing the arguments the script sends to redis commands.
      This is a mechanism was originally implemented in 48c49c48
      and 4f686555
      
      , and was removed in #10220 (thinking it's not needed
      and that it has no impact), but it now turns out it was wrong, and it
      indeed provides some 5% performance improvement.
      
      The implementation is a little bit too simplistic, it assumes consecutive
      calls use the same size in the same arg index, but that's arguably
      sufficient since it's only aimed at caching very small things.
      
      We could even consider always pre-allocating args to the full
      LUA_CMD_OBJCACHE_MAX_LEN (64 bytes) rather than the right size for the argument,
      that would increase the chance they'll be able to be re-used.
      But in some way this is already happening since we're using
      sdsalloc, which in turn uses s_malloc_usable and takes ownership
      of the full side of the allocation, so we are padded to the allocator
      bucket size.
      Co-authored-by: default avatarOran Agra <oran@redislabs.com>
      Co-authored-by: default avatarsundb <sundbcn@gmail.com>
      (cherry picked from commit 2d80cd78)
      6a6d8806
    • filipe oliveira's avatar
      Speedup GEODIST with fixedpoint_d2string as an optimized version of snprintf %.4f (#11552) · 77570e39
      filipe oliveira authored
      
      
      GEODIST used snprintf("%.4f") for the reply using addReplyDoubleDistance,
      which was slow. This PR optimizes it without breaking compatibility by following
      the approach of ll2string with some changes to match the use case of distance
      and precision. I.e. we multiply it by 10000 format it as an integer, and then add
      a decimal point. This can achieve about 35% increase in the achievable ops/sec.
      Co-authored-by: default avatarOran Agra <oran@redislabs.com>
      (cherry picked from commit 61c85a2b)
      77570e39
    • Yossi Gottlieb's avatar
      Improve TLS error handling. (#11563) · 60ef6d28
      Yossi Gottlieb authored
      * Remove duplicate code, propagating SSL errors into connection state.
      * Add missing error handling in synchronous IO functions.
      * Fix connection error reporting in some replication flows.
      
      (cherry picked from commit 155acef5)
      60ef6d28
    • filipe oliveira's avatar
      changing addReplySds and sdscat to addReplyStatusLength() within luaReplyToRedisReply() (#11556) · 669a8dba
      filipe oliveira authored
      profiling EVALSHA\ we see that luaReplyToRedisReply takes 8.73% out of the
      56.90% of luaCallFunction CPU cycles.
      
      Using addReplyStatusLength instead of directly composing the protocol to avoid
      sdscatprintf and addReplySds ( which imply multiple sdslen calls ).
      
      The new approach drops
      luaReplyToRedisReply CPU cycles to 3.77%
      
      (cherry picked from commit 68e87eb0)
      669a8dba
    • filipe oliveira's avatar
      Reduce eval related overhead introduced in v7.0 by evalCalcFunctionName (#11521) · 7c4f6e17
      filipe oliveira authored
      
      
      As being discussed in #10981 we see a degradation in performance
      between v6.2 and v7.0 of Redis on the EVAL command.
      
      After profiling the current unstable branch we can see that we call the
      expensive function evalCalcFunctionName twice.
      
      The current "fix" is to basically avoid calling evalCalcFunctionName and
      even dictFind(lua_scripts) twice for the same command.
      Instead we cache the current script's dictEntry (for both Eval and Functions)
      in the current client so we don't have to repeat these calls.
      The exception would be when doing an EVAL on a new script that's not yet
      in the script cache. in that case we will call evalCalcFunctionName (and even
      evalExtractShebangFlags) twice.
      Co-authored-by: default avatarOran Agra <oran@redislabs.com>
      (cherry picked from commit 7dfd7b91)
      7c4f6e17
    • zhaozhao.zz's avatar
      benchmark getRedisConfig exit only when meet NOAUTH error (#11096) · fba49b10
      zhaozhao.zz authored
      redis-benchmark: when trying to get the CONFIG before benchmark,
      avoid printing any warning on most errors (e.g. NOPERM error).
      avoid aborting the benchmark on NOPERM.
      keep the warning only when we abort the benchmark on a NOAUTH error
      
      (cherry picked from commit f0005b53)
      fba49b10
    • filipe oliveira's avatar
      Simplified geoAppendIfWithinShape() and removed spurious calls do sdsdup and sdsfree (#11522) · 3d206f0f
      filipe oliveira authored
      
      
      In scenarios in which we have large datasets and the elements are not
      contained within the range we do spurious calls do sdsdup and sdsfree.
      I.e. instead of pre-creating an sds before we know if we're gonna use it
      or not, change the role of geoAppendIfWithinShape to just do geoWithinShape,
      and let the caller create the string only when needed.
      Co-authored-by: default avatarOran Agra <oran@redislabs.com>
      (cherry picked from commit 376b689b)
      3d206f0f
    • DevineLiu's avatar
      [BUG] Fix announced ports not updating on local node when updated at runtime (#10745) · 81e72e8e
      DevineLiu authored
      
      
      The cluster-announce-port/cluster-announce-bus-port/cluster-announce-tls-port should take effect at runtime
      Co-authored-by: default avatarMadelyn Olson <madelyneolson@gmail.com>
      (cherry picked from commit 25ffa79b)
      81e72e8e
    • filipe oliveira's avatar
      GEOSEARCH BYBOX: Reduce wastefull computation on... · cffabb64
      filipe oliveira authored
      
      GEOSEARCH BYBOX: Reduce wastefull computation on geohashGetDistanceIfInRectangle and geohashGetDistance (#11535)
      
      Optimize geohashGetDistanceIfInRectangle when there are many misses.
      It calls 3x geohashGetDistance. The first 2 times we call them to produce intermediate results.
      This PR focus on optimizing for those 2 intermediate results.
      
      1 Reduce expensive computation on intermediate geohashGetDistance with same long
      2 Avoid expensive lon_distance calculation if lat_distance fails beforehand
      Co-authored-by: default avatarOran Agra <oran@redislabs.com>
      (cherry picked from commit ae1de549)
      cffabb64
    • sundb's avatar
      Ignore -Wstringop-overread warning for SHA1Transform() on GCC 12 (#11538) · 4972f576
      sundb authored
      Fix compile warning for SHA1Transform() method under alpine with GCC 12.
      
      Warning:
      ```
      In function 'SHA1Update',
          inlined from 'SHA1Final' at sha1.c:187:9:
      sha1.c:144:13: error: 'SHA1Transform' reading 64 bytes from a region of size 0 [-Werror=stringop-overread]
        144 |             SHA1Transform(context->state, &data[i]);
            |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
      sha1.c:144: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])
            |      ^~~~~~~~~~~~~
      ```
      
      This warning is a false positive because it has been determined in the loop judgment that there must be 64 chars after position `i`
      ```c
      for ( ; i + 63 < len; i += 64) {
          SHA1Transform(context->state, &data[i]);
      }
      ```
      
      Reference: https://github.com/libevent/libevent/commit/e1d7d3e40a7fd50348d849046fbfd9bf976e643c
      (cherry picked from commit fd808185)
      4972f576
    • Wen Hui's avatar
      Add explicit error log message for AOF_TRUNCATED status when server load AOF file (#11484) · d81c2a5f
      Wen Hui authored
      Now, according to the comments, if the truncated file is not the last file,
      it will be considered as a fatal error.
      And the return code will updated to AOF_FAILED, then server will exit
      without any error message to the client.
      
      Similar to other error situations, this PR add an explicit error message
      for this case and make the client know clearly what happens.
      
      (cherry picked from commit 6e9724cb)
      d81c2a5f
    • 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