[BUG][issue-2797] Block module unload when ACL references module subcommands by sachinvmurthy · Pull Request #3160 · valkey-io/valkey

sachinvmurthy · 2026-02-03T21:41:50Z

Summary:

Prevent MODULE UNLOAD when ACL rules reference a module subcommand.
Avoids crash during ACL recompute after module removal.
Adds coverage for subcommand ACL rules.
Tests:

make test
Fixes #2797

sachinvmurthy · 2026-02-03T22:57:42Z

cc: @roshkhatri @zuiderkwast Please review. Have made the changes as per the discussion. Thank you :D

Copilot

Pull request overview

This PR fixes a crash (issue #2797) that occurs when unloading a module that has commands referenced in ACL rules. When a module is unloaded, Valkey attempts to recompute ACL command bits, which fails if ACL rules reference commands that no longer exist.

Changes:

Adds pre-unload validation to check if any ACL users reference commands from the module being unloaded
Blocks module unload with a descriptive error message when ACL references are found
Adds test coverage for ACL subcommand rules blocking module unload

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File	Description
src/server.h	Declares new `ACLModuleHasCommandRules` function for checking ACL references to module commands
src/acl.c	Implements `ACLModuleHasCommandRules` to scan all ACL users and detect references to module commands or subcommands
src/module.c	Integrates ACL check into `moduleUnloadInternal` to block unload when ACL references exist
tests/unit/moduleapi/subcommands.tcl	Adds test verifying module unload is blocked when ACL rules reference module subcommands

tests/unit/moduleapi/subcommands.tcl

madolson

I think this makes more sense to prevent the crash instead of altering the user ACLs, also seems a bit simpler. Just some suggested improvements and added test coverage.

src/module.c

tests/unit/moduleapi/subcommands.tcl

src/acl.c

src/server.h

Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

src/acl.c

codecov · 2026-02-05T12:48:51Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 0.00%. Comparing base (5d7d542) to head (bfe06fa).
⚠️ Report is 36 commits behind head on unstable.

Additional details and impacted files

@@             Coverage Diff              @@
##           unstable   #3160       +/-   ##
============================================
- Coverage     74.72%       0   -74.73%     
============================================
  Files           129       0      -129     
  Lines         71309       0    -71309     
============================================
- Hits          53284       0    -53284     
+ Misses        18025       0    -18025

see 129 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.

zuiderkwast · 2026-02-09T10:49:10Z

Replaces Fix crash unloading module referenced by ACL rules #2923

roshkhatri

Overall the function ACLModuleHasCommandRules implementation looks good, just few suggestions about the tests.

tests/unit/moduleapi/subcommands.tcl

src/module.c

src/acl.c

roshkhatri · 2026-02-09T17:39:21Z

We should probably add this to release-notes

tests/unit/moduleapi/subcommands.tcl

roshkhatri · 2026-02-16T22:52:37Z

Please don't resolve the comments that are not responded to, or any updates are done.
Couldn't keep track of the comments if addressed for example: #3160 (comment)

roshkhatri · 2026-02-17T18:48:21Z

Hey @sachinvmurthy , I am sorry. I was wrong about the file where the tests should be. Tests should be going in https://github.com/sachinvmurthy/valkey/blob/unstable/tests/unit/moduleapi/aclcheck.tcl

roshkhatri

LGTM, we can merge this @zuiderkwast can you please look at this?

madolson

Cool, thanks!

…ommands (valkey-io#3160) Summary: Prevent MODULE UNLOAD when ACL rules reference a module subcommand. Avoids crash during ACL recompute after module removal. Adds coverage for subcommand ACL rules. Tests: make test Fixes valkey-io#2797 --------- Signed-off-by: sachinvmurthy <sachin.murthy97@gmail.com> Signed-off-by: Sachin Venkatesha Murthy <sachin.murthy97@gmail.com> Signed-off-by: Roshan Khatri <rvkhatri@amazon.com>

…ommands (valkey-io#3160) Summary: Prevent MODULE UNLOAD when ACL rules reference a module subcommand. Avoids crash during ACL recompute after module removal. Adds coverage for subcommand ACL rules. Tests: make test Fixes valkey-io#2797 --------- Signed-off-by: sachinvmurthy <sachin.murthy97@gmail.com> Signed-off-by: Sachin Venkatesha Murthy <sachin.murthy97@gmail.com>

…ommands (valkey-io#3160) Summary: Prevent MODULE UNLOAD when ACL rules reference a module subcommand. Avoids crash during ACL recompute after module removal. Adds coverage for subcommand ACL rules. Tests: make test Fixes valkey-io#2797 --------- Signed-off-by: sachinvmurthy <sachin.murthy97@gmail.com> Signed-off-by: Sachin Venkatesha Murthy <sachin.murthy97@gmail.com> Signed-off-by: Roshan Khatri <rvkhatri@amazon.com>

…ommands (valkey-io#3160) Prevent MODULE UNLOAD when ACL rules reference a module subcommand. Avoids crash during ACL recompute after module removal. Adds coverage for subcommand ACL rules. Fixes valkey-io#2797 (cherry picked from commit 385895a) Signed-off-by: Sachin Venkatesha Murthy <sachin.murthy97@gmail.com> Signed-off-by: Roshan Khatri <rvkhatri@amazon.com>

…ommands (valkey-io#3160) Summary: Prevent MODULE UNLOAD when ACL rules reference a module subcommand. Avoids crash during ACL recompute after module removal. Adds coverage for subcommand ACL rules. Tests: make test Fixes valkey-io#2797 --------- Signed-off-by: sachinvmurthy <sachin.murthy97@gmail.com> Signed-off-by: Sachin Venkatesha Murthy <sachin.murthy97@gmail.com>

…ommands (valkey-io#3160) Summary: Prevent MODULE UNLOAD when ACL rules reference a module subcommand. Avoids crash during ACL recompute after module removal. Adds coverage for subcommand ACL rules. Tests: make test Fixes valkey-io#2797 --------- Signed-off-by: sachinvmurthy <sachin.murthy97@gmail.com> Signed-off-by: Sachin Venkatesha Murthy <sachin.murthy97@gmail.com> Signed-off-by: Roshan Khatri <rvkhatri@amazon.com>

…ommands (valkey-io#3160) Prevent MODULE UNLOAD when ACL rules reference a module subcommand. Avoids crash during ACL recompute after module removal. Adds coverage for subcommand ACL rules. Fixes valkey-io#2797 (cherry picked from commit 385895a) Signed-off-by: Sachin Venkatesha Murthy <sachin.murthy97@gmail.com> Signed-off-by: Roshan Khatri <rvkhatri@amazon.com>

…ommands (#3160) Prevent MODULE UNLOAD when ACL rules reference a module subcommand. Avoids crash during ACL recompute after module removal. Adds coverage for subcommand ACL rules. Fixes #2797 (cherry picked from commit 385895a) Signed-off-by: Sachin Venkatesha Murthy <sachin.murthy97@gmail.com> Signed-off-by: Roshan Khatri <rvkhatri@amazon.com>

github-actions bot assigned sachinvmurthy Feb 3, 2026

sachinvmurthy force-pushed the bug/fixes_issue_2797 branch 2 times, most recently from 133aff1 to 5e229fd Compare February 3, 2026 21:44

madolson requested a review from Copilot February 3, 2026 23:18

Copilot started reviewing on behalf of madolson February 3, 2026 23:20 View session

Copilot AI reviewed Feb 3, 2026

View reviewed changes

tests/unit/moduleapi/subcommands.tcl Outdated Show resolved Hide resolved

madolson reviewed Feb 3, 2026

View reviewed changes

src/module.c Outdated Show resolved Hide resolved

tests/unit/moduleapi/subcommands.tcl Outdated Show resolved Hide resolved

src/acl.c Outdated Show resolved Hide resolved

src/server.h Outdated Show resolved Hide resolved

sachinvmurthy force-pushed the bug/fixes_issue_2797 branch from 5f079c1 to 891e8b4 Compare February 4, 2026 18:41

sachinvmurthy requested review from Copilot and madolson February 4, 2026 19:17

Copilot started reviewing on behalf of sachinvmurthy February 4, 2026 19:17 View session

Copilot AI reviewed Feb 4, 2026

View reviewed changes

src/acl.c Show resolved Hide resolved

sachinvmurthy force-pushed the bug/fixes_issue_2797 branch 2 times, most recently from da6df73 to 9adf03f Compare February 5, 2026 21:45

roshkhatri suggested changes Feb 9, 2026

View reviewed changes

sachinvmurthy force-pushed the bug/fixes_issue_2797 branch from 783ffed to 0fa9e46 Compare February 12, 2026 07:20

sachinvmurthy requested a review from roshkhatri February 12, 2026 07:24

sachinvmurthy force-pushed the bug/fixes_issue_2797 branch from 70c3cb4 to ca49ee3 Compare February 12, 2026 07:28

roshkhatri added the release-notes This issue should get a line item in the release notes label Feb 12, 2026

sachinvmurthy requested a review from zuiderkwast February 12, 2026 22:59

roshkhatri reviewed Feb 13, 2026

View reviewed changes

tests/unit/moduleapi/subcommands.tcl Outdated Show resolved Hide resolved

sachinvmurthy force-pushed the bug/fixes_issue_2797 branch from c2b95c7 to eaef2ff Compare February 13, 2026 23:30

sachinvmurthy requested a review from roshkhatri February 13, 2026 23:31

sachinvmurthy force-pushed the bug/fixes_issue_2797 branch from 80a4520 to 46f3b97 Compare February 17, 2026 04:05

sachinvmurthy force-pushed the bug/fixes_issue_2797 branch from c658327 to bfe06fa Compare February 17, 2026 19:20

zuiderkwast added this to Valkey 9.0 Feb 17, 2026

zuiderkwast moved this to In Progress in Valkey 9.0 Feb 17, 2026

roshkhatri approved these changes Feb 17, 2026

View reviewed changes

madolson approved these changes Feb 17, 2026

View reviewed changes

madolson added this to Valkey 8.0, Valkey 8.1 and Valkey 7.2 Feb 17, 2026

madolson moved this to To be backported in Valkey 8.1 Feb 17, 2026

madolson moved this to To be backported in Valkey 8.0 Feb 17, 2026

madolson moved this to To be backported in Valkey 7.2 Feb 17, 2026

madolson merged commit 385895a into valkey-io:unstable Feb 17, 2026
56 checks passed

github-project-automation bot moved this from In Progress to To be backported in Valkey 9.0 Feb 17, 2026

roshkhatri moved this from To be backported to 9.0.3 in Valkey 9.0 Feb 17, 2026

roshkhatri moved this from To be backported to 8.0.7 (WIP) in Valkey 8.0 Feb 18, 2026

roshkhatri moved this from To be backported to 7.2.12 in Valkey 7.2 Feb 18, 2026

roshkhatri moved this from To be backported to 8.1.6 WIP in Valkey 8.1 Feb 18, 2026

Comments

Conversation

sachinvmurthy commented Feb 3, 2026

Uh oh!

sachinvmurthy commented Feb 3, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

madolson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

codecov bot commented Feb 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

zuiderkwast commented Feb 9, 2026

Uh oh!

roshkhatri left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

roshkhatri commented Feb 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

roshkhatri commented Feb 16, 2026

Uh oh!

roshkhatri commented Feb 17, 2026

Uh oh!

roshkhatri left a comment

Choose a reason for hiding this comment

Uh oh!

madolson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov bot commented Feb 5, 2026 •

edited

Loading

roshkhatri commented Feb 9, 2026 •

edited

Loading