Adversarial testing found related to forkless

[madolson]

Five crashes in the forkless save path, all on unstable at bb741667d. Reproducers: madolson/valkey@forkless-bug-repros — ./runtest --single unit/forkless-bugs.

cc @JimB123

1. EXEC without MULTI dereferences a NULL c->mstate

call() runs bgIteration_blockClientIfRequired() (server.c:4056) before execCommand() rejects "EXEC without MULTI" (multi.c:202). isWriteCmd() counts execCommand as a write, so bgiteration.c:2345 dispatches to expediteKeysForMultiExec(), which reads c->mstate->count (bgiteration.c:1935). c->mstate is lazily allocated (server.h:1373). server.c:4600 already guards this exact pair.

valkey-cli exec on a fresh connection during a forkless BGSAVE:

# crashed by signal: 11, si_code: 2
Symbol: expediteKeysForMultiExec
argc: 1   argv[0]: "exec"

2. Concurrent list reads corrupt quicklist state under the serializer

bgIteration blocks writes only (bgiteration.c:2317), and pauseRehashing() covers only OBJ_ENCODING_HASHTABLE / OBJ_ENCODING_BTREE (:741). Read commands mutate quicklist nodes: __quicklistDecompressNode frees the quicklistLZF and sets encoding = RAW (quicklist.c:261), __quicklistCompressNode frees node->entry and sets encoding = LZF (:234). The background thread reads those fields at rdb.c:917-925.

list-compress-depth 1, 300k-element list, 8 clients running LRANGE/LINDEX across forkless saves. Roughly 1 in 100 saves:

# crashed by signal: 11, si_code: 2
# Accessing address: 0x3e10ba55c2cefa38
0 crcspeed64little        1 rioGenericUpdateChecksum
3 rdbWriteRaw             4 rdbSaveObject
5 rdbSaveKeyValuePair     6 forklessSaveProcessor   <- background thread

Either the freed quicklistLZF is read, or the RAW branch does rdbSaveRawString(rdb, node->entry, node->sz) on an LZF blob of 8 + lzf->sz bytes while node->sz is still the uncompressed size.

3. RENAME over an existing key leaves a dangling pointer in early_iterate_entries

Pointers go in without a refcount (bgiteration.c:1090) and come out only when the scan reaches them (:1049), on a propagated DEL/UNLINK (:2613), or on reallocation (:2688). RENAME frees and reallocates inside one command:

1. expediteKeysForWrite expedites src and dst, so both pointers are in the table.
2. renameGenericCommand dbDelete()s dst. Its dbEntry is freed, and RENAME propagates as RENAME, so :2613 never runs — dst's pointer is now dangling.
3. dbAdd(dst, src_robj) → objectSetKeyAndExpire() allocates a same-sized replacement; the allocator returns dst's just-freed address.
4. bgIteration_updateDbEntryPtr(src_robj, new) deletes src's pointer, then hashtableAdd(new) collides.

hashtableAdd returns false only for a duplicate key (hashtable.c:1631) and insert() cannot fail, so the collision is with dst's dangling pointer.

Equal-length key names, no TTLs, RENAME aaNNNNN bbNNNNN in a loop during a forkless BGSAVE:

# ==> bgiteration.c:2693 'wasAdded' is not true
0 bgIteration_updateDbEntryPtr  1 objectSetKeyAndExpire  2 dbAddInternal
3 renameGenericCommand          4 call

Collection keys emptied by SREM/LPOP/HDEL/ZREM leave the same dangling pointers, just without the same-command reallocation.

Separately, that dangling pointer makes iteratorHasPassedKey() lie. A defragged dbEntry landing on such an address keeps its epoch — objectCopyMetadata copies it and defrag does not signal a modification — so feedIterator skips a key that was never modified and it is missing from the snapshot.

4. repl-diskless-load swapdb frees the kvstore the iterator is scanning

