[backport] Backport sweep for 9.0 - #4226
Conversation
valkeyrie-ops
Bot
added
ai-resolved-conflicts
AI conflict resolution: source PR #4104Avoid offloading writes to IO threads for the slot migration export job while snapshotting. 2 conflicted files Claude Summary
AI-resolved conflicted files Full backport commit diff: commit efe4f502181a.
|
![valkeyrie-bot[bot]](https://github.githubassets.com/images/gravatars/gravatar-user-420.png){kind=avatar}
…ob while snapshotting. (#4104) **Problem Description** Atomic slot migration is failing when io-threads enabled and pipeline requests are ongoing ```== CRITICAL == This slot-import-target is sending an error to its slot-import-source: 'Protocol error: invalid CRLF in request' after processing the command 'set'``` **Proposed Fix** Disable I/O threads offloading for slot migration jobs when snapshotting to prevent query buffer desynchronization during pipelining Signed-off-by: Satheesha Gowda <satheesha.balaji@gmail.com>
When loading slot migration import jobs from the RDB, the start and end slots of each range were not validated. A corrupted or truncated RDB could produce out-of-range slot values or a reversed range (start > end), which would then be used to build migration jobs and slot ranges. Reject any slot import range whose start or end slot is greater than or equal to CLUSTER_SLOTS, or whose start slot is greater than the end slot, logging a warning and aborting the load. Fixes #4222. Signed-off-by: Binbin <binloveplay1314@qq.com>
## Summary - `clusterRDBLoadSlotImport()` accepted a variable-length `job_name` from RDB, but `createSlotImportJob()` always `memcpy`'s `CLUSTER_NAMELEN` (40) bytes. A crafted RDB with a shorter name caused a heap out-of-bounds read during startup. - Reject slot-import records whose `job_name` length is not exactly `CLUSTER_NAMELEN`, matching the existing command-path check. - Add an integration test that crafts a short `job_name` RDB and asserts the server fails closed. Fixes #4207 ## Test plan - [x] `./runtest --single tests/integration/rdb-slot-import.tcl` --------- Signed-off-by: quanyeyang <quanyemostima@gmail.com>
d41fa59 to
c1744a1
Compare
The cross-DB `COPY` guard in `getNodeByQuery()` (added in #1671) checks `margc >= 4` but then dereferences `margv[4]`: ```c if (mcmd->proc == copyCommand && margc >= 4 && !strcasecmp(objectGetVal(margv[3]), "db")) { long long value; if (getLongLongFromObject(margv[4], &value) != C_OK || ... ``` `COPY` has arity `-3`, so `COPY k1 k2 DB` (`argc == 4`) passes the arity check in `processCommand()` and reaches `getNodeByQuery()` — which runs *before* `copyCommand()`'s own parser would reject it as a syntax error. The read is one past the end of the argument vector, and the resulting garbage `robj *` is dereferenced. Reproduced on a two-node cluster (`cluster_state:ok`) with one slot `MIGRATING`: ``` control: COPY {b}k1 {b}k2 DB 0 -> TRYAGAIN ... (server survives) trigger: COPY {b}k1 {b}k2 DB -> Server closed the connection ``` ``` === VALKEY BUG REPORT START === crashed by signal: 11, si_code: 2 0 valkey-server getLongLongFromObject + 28 1 valkey-server getNodeByQuery + 1312 2 valkey-server processCommand + 1128 ``` Reaching the check requires the slot to be in migrating or importing state (`cluster.c` short-circuits otherwise), i.e. during a resharding. `COPY`'s `DB` and `REPLACE` tokens are optional and order-independent, and `DB` may appear more than once (`copyCommand()` keeps the last value — see the note above `copyDbIdArgs()`), but the guard only inspected `argv[3]`. So the check was silently skipped whenever `REPLACE` came first. On the same cluster, both keys present, slot migrating: ``` COPY {b}k1 {b}k2 DB 1 REPLACE -> TRYAGAIN ... (correctly blocked) COPY {b}k1 {b}k2 REPLACE DB 1 -> 1 (cross-DB copy went through) ``` The second form performed exactly the cross-DB copy during slot migration that this guard exists to prevent. Scan every `DB` clause the way `copyCommand()` does, instead of hardcoding `argv[3]`/`argv[4]`. A `DB` token with no value is left alone so `copyCommand()` reports the syntax error. Behavior with the fix, same setup: | command | before | after | |---|---|---| | `COPY k1 k2 DB` | **SIGSEGV** | `ERR syntax error` | | `COPY k1 k2 REPLACE DB` | `ERR syntax error` | `ERR syntax error` | | `COPY k1 k2 REPLACE DB 1` | `1` (cross-DB copy) | `TRYAGAIN ...` | | `COPY k1 k2 DB 1 REPLACE` | `TRYAGAIN ...` | `TRYAGAIN ...` | | `COPY k1 k2 DB 0 DB 1` | `1` (cross-DB copy) | `TRYAGAIN ...` | | `COPY k1 k2 DB 0 REPLACE` | `1` | `1` | | `COPY k1 k2 REPLACE DB 0` | `1` | `1` | Extended the existing "Cross-DB COPY command should not be allow during slot migration" test. Verified the new assertions fail on unstable without the code change and pass with it; `unit/cluster/slot-migration`, `unit/cluster/multidb` and `unit/keyspace` all pass. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
AI conflict resolution: source PR #4301Fix out-of-bounds read in getNodeByQuery's cross-DB COPY check 2 conflicted files Claude Summary
AI-resolved conflicted files Full backport commit diff: commit 8409457f5f44.
|
…4211) Fix a double free in `moduleTimerHandler()` when a module timer callback calls`ValkeyModule_StopTimer()` on the currently firing timer (`VM_StopTimer` frees the object, then the dispatcher freed it again). After the callback returns, only remove/free the timer if the original timer ID still maps to the same live object in `Timers`. Fixes #4200.
) #4049 fixed the FIELDS token validation for `HGETDEL`. The same parsing issue also exists in `HPERSIST` and `HTTL/HPTTL/HEXPIRETIME/HPEXPIRETIME`: The `FIELDS` keyword was never validated, so a malformed command like `HPERSIST key a 1 c` would silently treat `a` as a no-op and parse the remaining arguments instead of returning a syntax error. This PR aligns these commands with the HGETDEL fix: reject the command with a syntax error when the `FIELDS` keyword is missing, and adds tests covering the validation for all affected commands. Signed-off-by: cjx-zar <jxchenczar@foxmail.com>
…4253) Fixes #4231. ### Problem On a RESP3 connection that both subscribes and publishes to the same channel, a large pubsub payload can be emitted as a torn frame: the bulk payload first, then the PUBLISH integer reply, then an incomplete push header. Clients that track command replies (e.g. StackExchange.Redis) treat the bare bulk string as the PUBLISH reply and desync. ### Cause Self-publish defers push bytes into `pending_push_messages` via `_addReplyToBufferOrList`, but reply copy avoidance writes `bulkStrRef` directly into `c->buf` / `c->reply`, bypassing that deferral. The default copy-avoid threshold is 16384 bytes, matching the reported size. ### Fix Disable copy avoidance while `c->flag.pushing` is set in `isCopyAvoidPreferred()`, so push construction always uses the normal reply path that respects `pending_push_messages`. Signed-off-by: quanyeyang <quanyemostima@gmail.com> Co-authored-by: Binbin <binloveplay1314@qq.com>
5 participants
Backport sweep for 9.0
Automated cherry-picks from PRs marked "To be backported".
Applied
AI resolution details are posted as comments on this PR when available.
Needs attention
These candidates could not be applied automatically and need a maintainer to follow up.
1 candidate(s)
Generated by valkey-ci-agent using Claude Code.