Fix use-after-free in VM_RegisterClusterMessageReceiver by eifrah-aws · Pull Request #3846 · valkey-io/valkey

eifrah-aws · 2026-05-27T10:38:55Z

When unregistering a cluster message receiver (callback = NULL) for the head node of the linked list, the code incorrectly updated clusterReceivers[type]->next instead of clusterReceivers[type] itself. This left the array entry pointing to freed memory, causing a use-after-free on any subsequent list traversal.

When unregistering a cluster message receiver (callback = NULL) for the head node of the linked list, the code incorrectly updated clusterReceivers[type]->next instead of clusterReceivers[type] itself. This left the array entry pointing to freed memory, causing a use-after-free on any subsequent list traversal. Signed-off-by: Eran Ifrah <eifrah@amazon.com>

coderabbitai · 2026-05-27T10:39:13Z

📝 Walkthrough

Walkthrough

Fixes the head-unlink bug in VM_RegisterClusterMessageReceiver and adds test module commands plus a Tcl integration test that unregisters and re-registers a type-3 cluster receiver and verifies message reception.

Changes

Cluster receiver unregistration and tests

Layer / File(s)	Summary
Head node pointer update on receiver removal `src/module.c`	When removing a cluster message receiver that is the list head for its type, the head pointer is now set to `r->next` instead of an invalid intermediate assignment.
Test module: register/unregister/send cmds `tests/modules/cluster.c`	Adds `MSGTYPE_TEST_UAF` and commands `test.register_receiver`, `test.unregister_receiver`, and `test.send_msg_type3` to register/unregister the DingReceiver for type 3 and to send a type-3 message.
Tcl integration test `tests/unit/moduleapi/cluster.tcl`	Adds a test that unregisters and re-registers the type-3 receiver, sends a type-3 message from another node, and asserts reception and a matching log entry.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 16.67% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'Fix use-after-free in VM_RegisterClusterMessageReceiver' accurately describes the main bug fix in the changeset - correcting a use-after-free vulnerability in the receiver unregistration logic.
Description check	✅ Passed	The description clearly explains the bug and the fix, detailing how unregistering the head node was incorrectly updating the wrong pointer and causing a use-after-free issue.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

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

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

codecov · 2026-05-27T11:05:01Z

Codecov Report

❌ Patch coverage is 0% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 76.61%. Comparing base (e4fdae4) to head (d079a56).
⚠️ Report is 1 commits behind head on unstable.

Files with missing lines	Patch %	Lines
src/module.c	0.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##           unstable    #3846      +/-   ##
============================================
- Coverage     76.69%   76.61%   -0.08%     
============================================
  Files           162      162              
  Lines         80679    80679              
============================================
- Hits          61874    61816      -58     
- Misses        18805    18863      +58

Files with missing lines	Coverage Δ
src/module.c	`25.31% <0.00%> (ø)`

... and 26 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.

enjoy-binbin

Okay, so it was wrong from the very beginning.

Test that unregistering the head receiver and re-registering does not crash due to accessing freed memory. Signed-off-by: Eran Ifrah <eifrah@amazon.com>