replicaBeforeLoadPrimaryRDB() (replication.c:2385) kills only CHILD_TYPE_RDB; it never calls forklessSaveCancel(). The swapdb path skips RDBFLAGS_EMPTY_DATA (replication.c:2528), so fullScanIteratorFlushDb() — the only place it->kvs is cleared — never runs. swapMainDbWithTempDb() and disklessLoadDiscardTempDb() then hand that kvstore to lazyfree while it->kvs, captured at bgiteration.c:551, still points at it.

Replica with repl-diskless-load swapdb, 300k local keys, forkless BGSAVE, then REPLICAOF. 29ms after "Discarding old DB in background":

# crashed by signal: 11   Accessing address: 0x0
EIP: fullScanIteratorGetEntries
0 fullScanIteratorGetEntries  1 bgIteration_feedIterators_task  2 aeProcessEvents

Clearing it->kvs is not sufficient on its own. Because this path never notifies bgIteration, returnAllItemsToMainThread() never runs either, so the whole queued backlog stays checked out at refcount 2 while the bio thread walks the old kvstore. Logging every off-main-thread decrRefCount of an robj with refcount > 1 gave 2, 38, 58 and 87 shared entries across four trials, scaling with queue depth (item_count_target ceiling is 10000). emptyDbAsync (lazyfree.c:210) has no refcount guard — unlike freeObjAsync (refcount == 1) and tryOffloadFreeObjToIOThreads (refcount > 1) — and decrRefCount is a plain non-atomic read-branch-decrement, so those entries are decremented concurrently by the bio thread and by decrementEntryInuse on the main thread. A lost decrement leaks the entry and its value tree.

The crash currently masks that. Stopping the dereference without draining the queue would remove the crash and leave the race live on up to 10000 objects, so the fix needs to hand the borrowed entries back.

For contrast, the FLUSHALL path is safe and not worth changing: bgIteratorTerminate drains the queue synchronously, so exactly one entry is ever exposed (6/6 trials, always the one being serialized), and the only thing that keeps it exposed is an uninterruptible stall, which also pushes the two decrements >=78ms apart. With a large value instead of rdb-key-save-delay the abort check returns the entry before emptyDbAsync runs and nothing is shared at all (0/6).

5. A primary write to a held key panics the replica

bgIteration_blockClientIfRequired() has no caller-type filter (bgiteration.c:2313). blockClientInUseOnKeys() asserts !c->flag.replica (blocked.c:921), but the primary link client has flag.primary, so it reaches blockClient(c, BLOCKED_INUSE) and isReplicatedClient() (server.c:3704) is true at blocked.c:109. Also reachable for a slot-migration import client.

Replica runs a forkless BGSAVE; primary SETs an existing >512-byte key (too big for tryCloneDbEntry, so the applying client must block):

# ==> blocked.c:109 '!(isReplicatedClient(c) && btype != BLOCKED_MODULE && btype != BLOCKED_POSTPONE)' is not true
0 blockClient  1 blockClientInUseOnKeys  2 bgIteration_blockClientIfRequired  3 call  4 processCommand

Even without the assert, blockClientInUseOnKeys() clears the read handler and leaves pending_command set, so reploff stops advancing for the duration.

---

Things the AI flagged that I'm not convinced matter, but leaving in case someone else feels strongly about it.

• db.c:116 gates the LRU touch on hasActiveChildProcess() rather than hasActiveSaveOrChild(), so val->lru = lrulfu_touch(val->lru) runs on the object the background thread is reading.
• barrier_items is decremented twice on the terminate path (bgiteration.c:1421-1428 then :1497-1500), and handleFlushdb (:1833) lacks the completed || terminated guard handleSwapdb has (:1794). Unreachable until the first CONSISTENCY_EVENTUAL client.
  • This one might actually be real, but adversarial couldn't actually trigger this.

This was generated by AI but verified, with love, by a human.

[JimB123]

1. confirmed. fix identified.
2. confirmed. fix identified.

• HOWEVER it is noted that there are inherent inefficiencies with quicklist compression. In cases of read commands that change the compression, often the quicklist node is decompressed temporarily (changing the structure) only to recompress it again (again changing the structure). A mechanism that provides access to a transient decompression would save us the expense of immediate recompression - and avoid altering the in-memory structure.

3. confirmed. fix in progress.
4. TBD
5. TBD