CVE-2022-33105: Release 7.0.1 by oranagra · Pull Request #10829 · redis/redis

Conversation

add a comment to `container` in `quicklist.h`. Because `PLAIN` and `PACKED` are not as easy to understand as `NONE` and `LISTPACK` and we don’t have a detailed comment on it.

Co-authored-by: Oran Agra [email protected]

`the redis object pointer was populated.` -> `the sds pointer was populated.` We don’t populate the redis object pointer in this function.

Till now, on MacOS we only used to enable SO_KEEPALIVE, but we didn’t set the interval which is configurable via the `tcp-keepalive` config. This adds support for that on MacOS, to match what we already do on Linux.

update macros ZIPLIST_ENTRY_END i think the right definition is ((zl)+intrev32ifbe(ZIPLIST_BYTES(zl))-ZIPLIST_END_SIZE)

…#10663)

When user uses the same input key for SDIFF as the first one, the result must be empty, so we don’t need to process the elements to test.

This method is like the one done in zset‘s `zsetChooseDiffAlgorithm`

Co-authored-by: Oran Agra [email protected]

In general, our error handler make sure the error object is always a table. In some rare cases (such as OOM error), the error handler will not be called and the error object will be a string. The PR expose the error even if its a string and not a table.

Currently there is no way to test it but if it’ll ever happen, it is better to propagate this string upwards than just generate a generic error without any specific info.

* Module API doc script: Mark unreleased API functions

* fix broken quotes in generate-module-api-doc.rb

Co-authored-by: Oran Agra [email protected]

)

this seems to have been an oversight. syncing the flags so that NOTIFY_NEW is available to modules. missing in redis#10512

also fixing already defined constants build warning while at it.

Co-authored-by: Oran Agra [email protected]

Fixed RM_Scan() usage example: `RedisModuleCursor` -> `RedisModuleScanCursor`

…edis#10661)

If we want to support bits that can be overlapping, we need to make sure that:

1. we don’t use the same bit for two return values.
2. values should be sorted so that prefer ones (matching more bits) come first.

Unintentional change in redis#9644 (since RC1) meant that an empty `–save ""` config from command line, wouldn’t have clear any setting from the config file

Added tests to cover that, and improved test infra to take additional command line args for redis-server

fix some typo in "t_zset.c".

1. `zzlisinlexrange` the function name mentioned in the comment is misspelled.
2. fix typo in function name`zarndmemberReplyWithListpack` -> `zrandmemberReplyWithListpack`

Changed cursor’s type from `int` to `unsigned long` allows handling database or key with more than 2^31 elements

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

…0683)

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 redis#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, redis#10293)

Fixes redis#10680

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 9d520a7 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

Fix redis#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`

