Fix crash on module unload when ACL includes module commands; clean stale ACL rules and harden recompute#2916
Closed
sachinvmurthy wants to merge 1 commit intovalkey-io:7.2from
Closed
Conversation
…hat module was added to an ACL user.
Member
|
Thanks @sachinvmurthy for working on this, Can you please address these issues:
|
dvkashapov
reviewed
Dec 6, 2025
| /* Body after +/- */ | ||
| const char *body = tok+1; | ||
| if (*body == '@') { | ||
| /* Category: always keep */ |
Member
There was a problem hiding this comment.
Modules can register new ACL categories, I suppose we should clean them up too
Contributor
Author
There was a problem hiding this comment.
It does not crash on the categories so do we need to unregister them too?
dvkashapov
reviewed
Dec 6, 2025
| /* Command or subcommand. Extract main and optional sub after '|' */ | ||
| const char *pipe = strchr(body, '|'); | ||
| sds main = pipe ? sdsnewlen(body, pipe - body) : sdsnew(body); | ||
| struct redisCommand *cmd = dictFetchValue(server.orig_commands, main); |
Member
There was a problem hiding this comment.
Lets create this PR to unstable branch, now this struct is called serverCommand and wouldn't it be better to use struct serverCommand *lookupCommand(robj **argv, int argc)?
Contributor
Author
|
closing this PR since it needs to be raised against unstable. NEW PR - #2923 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.
Fix:
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.