1. 22 May, 2022 1 commit
  2. 18 May, 2022 1 commit
  3. 16 May, 2022 2 commits
  4. 15 May, 2022 2 commits
  5. 13 May, 2022 2 commits
  6. 12 May, 2022 2 commits
    • Binbin's avatar
      Fix race in module fork kill test (#10717) · 586a16ad
      Binbin authored
      The purpose of the test is to kill the child while it is running.
      From the last two lines we can see the child exits before being killed.
      ```
      - Module fork started pid: 56998
      * <fork> fork child started
      - Killing running module fork child: 56998
      * <fork> fork child exiting
      signal-handler (1652267501) Received SIGUSR1 in child, exiting now.
      ```
      
      In this commit, we pass an argument to `fork.create` indicating how
      long it should sleep. For the fork kill test, we use a longer time to
      avoid the child exiting before being killed.
      
      Other changes:
      use wait_for_condition instead of hardcoded `after 250`.
      Unify the test for failing fork with the one for killing it (save time) 
      586a16ad
    • yoav-steinberg's avatar
      Fix possible regression around TLS config changes. Add VOLATILE_CONFIG flag... · b16d1c27
      yoav-steinberg authored
      Fix possible regression around TLS config changes. Add VOLATILE_CONFIG flag for volatile configurations. (#10713)
      
      This fixes a possible regression in Redis 7.0.0, in which doing CONFIG SET
      on a TLS config would not reload the configuration in case the new config is
      the same file as before.
      
      A volatile configuration is a configuration value which is a reference to the
      configuration data and not the configuration data itself. In such a case Redis
      doesn't know if the config data changed under the hood and can't assume a
      change happens only when the config value changes. Therefore it needs to
      be applied even when setting a config value to the same value as it was before.
      b16d1c27
  7. 11 May, 2022 4 commits
    • Yossi Gottlieb's avatar
      b4146052
    • Yossi Gottlieb's avatar
      Fix Makefile.dep generation with ICC. (#10708) · 8bdd2d5d
      Yossi Gottlieb authored
      Before this commit, all source files including those that are not going
      to be compiled were used. Some of these files are platform specific and
      won't even pre-process on another platform. With GCC/Clang, that's not
      an issue and they'll simply ignore them, but ICC aborts in this case.
      
      This commit only attempts to generate Makefile.dep from the actual set
      of C source files that will be compiled.
      8bdd2d5d
    • Binbin's avatar
      redis-server command line arguments support take one bulk string with spaces... · bfbb15f7
      Binbin authored
      redis-server command line arguments support take one bulk string with spaces for MULTI_ARG configs parsing. And allow options value to use the -- prefix (#10660)
      
      ## Take one bulk string with spaces for MULTI_ARG configs parsing
      Currently redis-server looks for arguments that start with `--`,
      and anything in between them is considered arguments for the config.
      like: `src/redis-server --shutdown-on-sigint nosave force now --port 6380`
      
      MULTI_ARG configs behave differently for CONFIG command, vs the command
      line argument for redis-server.
      i.e. CONFIG command takes one bulk string with spaces in it, while the
      command line takes an argv array with multiple values.
      
      In this PR, in config.c, if `argc > 1` we can take them as is,
      and if the config is a `MULTI_ARG` and `argc == 1`, we will split it by spaces.
      
      So both of these will be the same:
      ```
      redis-server --shutdown-on-sigint nosave force now --shutdown-on-sigterm nosave force
      redis-server --shutdown-on-sigint nosave "force now" --shutdown-on-sigterm nosave force
      redis-server --shutdown-on-sigint nosave "force now" --shutdown-on-sigterm "nosave force"
      ```
      
      ## Allow options value to use the `--` prefix
      Currently it decides to switch to the next config, as soon as it sees `--`, 
      even if there was not a single value provided yet to the last config,
      this makes it impossible to define a config value that has `--` prefix in it.
      
      For instance, if we want to set the logfile to `--my--log--file`,
      like `redis-server --logfile --my--log--file --loglevel verbose`,
      current code will handle that incorrectly.
      
      In this PR, now we allow a config value that has `--` prefix in it.
      **But note that** something like `redis-server --some-config --config-value1 --config-value2 --loglevel debug`
      would not work, because if you want to pass a value to a config starting with `--`, it can only be a single value.
      like: `redis-server --some-config "--config-value1 --config-value2" --loglevel debug`
      
      An example (using `--` prefix config value):
      ```
      redis-server --logfile --my--log--file --loglevel verbose
      redis-cli config get logfile loglevel
      1) "loglevel"
      2) "verbose"
      3) "logfile"
      4) "--my--log--file"
      ```
      
      ### Potentially breaking change
      `redis-server --save --loglevel verbose` used to work the same as `redis-server --save "" --loglevel verbose`
      now, it'll error!
      bfbb15f7
    • Binbin's avatar
      FLUSHDB and FLUSHALL add call forceCommandPropagation / FLUSHALL reset dirty... · 783b210d
      Binbin authored
      FLUSHDB and FLUSHALL add call forceCommandPropagation / FLUSHALL reset dirty counter to 0 if we enable save (#10691)
      
      ## FLUSHALL
      We used to restore the dirty counter after `rdbSave` zeroed it if we enable save.
      Otherwise FLUSHALL will not be replicated nor put into the AOF.
      
      And then we do increment it again below.
      Without that extra dirty++, when db was already empty, FLUSHALL
      will not be replicated nor put into the AOF.
      
      We now gonna replace all that dirty counter magic with a call
      to forceCommandPropagation (REPL and AOF), instead of all the
      messing around with the dirty counter.
      Added tests to cover three part (dirty counter, REPL, AOF).
      
      One benefit other than cleaner code is that the `rdb_changes_since_last_save` is correct in this case.
      
      ## FLUSHDB
      FLUSHDB was not replicated nor put into the AOF when db was already empty.
      Unlike DEL on a non-existing key, FLUSHDB always does something, and that's to call the module hook. 
      So basically FLUSHDB is never a NOP, and thus it should always be propagated.
      Not doing that, could mean that if a module does something in that hook, and wants to
      avoid issues of that hook being missing on the replica if the db is empty, it'll need to do complicated things.
      
      So now FLUSHDB add call forceCommandPropagation, we will always propagate FLUSHDB.
      Always propagating FLUSHDB seems like a safe approach that shouldn't have any drawbacks (other than looking odd)
      
      This was mentioned in #8972
      
      ## Test section:
      We actually found it while solving a race condition in the BGSAVE test (other.tcl).
      It was found in extra_ci Daily Arm64 (test-libc-malloc).
      ```
      [exception]: Executing test client: ERR Background save already in progress.
      ERR Background save already in progress
      ```
      
      It look like `r flushdb` trigger (schedule) a bgsave right after `waitForBgsave r` and before `r save`.
      Changing flushdb to flushall, FLUSHALL will do a foreground save and then set the dirty counter to 0.
      783b210d
  8. 10 May, 2022 4 commits
    • guybe7's avatar
      Dediacted member to hold RedisModuleCommand (#10681) · 815a6f84
      guybe7 authored
      Fix #10552
      
      We no longer piggyback getkeys_proc to hold the RedisModuleCommand struct, when exists
      
      Others:
      Use `doesCommandHaveKeys` in `RM_GetCommandKeysWithFlags` and `getKeysSubcommandImpl`.
      It causes a very minor behavioral change in commands that don't have actual keys, but have a spec
      with `CMD_KEY_NOT_KEY`.
      For example, before this command `COMMAND GETKEYS SPUBLISH` would return
      `Invalid arguments specified for command` but not it returns `The command has no key arguments`
      815a6f84
    • Mariya Markova's avatar
      Replace float zero comparison to FP_ZERO comparison (#10675) · c2d8d4e6
      Mariya Markova authored
      I suggest to use "[fpclassify](https://en.cppreference.com/w/cpp/numeric/math/fpclassify)" for float
      comparison with zero, because of expression "value == 0" with value very close to zero can be
      considered as true with some performance compiler optimizations.
      
      Note: this code was introduced by 9d520a7f to accept zset scores that get ERANGE in conversion
      due to precision loss near 0.
      But with Intel compilers, ICC and ICX, where optimizations for 0 check are more aggressive, "==0" is
      true for mentioned functions, however should not be. Behavior is seen starting from O2.
      This leads to a failure in the ZSCAN test in scan.tcl
      c2d8d4e6
    • Binbin's avatar
      CLUSTER SHARDS should returns slots as integers, not strings (#10683) · 2a1ea8c7
      Binbin authored
      It used to returns slots as strings, like:
      ```
      redis> cluster shards
      1) 1) "slots"
         2) 1) "10923"
            2) "16383"
      ```
      
      CLUSTER SHARDS docs and the top comment of #10293 says that it returns integers.
      Note other commands like CLUSTER SLOTS, it returns slots as integers.
      Use addReplyLongLong instead of addReplyBulkLongLong, now it returns slots as integers:
      ```
      redis> cluster shards
      1) 1) "slots"
         2) 1) (integer) 10923
            2) (integer) 16383
      ```
      
      This is a small breaking change, introduced in 7.0.0 (7.0 RC3, #10293)
      
      Fixes #10680
      2a1ea8c7
    • Meir Shpilraien (Spielrein)'s avatar
      Fix #10705, avoid relinking the same library twice. (#10706) · 442e73ea
      Meir Shpilraien (Spielrein) authored
      Set `old_li` to NULL to avoid linking it again on error.
      Before the fix, loading an already existing library will cause the existing library to be added again. This cause not harm other then wrong statistics. The statistics that are effected  by the issue are:
      * `libraries_count` and `functions_count` returned by `function stats` command
      * `used_memory_functions` returned on `info memory` command
      * `functions.caches` returned on `memory stats` command
      442e73ea
  9. 09 May, 2022 5 commits
  10. 08 May, 2022 2 commits
  11. 04 May, 2022 1 commit
  12. 03 May, 2022 2 commits
  13. 02 May, 2022 3 commits
  14. 01 May, 2022 1 commit
  15. 28 Apr, 2022 3 commits
  16. 27 Apr, 2022 2 commits
    • Oran Agra's avatar
      Merge pull request #10651 from oranagra/meir_lua_readonly_tables · 89772ed8
      Oran Agra authored
      # Lua readonly tables
      The PR adds support for readonly tables on Lua to prevent security vulnerabilities:
      * (CVE-2022-24736) An attacker attempting to load a specially crafted Lua script
        can cause NULL pointer dereference which will result with a crash of the
        redis-server process. This issue affects all versions of Redis.
      * (CVE-2022-24735) By exploiting weaknesses in the Lua script execution
        environment, an attacker with access to Redis can inject Lua code that will
        execute with the (potentially higher) privileges of another Redis user.
      
      The PR is spitted into 4 commits.
      
      ### Change Lua to support readonly tables
      
      This PR modifies the Lua interpreter code to support a new flag on tables. The new flag indicating that the table is readonly and any attempt to perform any writes on such a table will result in an error. The new feature can be turned off and on using the new `lua_enablereadonlytable` Lua API. The new API can be used **only** from C code. Changes to support this feature was taken from https://luau-lang.org/
      
      ### Change eval script to set user code on Lua registry
      
      Today, Redis wrap the user Lua code with a Lua function. For example, assuming the user code is:
      
      ```
      return redis.call('ping')
      ```
      
      The actual code that would have sent to the Lua interpreter was:
      
      ```
      f_b3a02c833904802db9c34a3cf1292eee3246df3c() return redis.call('ping') end
      ```
      
      The warped code would have been saved on the global dictionary with the following name: `f_<script sha>` (in our example `f_b3a02c833904802db9c34a3cf1292eee3246df3c`). This approach allows one user to easily override the implementation of another user code, example:
      
      ```
      f_b3a02c833904802db9c34a3cf1292eee3246df3c = function() return 'hacked' end
      ```
      
      Running the above code will cause `evalsha b3a02c833904802db9c34a3cf1292eee3246df3c 0` to return `hacked` although it should have returned `pong`. Another disadvantage is that Redis basically runs code on the loading (compiling) phase without been aware of it. User can do code injection like this:
      
      ```
      return 1 end <run code on compling phase> function() return 1
      ```
      
      The warped code will look like this and the entire `<run code on compiling phase>` block will run outside of eval or evalsha context:
      
      ```
      f_<sha>() return 1 end <run code on compling phase> function() return 1 end
      ```
      
      The commits puts the user code on a special Lua table called the registry. This table is not accessible to the user so it can not be manipulated by him. Also there is no longer a need to warp the user code so there is no risk in code injection which will cause running code in the wrong context.
      
      ### Use `lua_enablereadonlytable` to protect global tables on eval and function
      
      The commit uses the new `lua_enablereadonlytable` Lua API to protect the global tables of both evals scripts and functions. For eval scripts, the implementation is easy, We simply call `lua_enablereadonlytable` on the global table to turn it into a readonly table.
      
      On functions its more complected, we want to be able to switch globals between load run and function run. To achieve this, we create a new empty table that acts as the globals table for function, we control the actual globals using metatable manipulations. Notice that even if the user gets a pointer to the original tables, all the tables are set to be readonly (using `lua_enablereadonlytable` Lua API) so he can not change them. The following better explains the solution:
      
      ```
      Global table {} <- global table metatable {.__index = __real_globals__}
      ```
      
      The `__real_globals__` is depends on the run context (function load or function call).
      
      Why is this solution needed and its not enough to simply switch globals? When we run in the context of function load and create our functions, our function gets the current globals that was set when they were created. Replacing the globals after the creation will not effect them. This is why this trick it mandatory.
      
      ### Protect the rest of the global API and add an allowed list to the provided API
      
      The allowed list is done by setting a metatable on the global table before initialising any library. The metatable set the `__newindex` field to a function that check the allowed list before adding the field to the table. Fields which is not on the
      allowed list are simply ignored.
      
      After initialisation phase is done we protect the global table and each table that might be reachable from the global table. For each table we also protect the table metatable if exists.
      
      ### Performance
      
      Performance tests was done on a private computer and its only purpose is to show that this fix is not causing any performance regression.
      
      case 1: `return redis.call('ping')`
      case 2: `for i=1,10000000 do redis.call('ping') end`
      
      |                             | Unstable eval | Unstable function | lua_readonly_tables eval | lua_readonly_tables function |
      |-----------------------------|---------------|-------------------|--------------------------|------------------------------|
      | case1 ops/sec               | 235904.70     | 236406.62         | 232180.16               | 230574.14                   |
      | case1 avg latency ms        | 0.175         | 0.164             | 0.178                    | 0.149                        |
      | case2 total time in seconds | 3.373         | 3.444s            | 3.268                   | 3.278                        |
      
      ### Breaking changes
      
      * `print` function was removed from Lua because it can potentially cause the Redis processes to get stuck (if no one reads from stdout). Users should use redis.log. An alternative is to override the `print` implementation and print the message to the log file.
      
      All the work by @MeirShpilraien, i'm just publishing it.
      89772ed8
    • chenyang8094's avatar
      Delete renamed new incr when write manifest failed (#10649) · e24d4600
      chenyang8094 authored
      Followup fix for #10616
      e24d4600
  17. 26 Apr, 2022 3 commits
    • meir's avatar
      Protect any table which is reachable from globals and added globals white list. · efa162bc
      meir authored
      The white list is done by setting a metatable on the global table before initializing
      any library. The metatable set the `__newindex` field to a function that check
      the white list before adding the field to the table. Fields which is not on the
      white list are simply ignored.
      
      After initialization phase is done we protect the global table and each table
      that might be reachable from the global table. For each table we also protect
      the table metatable if exists.
      efa162bc
    • meir's avatar
      Protect globals of both evals scripts and functions. · 3731580b
      meir authored
      Use the new `lua_enablereadonlytable` Lua API to protect the global tables of
      both evals scripts and functions. For eval scripts, the implemetation is easy,
      We simply call `lua_enablereadonlytable` on the global table to turn it into
      a readonly table.
      
      On functions its more complecated, we want to be able to switch globals between
      load run and function run. To achieve this, we create a new empty table that
      acts as the globals table for function, we control the actual globals using metatable
      manipulation. Notice that even if the user gets a pointer to the original tables, all
      the tables are set to be readonly (using `lua_enablereadonlytable` Lua API) so he can
      not change them. The following inlustration better explain the solution:
      
      ```
      Global table {} <- global table metatable {.__index = __real_globals__}
      ```
      
      The `__real_globals__` is set depends on the run context (function load or function call).
      
      Why this solution is needed and its not enough to simply switch globals?
      When we run in the context of function load and create our functions, our function gets
      the current globals that was set when they were created. Replacing the globals after
      the creation will not effect them. This is why this trick it mandatory.
      3731580b
    • meir's avatar
      Move user eval function to be located on Lua registry. · 992f9e23
      meir authored
      Today, Redis wrap the user Lua code with a Lua function.
      For example, assuming the user code is:
      
      ```
      return redis.call('ping')
      ```
      
      The actual code that would have sent to the Lua interpreter was:
      
      ```
      f_b3a02c833904802db9c34a3cf1292eee3246df3c() return redis.call('ping') end
      ```
      
      The wraped code would have been saved on the global dictionary with the
      following name: `f_<script sha>` (in our example `f_b3a02c833904802db9c34a3cf1292eee3246df3c`).
      
      This approach allows one user to easily override the implementation a another user code, example:
      
      ```
      f_b3a02c833904802db9c34a3cf1292eee3246df3c = function() return 'hacked' end
      ```
      
      Running the above code will cause `evalsha b3a02c833904802db9c34a3cf1292eee3246df3c 0` to return
      hacked although it should have returned `pong`.
      
      Another disadventage is that Redis basically runs code on the loading (compiling) phase without been
      aware of it. User can do code injection like this:
      
      ```
      return 1 end <run code on compling phase> function() return 1
      ```
      
      The wraped code will look like this and the entire `<run code on compling phase>` block will run outside
      of eval or evalsha context:
      
      ```
      f_<sha>() return 1 end <run code on compling phase> function() return 1 end
      ```
      992f9e23