Fix UAF in streams when XADD creates a new node

[Baraa-Hasheesh]

Fixes #4705.

Summary

XADD can leave a stream's radix tree holding a dangling pointer to a freed
listpack. When the entry is later read (XRANGE, XREVRANGE,
XINFO STREAM ... FULL), we hit UB.

Root cause

In streamAppendItem(), when a XADD cannot append to the current tail node, the tail node is shrunk to fit,
that in turn might relocate it to a different location in memory, while ri.data is kept pointing to the old dangling pointer.

if (new_node) {
    lp = lpShrinkToFit(lp); <------ lp is moved to a new location in memory
    if (ri.data != lp) raxInsert(s->rax, ri.key, ri.key_len, lp, NULL);
    lp = NULL;
    <----- ri.data is still pointing to the old dangling pointer
}

After which a new node is created & is inserted.

    lp = lpNew(prealloc);
    ....
    raxInsert(s->rax, (unsigned char *)&rax_key, sizeof(rax_key), lp, NULL);
    <---- lp is now pointing to a new location different from ri.data (will later dangle)

Following that multiple appends happen on that node, that operation might reallocate it & it's possible that the final
allocation might just happen to reallocate to the same location as the original node ri.data, I.E. ri.data == lp

Due to this unfortunate allocation, the radix tree will never be updated with the latest values leaving it with a dangling pointer

if (ri.data != lp) raxInsert(s->rax, (unsigned char *)&rax_key, sizeof(rax_key), lp, NULL);
<---- This check fails as ri.data == lp

Fix

Two changes in streamAppendItem():

• ri.data is invalidated if the current tail node is shrunk & reallocated to a new location
• remove raxInsert when a new node is created to avoid sending a pointer to the radix tree that might later get invalidated

Testing

Added an integration test in tests/unit/type/stream.tcl that reliably fails with jemalloc without the fix

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Advanced

Run ID: f35d04a7-42d0-42e5-88e6-2b9caa06cb1a

📥 Commits

Reviewing files that changed from the base of the PR and between 334343b and 7f9c834.

📒 Files selected for processing (2)

• src/t_stream.c
• tests/unit/type/stream.tcl

🚧 Files skipped from review as they are similar to previous changes (2)

• tests/unit/type/stream.tcl
• src/t_stream.c

Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review.

---

📝 Walkthrough

Walkthrough

The stream append path now tracks radix-tree listpack pointers separately during replacement and insertion. A regression test covers traversal across macro nodes and restores the previous stream-node byte limit.

Changes

Stream macro-node handling

Layer / File(s)	Summary
Listpack replacement and regression coverage src/t_stream.c, tests/unit/type/stream.tcl	streamAppendItem tracks the radix-tree listpack pointer separately, updates it after lpShrinkToFit, and uses the common insertion path for new listpacks. The regression test validates stream traversal, payload integrity, and configuration restoration across multiple macro nodes.

Priority: ⬆️ High

Estimated code review effort: 2 (Simple) | ~10 minutes

Severity of issue fixed: High

Suggested reviewers: madolson

Merge Risk: ⚪ Minimal · up to 7f9c8

The stream append fix updates the radix-tree pointer after listpack replacement and the regression test validates forward and reverse traversal across macro nodes. No actionable merge risk remains.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Linked Issues check	⚠️ Warning	The implementation addresses the dangling-pointer path in streamAppendItem(): it tracks the pointer held by the radix tree, updates that entry after tail-node shrinking, and inserts a new node only …	Add XGROUP CREATE k2 g2 0 MKSTREAM to the regression sequence, or provide evidence that the current sequence fails before the fix and passes with the fix under jemalloc.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely describes the main change: fixing a use-after-free in stream handling when XADD creates a new node.
Description check	✅ Passed	The description accurately explains the use-after-free, root cause, fix, affected commands, linked issue, and regression test.
Out of Scope Changes check	✅ Passed	The changes are limited to stream radix-tree pointer handling in src/t_stream.c and a focused regression test in tests/unit/type/stream.tcl. These changes support issue #4705.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 1 files. (1 skipped: 1 …

Full details: Linked Issues check

Explanation

The implementation addresses the dangling-pointer path in streamAppendItem(): it tracks the pointer held by the radix tree, updates that entry after tail-node shrinking, and inserts a new node only through the common update path. The regression test covers stream-node-max-bytes=1, the issue payload, and full XINFO STREAM, XRANGE, and XREVRANGE traversal. However, the test omits the issue's documented load-bearing XGROUP CREATE k2 g2 0 MKSTREAM command. The test therefore does not establish reproduction of the reported jemalloc failure sequence.

• Fix all pre-merge checks with AI

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[valkey-review-bot]

The fix handles the stale pointer comparison and the server target builds successfully. One test-isolation issue remains.

[valkey-review-bot]

This hard-codes the global config to its default instead of restoring the value that was active before the test. --config stream-node-max-bytes ... and external-server runs can start with a different value, and this leaves later stream tests running with 4096. Save it with config_get_set stream-node-max-bytes 1 at the first change, then restore that saved value here.

[Baraa-Hasheesh]

handled in d5b3623

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 80.63%. Comparing base (122ec7e) to head (7f9c834).
⚠️ Report is 8 commits behind head on unstable.

Additional details and impacted files

@@             Coverage Diff              @@
##           unstable    #4711      +/-   ##
============================================
- Coverage     80.63%   80.63%   -0.01%     
============================================
  Files           192      192              
  Lines        100803   100852      +49     
============================================
+ Hits          81287    81321      +34     
- Misses        19516    19531      +15     

Files with missing lines	Coverage Δ
src/t_stream.c	95.04% <100.00%> (+<0.01%)	⬆️

... and 24 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[madolson]

Fix looks fine, but an alternative suggestion.