coderabbitai

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@tests/modules/cluster.c`:
- Around line 93-112: Each handler lacks an arity check—add a guard at the top
of test_register_receiver, test_unregister_receiver, and test_send_msg_type3 to
ensure argc == 1; if not, return an argument error (e.g. call
ValkeyModule_ReplyWithError(ctx, "ERR wrong number of arguments") and return)
before using ValkeyModule_RegisterClusterMessageReceiver /
ValkeyModule_SendClusterMessage so the command contract is enforced.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 412db148-e870-4026-beca-b5924d46bb8c

📥 Commits

Reviewing files that changed from the base of the PR and between afd2368 and d079a56.

📒 Files selected for processing (2)

tests/modules/cluster.c
tests/unit/moduleapi/cluster.tcl

When unregistering a cluster message receiver (callback = NULL) for the head node of the linked list, the code incorrectly updated clusterReceivers[type]->next instead of clusterReceivers[type] itself. This left the array entry pointing to freed memory, causing a use-after-free on any subsequent list traversal. --------- Signed-off-by: Eran Ifrah <eifrah@amazon.com>

# Backport sweep for 9.1 Automated cherry-picks from PRs marked "To be backported". ## Applied | Source PR | Title | Detail | |---|---|---| | #3743 | Fix buffered_reply assert in HFE commands with module keyspace notifications | cherry-picked in a prior sweep | | #3766 | Fix flaky block_keyspace_notification test for HGETDEL notify race | cherry-picked in a prior sweep | | #3800 | Fix heap-use-after-free in ACL LOAD when client free is deferred | cherry-picked in a prior sweep | | #3723 | Fix double-finish and RESP reply violation in cluster slot migration | cherry-picked in a prior sweep | | #3872 | Redacting customer information when hide_user_data_from_log is true in rdb.c, networking.c, debug.c and t_hash | cherry-picked in a prior sweep | | #3846 | Fix use-after-free in VM_RegisterClusterMessageReceiver | cherry-picked in a prior sweep | | #3806 | Add ALL_DBS flag to CLUSTER FLUSHSLOT for database-level ACL | cherry-picked in a prior sweep | | #3847 | Harden SENTINEL commands and config rewrite against control-character injection | | | #3801 | Validate every DB clause in COPY against ACL db= permissions | | | #3851 | Replace AUTOMATION_PAT with Valkeyrie Bot GitHub App token | | | #3848 | Fix cluster AUX-field control-character and delimiter injection | | | #3544 | Revert "IO-Threads redesign cleanup work (#3544)" | cherry-picked in a prior sweep | | #3888 | Report exact dbid for COPY in ACL LOG when db= access is denied | conflicts resolved by Claude Code | | #3942 | Fix shard_id format specifier in UPDATE message log | | | #3941 | Avoid random() % 0 undefined behaviour when cluster-node-timeout < 30 | | --- *Generated by valkey-ci-agent using Claude Code.* --------- Signed-off-by: Binbin <binloveplay1314@qq.com> Signed-off-by: Ran Shidlansik <ranshid@amazon.com> Signed-off-by: chx9 <lovelypiska@outlook.com> Signed-off-by: zackcam <zackcam@amazon.com> Signed-off-by: Eran Ifrah <eifrah@amazon.com> Signed-off-by: Jules Lasarte <lasartej@amazon.com> Signed-off-by: jjuleslasarte <jules.lasarte@gmail.com> Signed-off-by: akash kumar <akumdev@amazon.com> Co-authored-by: Binbin <binloveplay1314@qq.com> Co-authored-by: Ran Shidlansik <ranshid@amazon.com> Co-authored-by: lovelypiska <lovelypiska@outlook.com> Co-authored-by: zackcam <zackcam@amazon.com> Co-authored-by: eifrah-aws <eifrah@amazon.com> Co-authored-by: jjuleslasarte <jules.lasarte@gmail.com> Co-authored-by: Jules Lasarte <lasartej@amazon.com> Co-authored-by: Akash Kumar <45854686+akashkgit@users.noreply.github.com>

When unregistering a cluster message receiver (callback = NULL) for the head node of the linked list, the code incorrectly updated clusterReceivers[type]->next instead of clusterReceivers[type] itself. This left the array entry pointing to freed memory, causing a use-after-free on any subsequent list traversal. --------- Signed-off-by: Eran Ifrah <eifrah@amazon.com>

github-actions Bot assigned eifrah-aws May 27, 2026

enjoy-binbin approved these changes May 27, 2026

View reviewed changes

enjoy-binbin added this to Valkey 9.0, Valkey 9.1, Valkey 8.1, Valkey 8.0 and Valkey 7.2 May 27, 2026

enjoy-binbin moved this to To be backported in Valkey 7.2 May 27, 2026

enjoy-binbin moved this to To be backported in Valkey 8.0 May 27, 2026

enjoy-binbin moved this to To be backported in Valkey 8.1 May 27, 2026

enjoy-binbin moved this to To be backported in Valkey 9.0 May 27, 2026

enjoy-binbin moved this to To be backported in Valkey 9.1 May 27, 2026

eifrah-aws mentioned this pull request May 27, 2026

Fix use after free #3836

Closed

sarthakaggarwal97 approved these changes May 27, 2026

View reviewed changes

murphyjacob4 reviewed May 27, 2026

View reviewed changes

Comment thread src/module.c

eifrah-aws added 2 commits May 28, 2026 13:23

Add test for use-after-free fix in VM_RegisterClusterMessageReceiver

07f1b57

Test that unregistering the head receiver and re-registering does not crash due to accessing freed memory. Signed-off-by: Eran Ifrah <eifrah@amazon.com>

Merge branch 'unstable' into security-fix-use-after-free

d079a56

coderabbitai Bot reviewed May 28, 2026

View reviewed changes

Comment thread tests/modules/cluster.c

ranshid approved these changes May 31, 2026

View reviewed changes

ranshid merged commit f25ec5e into valkey-io:unstable May 31, 2026
63 checks passed

This was referenced May 30, 2026

[backport] Backport sweep for 9.1 #3774

Merged

[backport] Backport sweep for 7.2 #3738

Merged

valkeyrie-ops Bot mentioned this pull request May 30, 2026

[backport] Backport sweep for 8.1 #3737

Merged

valkeyrie-ops Bot mentioned this pull request May 31, 2026

[backport] Backport sweep for 8.0 #3736

Merged

eifrah-aws deleted the security-fix-use-after-free branch June 1, 2026 07:35

zuiderkwast added the release-notes This issue should get a line item in the release notes label Jun 2, 2026

zuiderkwast moved this from To be backported to 8.1.8 (WIP) in Valkey 8.1 Jun 2, 2026

sarthakaggarwal97 moved this from To be backported to Done in Valkey 9.1 Jun 15, 2026

valkeyrie-ops Bot moved this from To be backported to Done in Valkey 8.0 Jun 18, 2026

This was referenced Jun 19, 2026

[backport] Backport sweep for 9.0 #3879

Open

[backport] Backport sweep for 7.2 #4048

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix use-after-free in VM_RegisterClusterMessageReceiver#3846

Fix use-after-free in VM_RegisterClusterMessageReceiver#3846
ranshid merged 3 commits into
valkey-io:unstablefrom
eifrah-aws:security-fix-use-after-free

eifrah-aws commented May 27, 2026

Uh oh!

coderabbitai Bot commented May 27, 2026 •

edited

Loading

Walkthrough

Changes

Estimated code review effort

❌ Failed checks (1 warning)

Uh oh!

codecov Bot commented May 27, 2026 •

edited

Loading

Uh oh!

enjoy-binbin left a comment

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Uh oh!

Conversation

eifrah-aws commented May 27, 2026

Uh oh!

coderabbitai Bot commented May 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

❌ Failed checks (1 warning)

Uh oh!

codecov Bot commented May 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

enjoy-binbin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

coderabbitai Bot commented May 27, 2026 •

edited

Loading

codecov Bot commented May 27, 2026 •

edited

Loading