Skip to content

Conversation

@sundb
Copy link
Collaborator

@sundb sundb commented Jul 3, 2025

This PR is based on valkey-io/valkey#2109

When we refactored the blocking framework we introduced the client reprocessing infrastructure. In cases the client was blocked on keys, it will attempt to reprocess the command. One challenge was to keep track of the command timeout, since we are reprocessing and do not want to re-register the client with a fresh timeout each time. The solution was to consider the client reprocessing flag when the client is blockedOnKeys:

    if (!(c->flags & CLIENT_REPROCESSING_COMMAND)) {
        /* If the client is re-processing the command, we do not set the timeout
         * because we need to retain the client's original timeout. */
        c->bstate.timeout = timeout;
    }

However, this introduced a new issue. There are cases where the client will consecutive blocking of different types for example:

CLIENT PAUSE 10000 ALL
BZPOPMAX zset 1

would have the client blocked on the zset endlessly if nothing will be written to it.

Credits to @uriyage for locating this with his fuzzer testing

The suggested solution is to only flag the client when it is specifically unblocked on keys.

Signed-off-by: Ran Shidlansik ranshid@amazon.com
Co-authored-by: Ran Shidlansik ranshid@amazon.com
Co-authored-by: Binbin binloveplay1314@qq.com

When we refactored the blocking framework we introduced the client
reprocessing infrastructure. In cases the client was blocked on keys, it
will attempt to reprocess the command. One challenge was to keep track
of the command timeout, since we are reprocessing and do not want to
re-register the client with a fresh timeout each time. The solution was
to consider the client reprocessing flag when the client is
blockedOnKeys:

```
if (!c->flag.reprocessing_command) {
        /* If the client is re-processing the command, we do not set the timeout
         * because we need to retain the client's original timeout. */
        c->bstate->timeout = timeout;
    }
```

However, this introduced a new issue. There are cases where the client
will consecutive blocking of different types for example:
```
CLIENT PAUSE 10000 ALL
BZPOPMAX zset 1
```
would have the client blocked on the zset endlessly if nothing will be
written to it.

**Credits to @uriyage for locating this with his fuzzer testing**

The suggested solution is to only flag the client when it is
specifically unblocked on keys.
@snyk-io
Copy link

snyk-io bot commented Jul 3, 2025

🎉 Snyk checks have passed. No issues have been found so far.

security/snyk check is complete. No issues have been found. (View Details)

license/snyk check is complete. No issues have been found. (View Details)

@github-project-automation github-project-automation bot moved this to Todo in Redis 8.2 Jul 3, 2025
@sundb sundb added the release-notes indication that this issue needs to be mentioned in the release notes label Jul 3, 2025
Copy link
Member

@ShooterIT ShooterIT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but feel the title is too internal, how about Fix missing timeout update when reprocessing command with different block type, you may have better one

@ShooterIT
Copy link
Member

I have a question, does it have a problem?

CLIENT PAUSE 10 ALL
BZPOPMAX zset 1

if we don't have any operation, BZPOPMAX will return after 11s instead of 1s?

@sundb
Copy link
Collaborator Author

sundb commented Jul 7, 2025

I have a question, does it have a problem?

CLIENT PAUSE 10 ALL
BZPOPMAX zset 1

if we don't have any operation, BZPOPMAX will return after 11s instead of 1s?

yes.
i guess you want to ask is CLIENT PAUSE 10000 ALL(10s)

@kaplanben
Copy link

kaplanben commented Jul 21, 2025

Logo
Checkmarx One – Scan Summary & Details313b71d9-641c-4c85-97e3-061cc84a8766

New Issues (5)

Checkmarx found the following issues in this Pull Request

