Commit Graph

1174 Commits

Author SHA1 Message Date
bodong.ybd 0f14d32792 Fix sort_ro get-keys function return wrong key number (#12522)
Before:
```
127.0.0.1:6379> command getkeys sort_ro key
(empty array)
127.0.0.1:6379>
```
After:
```
127.0.0.1:6379> command getkeys sort_ro key
1) "key"
127.0.0.1:6379>
```

(cherry picked from commit b59f53efb3)
2023-09-06 20:55:58 +03:00
zhaozhao.zz 4d67bb6afa do not call handleClientsBlockedOnKeys inside yielding command (#12459)
Fix the assertion when a busy script (timeout) signal ready keys (like LPUSH),
and then an arbitrary client's `allow-busy` command steps into `handleClientsBlockedOnKeys`
try wake up clients blocked on keys (like BLPOP).

Reproduction process:
1. start a redis with aof
    `./redis-server --appendonly yes`
2. exec blpop
    `127.0.0.1:6379> blpop a 0`
3. use another client call a busy script and this script push the blocked key
    `127.0.0.1:6379> eval "redis.call('lpush','a','b') while(1) do end" 0`
4. user a new client call an allow-busy command like auth
    `127.0.0.1:6379> auth a`

BTW, this issue also break the atomicity of script.

This bug has been around for many years, the old versions only have the
atomic problem, only 7.0/7.2 has the assertion problem.

Co-authored-by: Oran Agra <oran@redislabs.com>
(cherry picked from commit 8226f39fb2)
2023-09-06 20:55:58 +03:00
sundb 646069a900 Skip test for sdsRemoveFreeSpace when mem_allocator is not jemalloc (#11878)
Test `trim on SET with big value` (introduced from #11817) fails under mac m1 with libc mem_allocator.
The reason is that malloc(33000) will allocate 65536 bytes(>42000).
This test still passes under ubuntu with libc mem_allocator.

```
*** [err]: trim on SET with big value in tests/unit/type/string.tcl
Expected [r memory usage key] < 42000 (context: type source line 471 file /Users/iospack/data/redis_fork/tests/unit/type/string.tcl cmd {assert {[r memory usage key] < 42000}} proc ::test)
```

simple test under mac m1 with libc mem_allocator:
```c
void *p = zmalloc(33000);
printf("malloc size: %zu\n", zmalloc_size(p));

# output
malloc size: 65536
```

(cherry picked from commit 3fba3ccd96)
2023-09-06 20:55:58 +03:00
Lior Lahav bd1dac0c6e Fix possible crash in command getkeys (#12380)
When getKeysUsingKeySpecs processes a command with more than one key-spec,
and called with a total of more than 256 keys, it'll call getKeysPrepareResult again,
but since numkeys isn't updated, getKeysPrepareResult will not bother to copy key
names from the old result (leaving these slots uninitialized). Furthermore, it did not
consider the keys it already found when allocating more space.

Co-authored-by: Oran Agra <oran@redislabs.com>
(cherry picked from commit b7559d9f3b)
2023-07-10 14:39:42 +03:00
sundb 25f610fc29 Use Reservoir Sampling for random sampling of dict, and fix hang during fork (#12276)
## Issue:
When a dict has a long chain or the length of the chain is longer than
the number of samples, we will never be able to sample the elements
at the end of the chain using dictGetSomeKeys().
This could mean that SRANDMEMBER can be hang in and endless loop.
The most severe case, is the pathological case of when someone uses SCAN+DEL
or SSCAN+SREM creating an unevenly distributed dict.
This was amplified by the recent change in #11692 which prevented a
down-sizing rehashing while there is a fork.

## Solution
1. Before, we will stop sampling when we reach the maximum number
  of samples, even if there is more data after the current chain.
  Now when we reach the maximum we use the Reservoir Sampling
  algorithm to fairly sample the end of the chain that cannot be sampled
2. Fix the rehashing code, so that the same as it allows rehashing for up-sizing
  during fork when the ratio is extreme, it will allow it for down-sizing as well.

Issue was introduced (or became more severe) by #11692

Co-authored-by: Oran Agra <oran@redislabs.com>
(cherry picked from commit b00a235186)
2023-07-10 14:39:42 +03:00
Oran Agra 2ba8de9d5e Fix WAIT for clients being blocked in a module command (#12220)
So far clients being blocked and unblocked by a module command would
update the c->woff variable and so WAIT was ineffective and got released
without waiting for the command actions to propagate.

This seems to have existed since forever, but not for RM_BlockClientOnKeys.

It is problematic though to know if the module did or didn't propagate
anything in that command, so for now, instead of adding an API, we'll
just update the woff to the latest offset when unblocking, this will
cause the client to possibly wait excessively, but that's not that bad.

(cherry picked from commit 6117f28822)
2023-07-10 14:39:42 +03:00
Oran Agra f6a7c9f9ec Lua cjson and cmsgpack integer overflow issues (CVE-2022-24834)
* Fix integer overflows due to using wrong integer size.
* Add assertions / panic when overflow still happens.
* Deletion of dead code to avoid need to maintain it
* Some changes are not because of bugs, but rather paranoia.
* Improve cmsgpack and cjson test coverage.

Co-authored-by: Yossi Gottlieb <yossigo@gmail.com>
2023-07-10 14:39:42 +03:00
Oran Agra 6b17d824c5 fix false valgrind error on new hash test (#11200)
New test fails on valgrind because strtold("+inf") with valgrind returns a non-inf result
same thing is done in incr.tcl.

(cherry picked from commit c3b7bde914)
2023-04-17 15:54:03 +03:00
Slava Koyfman 90f489b0a4 Disconnect pub-sub subscribers when revoking `allchannels` permission (#11992)
The existing logic for killing pub-sub clients did not handle the `allchannels`
permission correctly. For example, if you:

    ACL SETUSER foo allchannels

Have a client authenticate as the user `foo` and subscribe to a channel, and then:

    ACL SETUSER foo resetchannels

The subscribed client would not be disconnected, though new clients under that user
would be blocked from subscribing to any channels.

This was caused by an incomplete optimization in `ACLKillPubsubClientsIfNeeded`
checking whether the new channel permissions were a strict superset of the old ones.

(cherry picked from commit f38aa6bfb7)
2023-04-17 15:54:03 +03:00
chendianqiang 1c1bd618c9 fix hincrbyfloat not to create a key if the new value is invalid (#11149)
Check the validity of the value before performing the create operation,
prevents new data from being generated even if the request fails to execute.

Co-authored-by: Oran Agra <oran@redislabs.com>
Co-authored-by: chendianqiang <chendianqiang@meituan.com>
Co-authored-by: Binbin <binloveplay1314@qq.com>
(cherry picked from commit bc7fe41e58)
2023-04-17 15:54:03 +03:00
Oran Agra 6956d15b77 Avoid assertion when MSETNX is used with the same key twice (CVE-2023-28425)
Using the same key twice in MSETNX command would trigger an assertion.

This reverts #11594 (introduced in Redis 7.0.8)
2023-03-20 19:16:50 +02:00
Binbin f8ae7a414c Fix the bug that CLIENT REPLY OFF|SKIP cannot receive push notifications (#11875)
This bug seems to be there forever, CLIENT REPLY OFF|SKIP will
mark the client with CLIENT_REPLY_OFF or CLIENT_REPLY_SKIP flags.
With these flags, prepareClientToWrite called by addReply* will
return C_ERR directly. So the client can't receive the Pub/Sub
messages and any other push notifications, e.g client side tracking.

In this PR, we adding a CLIENT_PUSHING flag, disables the reply
silencing flags. When adding push replies, set the flag, after the reply,
clear the flag. Then add the flag check in prepareClientToWrite.

Fixes #11874

Note, the SUBSCRIBE command response is a bit awkward,
see https://github.com/redis/redis-doc/pull/2327

Co-authored-by: Oran Agra <oran@redislabs.com>
(cherry picked from commit 416842e6c0)
2023-03-20 19:16:50 +02:00
Ozan Tezcan a39032214d Ignore RM_Call deny-oom flag if maxmemory is zero (#11319)
If a command gets an OOM response and then if we set maxmemory to zero
to disable the limit, server.pre_command_oom_state never gets updated
and it stays true. As RM_Call() calls with "respect deny-oom" flag checks
server.pre_command_oom_state, all calls will fail with OOM.

Added server.maxmemory check in RM_Call() to process deny-oom flag
only if maxmemory is configured.

(cherry picked from commit 18920813a9)
2023-03-20 19:16:50 +02:00
Oran Agra 2a2a582e7c Integer Overflow in RAND commands can lead to assertion (CVE-2023-25155)
Issue happens when passing a negative long value that greater than
the max positive value that the long can store.
2023-02-28 18:32:34 +02:00
Tom Levy 0825552565 String pattern matching had exponential time complexity on pathological patterns (CVE-2022-36021)
Authenticated users can use string matching commands with a
specially crafted pattern to trigger a denial-of-service attack on Redis,
causing it to hang and consume 100% CPU time.
2023-02-28 18:32:34 +02:00
ranshid 7091b495a0 Fix possible memory corruption in FLUSHALL when a client watches more than one key (#11854)
Avoid calling unwatchAllKeys when running touchAllWatchedKeysInDb (which was unnecessary)
This can potentially lead to use-after-free and memory corruption when the next entry
pointer held by the watched keys iterator is freed when unwatching all keys of a specific client.
found with address sanitizer, added a test which will not always fail (depending on the random
dict hashing seed)
problem introduced in #9829 (Reids 7.0)

Co-authored-by: Oran Agra <oran@redislabs.com>
(cherry picked from commit 18017df7c1)
2023-02-28 18:32:34 +02:00
Madelyn Olson ddf984d058 Prevent Redis from crashing from key tracking invalidations (#11814) 2023-02-28 18:32:34 +02:00
judeng feb796d312 add test case and comments for active expiry in the writeable replica (#11789)
This test case is to cover a edge scenario: when a writable replica enabled AOF
at the same time, active expiry keys which was created in writable replicas should
propagate to the AOF file, and some versions might crash (fixed by #11615).
For details, please refer to #11778

(cherry picked from commit 40659c3424)
2023-02-28 18:32:34 +02:00
guybe7 2db720591b SCAN/RANDOMKEY and lazy-expire (#11788)
Starting from Redis 7.0 (#9890) we started wrapping everything a command
 propagates with MULTI/EXEC. The problem is that both SCAN and RANDOMKEY can
lazy-expire arbitrary keys (similar behavior to active-expire), and put DELs in a transaction.

Fix: When these commands are called without a parent exec-unit (e.g. not in EVAL or
MULTI) we avoid wrapping their DELs in a transaction (for the same reasons active-expire
and eviction avoids a transaction)

This PR adds a per-command flag that indicates that the command may touch arbitrary
keys (not the ones in the arguments), and uses that flag to avoid the MULTI-EXEC.
For now, this flag is internal, since we're considering other solutions for the future.

Note for cluster mode: if SCAN/RANDOMKEY is inside EVAL/MULTI it can still cause the
same situation (as it always did), but it won't cause a CROSSSLOT because replicas and AOF
do not perform slot checks.
The problem with the above is mainly for 3rd party ecosystem tools that propagate commands
from master to master, or feed an AOF file with redis-cli into a master.
This PR aims to fix the regression in redis 7.0, and we opened #11792 to try to handle the
bigger problem with lazy expire better for another release.

(cherry picked from commit fd82bccd0e)
2023-02-28 18:32:34 +02:00
Harkrishn Patro ca0b6caeed Propagate message to a node only if the cluster link is healthy. (#11752)
Currently while a sharded pubsub message publish tries to propagate the message across the cluster, a NULL check is missing for clusterLink. clusterLink could be NULL if the link is causing memory beyond the set threshold cluster-link-sendbuf-limit and server terminates the link.

This change introduces two things:

Avoids the engine crashes on the publishing node if a message is tried to be sent to a node and the link is NULL.
Adds a debugging tool CLUSTERLINK KILL to terminate the clusterLink between two nodes.

(cherry picked from commit fd3975684a)
2023-02-28 18:32:34 +02:00
uriyage af80a4a554 Optimization: sdsRemoveFreeSpace to avoid realloc on noop (#11766)
In #7875 (Redis 6.2), we changed the sds alloc to be the usable allocation
size in order to:

> reduce the need for realloc calls by making the sds implicitly take over
the internal fragmentation

This change was done most sds functions, excluding `sdsRemoveFreeSpace` and
`sdsResize`, the reason is that in some places (e.g. clientsCronResizeQueryBuffer)
we call sdsRemoveFreeSpace when we see excessive free space and want to trim it.
so if we don't trim it exactly to size, the caller may still see excessive free space and
call it again and again.

However, this resulted in some excessive calls to realloc, even when there's no need
and it's gonna be a no-op (e.g. when reducing 15 bytes allocation to 13).

It turns out that a call for realloc with jemalloc can be expensive even if it ends up
doing nothing, so this PR adds a check using `je_nallocx`, which is cheap to avoid
the call for realloc.

in addition to that this PR unifies sdsResize and sdsRemoveFreeSpace into common
code. the difference between them was that sdsResize would avoid using SDS_TYPE_5,
since it want to keep the string ready to be resized again, while sdsRemoveFreeSpace
would permit using SDS_TYPE_5 and get an optimal memory consumption.
now both methods take a `would_regrow` argument that makes it more explicit.

the only actual impact of that is that in clientsCronResizeQueryBuffer we call both sdsResize
and sdsRemoveFreeSpace for in different cases, and we now prevent the use of SDS_TYPE_5 in both.

The new test that was added to cover this concern used to pass before this PR as well,
this PR is just a performance optimization and cleanup.

Benchmark:
`redis-benchmark -c 100 -t set  -d 512 -P 10  -n  100000000`
on i7-9850H with jemalloc, shows improvement from 1021k ops/sec to 1067k (average of 3 runs).
some 4.5% improvement.

Co-authored-by: Oran Agra <oran@redislabs.com>
(cherry picked from commit 46393f9819)
2023-02-28 18:32:34 +02:00
Wen Hui 4d5a4e4b36 Fix command BITFIELD_RO and BITFIELD argument json file, add some test cases for them (#11445)
According to the source code, the commands can be executed with only key name,
and no GET/SET/INCR operation arguments.
change the docs to reflect that by marking these arguments as optional.
also add tests.

(cherry picked from commit fea9bbbe0f)
2023-02-28 18:32:34 +02:00
Oran Agra 3f1f02034c Fix range issues in ZRANDMEMBER and HRANDFIELD (CVE-2023-22458)
missing range check in ZRANDMEMBER and HRANDIFLD leading to panic due
to protocol limitations
2023-01-16 18:40:35 +02:00
Oran Agra 6c25c6b7da Avoid integer overflows in SETRANGE and SORT (CVE-2022-35977)
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.
2023-01-16 18:40:35 +02:00
Oran Agra 4537830ea1 Obuf limit, exit during loop in *RAND* commands and KEYS
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).
2023-01-16 18:40:35 +02:00
Gabi Ganam 574a49b96c Blocking command with a 0.001 seconds timeout blocks indefinitely (#11688)
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 eef29b68a2)
2023-01-16 18:40:35 +02:00
Oran Agra 61a1d4540d Fix potential issue with Lua argv caching, module command filter and libc realloc (#11652)
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 c8052122a2)
2023-01-16 18:40:35 +02:00
filipe oliveira 41bff94b43 Fixed small distance replies on GEODIST and GEO commands WITHDIST (#11631)
Fixes a regression introduced by #11552 in 7.0.6.
it causes replies in the GEO commands to contain garbage when the
result is a very small distance (less than 1)
Includes test to confirm indeed with junk in buffer now we properly reply

(cherry picked from commit d7b4c9175e)
2022-12-16 12:52:57 +02:00
Binbin 3c525fab6a Fix redis-cli cluster add-node race in cli.tcl (#11349)
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 a549b78c48)
2022-12-12 17:36:34 +02:00
ranshid e1557e6c7a fix test Migrate the last slot away from a node using redis-cli (#11221)
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 c0ce97facc)
2022-12-12 17:36:34 +02:00
Binbin b5784feabd Fix timing issue in cluster test (#11008)
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 5ce64ab010)
2022-12-12 17:36:34 +02:00
Binbin a43c51b297 Fix CLUSTERDOWN issue in cluster reshard unblock test (#11139)
change the cluster-node-timeout from 1 to 1000

(cherry picked from commit 3a16ad30b7)
2022-12-12 17:36:34 +02:00
Harkrishn Patro 05c9378b3f Optimize client memory usage tracking operation while client eviction is disabled (#11348)
## 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: Oran Agra <oran@redislabs.com>
(cherry picked from commit c0267b3fa5)
2022-12-12 17:36:34 +02:00
filipe oliveira 6a6d8806a4 Reintroduce lua argument cache in luaRedisGenericCommand removed in v7.0 (#11541)
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 48c49c4
and 4f68655, 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: Oran Agra <oran@redislabs.com>
Co-authored-by: sundb <sundbcn@gmail.com>
(cherry picked from commit 2d80cd7840)
2022-12-12 17:36:34 +02:00
filipe oliveira 77570e3965 Speedup GEODIST with fixedpoint_d2string as an optimized version of snprintf %.4f (#11552)
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: Oran Agra <oran@redislabs.com>
(cherry picked from commit 61c85a2b20)
2022-12-12 17:36:34 +02:00
DevineLiu 81e72e8e29 [BUG] Fix announced ports not updating on local node when updated at runtime (#10745)
The cluster-announce-port/cluster-announce-bus-port/cluster-announce-tls-port should take effect at runtime

Co-authored-by: Madelyn Olson <madelyneolson@gmail.com>
(cherry picked from commit 25ffa79b64)
2022-12-12 17:36:34 +02:00
Madelyn Olson a26ac7ebbc Explicitly send function commands to monitor (#11510)
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 d136bf2830)
2022-12-12 17:36:34 +02:00
uriyage 7ad786db2e Module CLIENT_CHANGE, Fix crash on free blocked client with DB!=0 (#11500)
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: Madelyn Olson <34459052+madolson@users.noreply.github.com>
Co-authored-by: Binbin <binloveplay1314@qq.com>
(cherry picked from commit e4eb18b303)
2022-12-12 17:36:34 +02:00
sundb 9fc20f4f9d Fix crash due to to reuse iterator entry after list deletion in module (#11383)
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: Viktor Söderqvist <viktor@zuiderkwast.se>
(cherry picked from commit 6dd213558b)
2022-12-12 17:36:34 +02:00
C Charles f95af7785b 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 9ab873d9d3)
2022-12-12 17:36:34 +02:00
Meir Shpilraien (Spielrein) 6cf24fa437 `RedisModule_ResetDataset` should not clear the functions. (#11268)
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 d2ad01ab3e)
2022-12-12 17:36:34 +02:00
guybe7 5b2119c639 RM_CreateCommand should not set CMD_KEY_VARIABLE_FLAGS automatically (#11320)
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 3330ea1864)
2022-12-12 17:36:34 +02:00
Shaya Potter b8fcd322c6 Add RM_SetContextUser to support acl validation in RM_Call (and scripts) (#10966)
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 6e993a5dfa)
2022-12-12 17:36:34 +02:00
Oran Agra fa6815e14e Fix heap overflow vulnerability in XAUTOCLAIM (CVE-2022-35951)
Executing an XAUTOCLAIM command on a stream key in a specific state, with a
specially crafted COUNT argument may cause an integer overflow, a subsequent
heap overflow, and potentially lead to remote code execution.
The problem affects Redis versions 7.0.0 or newer.
2022-09-21 22:42:01 +03:00
Shaya Potter a1ec0caed0 Improve cmd_flags for script/functions in RM_Call (#11159)
When RM_Call was used with `M` (reject OOM), `W` (reject writes),
as well as `S` (rejecting stale or write commands in "Script mode"),
it would have only checked the command flags, but not the declared
script flag in case it's a command that runs a script.

Refactoring: extracts out similar code in server.c's processCommand
to be usable in RM_Call as well.

(cherry picked from commit bed6d759bc)
2022-09-21 22:42:01 +03:00
Shay Fadida c9eabbf9b1 Fix missing sections for INFO ALL with module (#11291)
When using `INFO ALL <section>`, when `section` is a specific module section. 
Redis will not print the additional section(s).

The fix in this case, will search the modules info sections if the user provided additional sections to `ALL`.

Co-authored-by: Oran Agra <oran@redislabs.com>
(cherry picked from commit eedb8b1724)
2022-09-21 22:42:01 +03:00
sundb 01358df34e Fix crash due to delete entry from compress quicklistNode and wrongly split quicklistNode (#11242)
This PR mainly deals with 2 crashes introduced in #9357,
and fix the QUICKLIST-PACKED-THRESHOLD mess in external test mode.

1. Fix crash due to deleting an entry from a compress quicklistNode
   When inserting a large element, we need to create a new quicklistNode first,
   and then delete its previous element, if the node where the deleted element is
   located is compressed, it will cause a crash.
   Now add `dont_compress` to quicklistNode, if we want to use a quicklistNode
   after some operation, we can use this flag like following:

    ```c
    node->dont_compress = 1; /* Prevent to be compressed */
    some_operation(node); /* This operation might try to compress this node */
    some_other_operation(node); /* We can use this node without decompress it */
    node->dont_compress = 0; /* Re-able compression */
    quicklistCompressNode(node);
    ```

   Perhaps in the future, we could just disable the current entry from being
   compressed during the iterator loop, but that would require more work.

2. Fix crash due to wrongly split quicklist
   before #9357, the offset param of _quicklistSplitNode() will not negative.
   For now, when offset is negative, the split extent will be wrong.
   following example:
    ```c
    int orig_start = after ? offset + 1 : 0;
    int orig_extent = after ? -1 : offset;
    int new_start = after ? 0 : offset;
    int new_extent = after ? offset + 1 : -1;
    # offset: -2, after: 1, node->count: 2
    # current wrong range: [-1,-1] [0,-1]
    # correct range: [1,-1] [0, 1]
    ```

   Because only `_quicklistInsert()` splits the quicklistNode and only
   `quicklistInsertAfter()`, `quicklistInsertBefore()` call _quicklistInsert(), 
   so `quicklistReplaceEntry()` and `listTypeInsert()` might occur this crash.
   But the iterator of `listTypeInsert()` is alway from head to tail(iter->offset is
   always positive), so it is not affected.
   The final conclusion is this crash only occur when we insert a large element
   with negative index into a list, that affects `LSET` command and `RM_ListSet`
   module api.
     
3. In external test mode, we need to restore quicklist packed threshold after
   when the end of test.
4. Show `node->count` in quicklistRepr().
5. Add new tcl proc `config_get_set` to support restoring config in tests.

(cherry picked from commit 13d25dd95e)
2022-09-21 22:42:01 +03:00
Oran Agra a2a28b8034 Fix assertion when a key is lazy expired during cluster key migration (#11176)
Redis 7.0 has #9890 which added an assertion when the propagation queue
was not flushed and we got to beforeSleep.
But it turns out that when processCommands calls getNodeByQuery and
decides to reject the command, it can lead to a key that was lazy
expired and is deleted without later flushing the propagation queue.

This change prevents lazy expiry from deleting the key at this stage
(not as part of a command being processed in `call`)

(cherry picked from commit c789fb0aa7)
2022-09-21 22:42:01 +03:00
Binbin fa60cd1369 Fix memory leak in moduleFreeCommand (#11147)
Currently, we call zfree(cmd->args), but the argument array
needs to be freed recursively (there might be sub-args).
Also fixed memory leaks on cmd->tips and cmd->history.

Fixes #11145

(cherry picked from commit fc3956e8f4)
2022-09-21 22:42:01 +03:00
DarrenJiang13 4dfc487f96 fix the client type in trackingInvalidateKey() (#11052)
Fix bug with scripts ignoring client tracking NOLOOP and
send an invalidation message anyway.

(cherry picked from commit 44859a41ee)
2022-09-21 22:42:01 +03:00