…t dirty counter to 0 if we enable save (redis#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 redis#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.

… spaces for MULTI_ARG configs parsing. And allow options value to use the – prefix (redis#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!

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.

…G flag for volatile configurations. (redis#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.

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)

…10645)

Updated the comments for: info command lmpopCommand and blmpopCommand sinterGenericCommand

Fix the missing “key” words in the srandmemberCommand function For LPOS command, when rank is 0, prompt user that rank could be positive number or negative number, and add a test for it

Alias was mistakenly forgotten when the sub commands introduced as json files.

since the sharded pubsub is different with pubsub, it’s better to give users a hint to make it more clear.

This allows the module to know the usable size of an allocation it made, rather than the consumed size.

…truct (redis#10697)

Move the client flags to a more cache friendly position within the client struct we regain the lost 2% of CPU cycles since v6.2 ( from 630532.57 to 647449.80 ops/sec ). These are due to higher rate of calls to getClientType due to changes in redis#9166 and redis#10020

…M check bug, and new RM_Call checks. (redis#10786)

* Fix broken protocol when redis can’t persist to RDB (general commands, not modules), excessive newline. regression of redis#10372 (7.0 RC3) * Fix broken protocol when Redis can’t persist to AOF (modules and scripts), missing newline. * Fix bug in OOM check of EVAL scripts called from RM_Call. set the cached OOM state for scripts before executing module commands too, so that it can serve scripts that are executed by modules. i.e. in the past EVAL executed by RM_Call could have either falsely fail or falsely succeeded because of a wrong cached OOM state flag. * Fix bugs with RM_Yield:

1. SHUTDOWN should only accept the NOSAVE mode
2. Avoid eviction during yield command processing.
3. Avoid processing master client commands while yielding from another client * Add new two more checks to RM_Call script mode.
4. READONLY You can’t write against a read only replica
5. MASTERDOWN Link with MASTER is down and `replica-serve-stale-data` is set to `no` * Add new RM_Call flag to let redis automatically refuse `deny-oom` commands while over the memory limit. * Add tests to cover various errors from Scripts, Modules, Modules calling scripts, and Modules calling commands in script mode.

Add tests: * Looks like the MISCONF error was completely uncovered by the tests, add tests for it, including from scripts, and modules * Add tests for NOREPLICAS from scripts * Add tests for the various errors in module RM_Call, including RM_Call that calls EVAL, and RM_call in "eval mode". that includes: NOREPLICAS, READONLY, MASTERDOWN, MISCONF

The important part is that read-only scripts (not just EVAL_RO and FCALL_RO, but also ones with `no-writes` executed by normal EVAL or FCALL), will now be permitted to run during CLIENT PAUSE WRITE (unlike before where only the _RO commands would be processed).

Other than that, some errors like OOM, READONLY, MASTERDOWN are now handled by processCommand, rather than the command itself affects the error string (and even error code in some cases), and command stats.

Besides that, now the `may-replicate` commands, PFCOUNT and PUBLISH, will be considered `write` commands in scripts and will be blocked in all read-only scripts just like other write commands. They’ll also be blocked in EVAL_RO (i.e. even for scripts without the `no-writes` shebang flag.

This commit also hides the `may_replicate` flag from the COMMAND command output. this is a **breaking change**.

background about may_replicate: We don’t want to expose a no-may-replicate flag or alike to scripts, since we consider the may-replicate thing an internal concern of redis, that we may some day get rid of. In fact, the may-replicate flag was initially introduced to flag EVAL: since we didn’t know what it’s gonna do ahead of execution, before function-flags existed). PUBLISH and PFCOUNT, both of which because they have side effects which may some day be fixed differently.

code changes: The changes in eval.c are mostly code re-ordering:

• evalCalcFunctionName is extracted out of evalGenericCommand
• evalExtractShebangFlags is extracted luaCreateFunction
• evalGetCommandFlags is new code

Apparently, GCC 11.2.0 has a new fancy warning for misleading indentations. It prints a warning when BRET(b) is on the same line as the loop.

…, and inserting comments around module and acl configs (redis#10761)

A regression from redis#10285 (redis 7.0). CONFIG REWRITE would put lines with: `include`, `rename-command`, `user`, `loadmodule`, and any module specific config in a comment.

For ACL `user`, `loadmodule` and module specific configs would be re-inserted at the end (instead of updating existing lines), so the only implication is a messy config file full of comments.

But for `rename-command` and `include`, the implication would be that they’re now missing, so a server restart would lose them.

Co-authored-by: Oran Agra [email protected]

Skip the print on AOF_NOT_EXIST status.

Redis 7 adds some new alias config like `hash-max-listpack-entries` alias `hash-max-ziplist-entries`.

If a config file contains both real name and alias like this: ``` hash-max-listpack-entries 20 hash-max-ziplist-entries 20 ```

after set `hash-max-listpack-entries` to 100 and `config rewrite`, the config file becomes to: ``` hash-max-listpack-entries 100 hash-max-ziplist-entries 20 ```

we can see that the alias config is not modified, and users will get wrong config after restart.

6.0 and 6.2 doesn’t have this bug, since they only have the `slave` word alias.

Co-authored-by: Oran Agra [email protected]

* Update time independent string compare to use hash length

On line 4068, redis has a logical nodeIsSlave(myself) on the outer if layer, which you can delete without having to repeat the decision

…and instantaneous_output_repl_kbps. (redis#10810)

A supplement to redis#10062 Split `instantaneous_repl_total_kbps` to `instantaneous_input_repl_kbps` and `instantaneous_output_repl_kbps`.

Work:

This PR:

• delete 1 info field:
  • `instantaneous_repl_total_kbps`
• add 2 info fields:
  • `instantaneous_input_repl_kbps / instantaneous_output_repl_kbps`

Result:

• master ``` total_net_input_bytes:26633673 total_net_output_bytes:21716596 total_net_repl_input_bytes:0 total_net_repl_output_bytes:18433052 instantaneous_input_kbps:0.02 instantaneous_output_kbps:0.00 instantaneous_input_repl_kbps:0.00 instantaneous_output_repl_kbps:0.00 ```
• slave ``` total_net_input_bytes:18433212 total_net_output_bytes:94790 total_net_repl_input_bytes:18433052 total_net_repl_output_bytes:0 instantaneous_input_kbps:0.00 instantaneous_output_kbps:0.05 instantaneous_input_repl_kbps:0.00 instantaneous_output_repl_kbps:0.00 ```

Trying to avoid people opening crash report issues about module crashes and ARM QEMU bugs.

Current documentation only states a single GET token is needed which is not true. Marking the command with multiple_token.

Currently generate-command.help.rb dose not handle the multiple_token flag, handle this flag in this PR. The format is the same as redis-cli rendering. ```diff

• bitfield_ro key GET encoding offset [encoding offset …]

• bitfield_ro key GET encoding offset [GET encoding offset …] ```

Re run generate-command-code.py which was forget in redis#10820. Also change the flag value from string to bool, like “true” to true

Correcting the introduction version of the command `BITFIELD_RO` Command added by commit: b3e4abf

Add history info of the `FULL` modifier in `XINFO STREAM` Modifier was added by commit: 1e2aee3

(Includes output from `./utils/generate-command-code.py`)

Currently, we only increment stat_rdb_saves in rdbSaveBackground, we should also increment it in the SAVE command.

We concluded there’s no need to increment when:

1. saving a base file for an AOF
2. when saving an empty rdb file to delete an old one
3. when saving to sockets (not creating a persistence / snapshot file)

The stat counter was introduced in redis#10178

* fix a wrong comment in startSaving

This change fixes failing `integration/logging.tcl` test in Gentoo with musl libc, where `ldd` returns ``` libc.so => /lib/ld-musl-x86_64.so.1 (0x7f9d5f171000) ``` unlike Alpine’s ``` libc.musl-x86_64.so.1 => /lib/ld-musl-x86_64.so.1 (0x7f82cfa16000) ``` The solution is to extend matching pattern introduced in redis#8532.

oranagra changed the base branch from unstable to 7.0

Jun 8, 2022