Severity Issue Source File / Package Checkmarx Insight
CRITICAL Buffer_Overflow_Wrong_Buffer_Size /src/redis-cli.c: 3676
detailsThe buffer buf created in /src/redis-cli.c at line 3676 is written to a buffer in /src/sds.c at line 302 by hdrlen, but an error in calculating ...
ID: U45IKYQJxkTIUE8XBO7VPkQk%2FM8%3D
Attack Vector
CRITICAL Buffer_Overflow_Wrong_Buffer_Size /src/redis-cli.c: 3676
detailsThe buffer buf created in /src/redis-cli.c at line 3676 is written to a buffer in /src/sds.c at line 302 by newsh, but an error in calculating t...
ID: IUKQBEyr3rqcqEvkYxv3vc0w5t4%3D
Attack Vector
CRITICAL Buffer_Overflow_Wrong_Buffer_Size /src/redis-cli.c: 3676
detailsThe buffer buf created in /src/redis-cli.c at line 3676 is written to a buffer in /src/sds.c at line 313 by hdrlen, but an error in calculating ...
ID: XRk2hGGeHCb9c7cmhVeeADtKGQk%3D
Attack Vector
CRITICAL Buffer_Overflow_Wrong_Buffer_Size /src/redis-cli.c: 3676
detailsThe buffer buf created in /src/redis-cli.c at line 3676 is written to a buffer in /src/sds.c at line 313 by newsh, but an error in calculating t...
ID: g4WaUOGEOKV94jC0JQRM5PQ0P7k%3D
Attack Vector
MEDIUM Divide_By_Zero /modules/vector-sets/fastjson_test.c: 121
detailsThe application performs an illegal operation in generate_random_string, in /modules/vector-sets/fastjson_test.c. In line 121, the program at...
ID: qiowoZ%2FDUFf8wA3ZCvKY8M0GHks%3D
Attack Vector
Fixed Issues (7)

Great job! The following issues were fixed in this Pull Request

Severity Issue Source File / Package
CRITICAL Buffer_Overflow_Wrong_Buffer_Size /deps/linenoise/linenoise.c: 1200
CRITICAL Buffer_Overflow_Wrong_Buffer_Size /src/redis-cli.c: 3676
CRITICAL Buffer_Overflow_Wrong_Buffer_Size /src/redis-cli.c: 3676
CRITICAL Buffer_Overflow_Wrong_Buffer_Size /src/redis-cli.c: 10588
CRITICAL Buffer_Overflow_Wrong_Buffer_Size /deps/linenoise/linenoise.c: 1166
MEDIUM Divide_By_Zero /src/redis-cli.c: 6037
MEDIUM Divide_By_Zero /deps/jemalloc/src/nstime.c: 149

@sundb sundb merged commit 45c8fcc into redis:unstable Jul 21, 2025
19 checks passed
@github-project-automation github-project-automation bot moved this from Todo to Done in Redis 8.2 Jul 21, 2025
@sundb sundb mentioned this pull request Aug 4, 2025
sundb added a commit that referenced this pull request Aug 4, 2025
This is the General Availability release of Redis Open Source 8.2.

### Major changes compared to 8.0

- Streams - new commands: `XDELEX` and `XACKDEL`; extension to `XADD`
and `XTRIM`
- Bitmap - `BITOP`: new operators: `DIFF`, `DIFF1`, `ANDOR`, and `ONE`
- Query Engine - new SVS-VAMANA vector index type which supports vector
compression
- More than 15 performance and resource utilization improvements
- New metrics: per-slot usage metrics, key size distributions for basic
data types, and more

### Binary distributions

- Alpine and Debian Docker images - https://hub.docker.com/_/redis
- Install using snap - see https://github.com/redis/redis-snap
- Install using brew - see https://github.com/redis/homebrew-redis
- Install using RPM - see https://github.com/redis/redis-rpm
- Install using Debian APT - see https://github.com/redis/redis-debian


### Operating systems we test Redis 8.2 on

- Ubuntu 22.04 (Jammy Jellyfish), 24.04 (Noble Numbat)
- Rocky Linux 8.10, 9.5
- AlmaLinux 8.10, 9.5
- Debian 12 (Bookworm)
- macOS 13 (Ventura), 14 (Sonoma), 15 (Sequoia)

### Security fixes (compared to 8.2-RC1)

- (CVE-2025-32023) Fix out-of-bounds write in `HyperLogLog` commands
- (CVE-2025-48367) Retry accepting other connections even if the
accepted connection reports an error

### New Features (compared to 8.2-RC1)

- #14141 Keyspace notifications - new event types:
  - `OVERWRITTEN` - the value of a key is completely overwritten
  - `TYPE_CHANGED` - key type change

