Fix crash unloading module referenced by ACL rules by sachinvmurthy · Pull Request #2923 · valkey-io/valkey

sachinvmurthy · 2025-12-09T21:41:18Z

Problem:

Unloading a module crashes if any ACL user has rules referencing that module’s commands or subcommands (e.g., +hello.simple). The crash occurs in ACLRecomputeCommandBitsFromCommandRulesAllUsers while applying stale rules; previously it asserted on failure.

Root Cause:
Stale ACL command rules persist in users’ selectors after module unload. Recompute attempts to reapply them, resulting in invalid lookups or assertions.

Fixes #2797

Add ACLCleanupStaleCommandRulesAllUsers to proactively remove non-existent module commands/subcommands from selector->command_rules.
Make ACLRecomputeCommandBitsFromCommandRulesAllUsers skip invalid rules with a warning instead of asserting.

sachinvmurthy · 2025-12-09T21:44:57Z

cc: @roshkhatri @dmitrypol @dvkashapov

Thanks for reviewing the earlier PR. Created a new PR against unstable as addressed in the old PR.

zuiderkwast · 2025-12-10T15:40:42Z

src/acl.c

+                        const char *sub = pipe + 1;
+                        if (cmd->subcommands_ht) {
+                            sds sub_sds = sdsnew(sub);
+                            struct serverCommand *sc = (struct serverCommand *)dictFetchValue(cmd->subcommands_ht, sub_sds);


The command table uses hashtable instead of dict in 8.1 and later. That seems to be why the build is failing.

We probably need this code with dict when we backport the fix to 8.0 and 7.2 though.

... or would it work to use lookupCommandBySdsLogic here?

UNTESTED:

Suggested change

struct serverCommand *sc = (struct serverCommand *)dictFetchValue(cmd->subcommands_ht, sub_sds);

struct serverCommand *sc = lookupCommandBySdsLogic(cmd->subcommands_ht, sub_sds);

ah okay! thank you. will try and see if that helps

zuiderkwast · 2025-12-10T17:12:25Z

src/acl.c

-/* Get a command from the original command table, that is not affected
- * by the command renaming operations: we base all the ACL work from that
- * table, so that ACLs are valid regardless of command renaming. */
-static struct serverCommand *ACLLookupCommand(const char *name) {


If you add the new function below this existing function, there is no need to move the existing function ACLLookupCommand within the file. We can minimize the diff, the git history footprint.

src/module.c

rjd15372

LGTM. I'm approving the PR but make sure to address @zuiderkwast comments.

roshkhatri

I agree with @zuiderkwast 's review. Overall looks good.

roshkhatri · 2025-12-15T21:46:58Z

src/acl.c

+                if (first != '+' && first != '-') {
+                    /* Unknown formatting, keep as-is */
+                    new_rules = sdscatfmt(new_rules, "%S ", tok);
+                    continue;


What are the unknown formatting are we looking at here?

just wanted to cover all edge cases.

Signed-off-by: sachinvmurthy <sachin.murthy97@gmail.com>

roshkhatri

Everything looks good, just minor questions and feedback

roshkhatri · 2026-02-02T04:57:21Z

src/acl.c

+            aclSelector *selector = (aclSelector *) listNodeValue(ln);
+            int argc = 0;
+            sds *argv = sdssplitargs(selector->command_rules, &argc);
+            if (!argv) continue;


Can we atleast add a warning here while continuing, if sdssplitargs() fails for any reason?

roshkhatri · 2026-02-02T05:00:53Z

src/acl.c

+            } else {
+                sdsfree(selector->command_rules);
+                selector->command_rules = sdsempty();
+                sdsfree(new_rules);


How can we make sure the new rules are parsable? is there a small check we can add?

roshkhatri · 2026-02-02T21:21:52Z

After discussion in the meeting today, it was decided to not edit the command rules and reject unloading of the module until ACLs are cleaned by the user

github-actions bot assigned sachinvmurthy Dec 9, 2025

sachinvmurthy force-pushed the bug/fix_issue_2797 branch from f0a86d9 to a575173 Compare December 9, 2025 21:42

This was referenced Dec 9, 2025

[CRASH] Valkey7/8/9 crashes when unloading a module if a command from that module was added to an ACL user. #2797

Closed

Fix crash on module unload when ACL includes module commands; clean stale ACL rules and harden recompute #2916

Closed

sachinvmurthy force-pushed the bug/fix_issue_2797 branch from f086df4 to 1f2b77e Compare December 9, 2025 21:48

zuiderkwast changed the title ~~fix for bug reported in issue #2797~~ Fix crash unloading module referenced by ACL rules Dec 10, 2025

zuiderkwast reviewed Dec 10, 2025

View reviewed changes

src/module.c Show resolved Hide resolved

rjd15372 approved these changes Dec 11, 2025

View reviewed changes

roshkhatri reviewed Dec 15, 2025

View reviewed changes

sachinvmurthy force-pushed the bug/fix_issue_2797 branch from 1f2b77e to c47e846 Compare December 31, 2025 20:31

sachinvmurthy added 3 commits December 31, 2025 12:40

fix for bug reported in issue valkey-io#2797

466df19

Signed-off-by: sachinvmurthy <sachin.murthy97@gmail.com>

removed debug files

547335e

Signed-off-by: sachinvmurthy <sachin.murthy97@gmail.com>

addressed comments

e5d3201

Signed-off-by: sachinvmurthy <sachin.murthy97@gmail.com>

sachinvmurthy force-pushed the bug/fix_issue_2797 branch from c47e846 to e5d3201 Compare December 31, 2025 20:41

sachinvmurthy requested review from roshkhatri and zuiderkwast December 31, 2025 20:48

roshkhatri reviewed Feb 2, 2026

View reviewed changes

sachinvmurthy closed this Feb 3, 2026

zuiderkwast mentioned this pull request Feb 9, 2026

[BUG][issue-2797] Block module unload when ACL references module subcommands #3160

Merged

	struct serverCommand sc = (struct serverCommand )dictFetchValue(cmd->subcommands_ht, sub_sds);
	struct serverCommand *sc = lookupCommandBySdsLogic(cmd->subcommands_ht, sub_sds);

Comments

Conversation

sachinvmurthy commented Dec 9, 2025 • edited by zuiderkwast Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sachinvmurthy commented Dec 9, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rjd15372 left a comment

Choose a reason for hiding this comment

Uh oh!

roshkhatri left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

roshkhatri left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

roshkhatri commented Feb 2, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

sachinvmurthy commented Dec 9, 2025 •

edited by zuiderkwast

Loading

roshkhatri left a comment •

edited

Loading