### Bug fixes (compared to 8.2-RC1)

- #14162 Crash when using evport with I/O threads
- #14163 `EVAL` crash when error table is empty
- #14144 Vector sets - RDB format is not compatible with big endian
machines
- #14165 Endless client blocking for blocking commands
- #14164 Prevent `CLIENT UNBLOCK` from unblocking `CLIENT PAUSE`
- #14216 TTL was not removed by the `SET` command
- #14224 `HINCRBYFLOAT` removes field expiration on replica

### Performance and resource utilization improvements (compared to
8.2-RC1)

- #14200 Store iterators on stack instead of on heap
- #14144 Vector set - improve RDB loading / RESTORE speed by storing the
worst link info
- #Q6430 More compression variants for the SVS-VAMANA vector index
- #Q6535 `SHARD_K_RATIO` parameter - favor network latency over accuracy
for KNN vector query in a Redis cluster (unstable feature) (MOD-10359)

### Modules API

- #14051 `RedisModule_Get*`, `RedisModule_Set*` - allow modules to
access Redis configurations
- #14114 `RM_UnsubscribeFromKeyspaceEvents` - unregister a module from
specific keyspace notifications
YaacovHazan pushed a commit to YaacovHazan/redis that referenced this pull request Sep 29, 2025
…14165)

This PR is based on valkey-io/valkey#2109

When we refactored the blocking framework we introduced the client
reprocessing infrastructure. In cases the client was blocked on keys, it
will attempt to reprocess the command. One challenge was to keep track
of the command timeout, since we are reprocessing and do not want to
re-register the client with a fresh timeout each time. The solution was
to consider the client reprocessing flag when the client is
blockedOnKeys:

```c
    if (!(c->flags & CLIENT_REPROCESSING_COMMAND)) {
        /* If the client is re-processing the command, we do not set the timeout
         * because we need to retain the client's original timeout. */
        c->bstate.timeout = timeout;
    }
```

However, this introduced a new issue. There are cases where the client
will consecutive blocking of different types for example:
```
CLIENT PAUSE 10000 ALL
BZPOPMAX zset 1
```
would have the client blocked on the zset endlessly if nothing will be
written to it.

**Credits to @uriyage for locating this with his fuzzer testing**

The suggested solution is to only flag the client when it is
specifically unblocked on keys.

Signed-off-by: Ran Shidlansik <ranshid@amazon.com>
Co-authored-by: Ran Shidlansik <ranshid@amazon.com>
Co-authored-by: Binbin <binloveplay1314@qq.com>
YaacovHazan pushed a commit to YaacovHazan/redis that referenced this pull request Sep 30, 2025
…14165)

This PR is based on valkey-io/valkey#2109

When we refactored the blocking framework we introduced the client
reprocessing infrastructure. In cases the client was blocked on keys, it
will attempt to reprocess the command. One challenge was to keep track
of the command timeout, since we are reprocessing and do not want to
re-register the client with a fresh timeout each time. The solution was
to consider the client reprocessing flag when the client is
blockedOnKeys:

```c
    if (!(c->flags & CLIENT_REPROCESSING_COMMAND)) {
        /* If the client is re-processing the command, we do not set the timeout
         * because we need to retain the client's original timeout. */
        c->bstate.timeout = timeout;
    }
```

However, this introduced a new issue. There are cases where the client
will consecutive blocking of different types for example:
```
CLIENT PAUSE 10000 ALL
BZPOPMAX zset 1
```
would have the client blocked on the zset endlessly if nothing will be
written to it.

**Credits to @uriyage for locating this with his fuzzer testing**

The suggested solution is to only flag the client when it is
specifically unblocked on keys.

Signed-off-by: Ran Shidlansik <ranshid@amazon.com>
Co-authored-by: Ran Shidlansik <ranshid@amazon.com>
Co-authored-by: Binbin <binloveplay1314@qq.com>
@YaacovHazan YaacovHazan moved this from Todo to pending in Redis 7.2 Backport Sep 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release-notes indication that this issue needs to be mentioned in the release notes

Projects

Status: pending
Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants