Bug/fixes issue 2797

src/acl.c

@@ -192,6 +192,7 @@ static void ACLResetFirstArgs(aclSelector *selector);
 static void ACLAddAllowedFirstArg(aclSelector *selector, unsigned long id, const char *sub);
 static void ACLFreeLogEntry(void *le);
 static int ACLSetSelector(aclSelector *selector, const char *op, size_t oplen);
+static struct serverCommand *ACLLookupCommand(const char *name);
 
 /* The length of the string representation of a hashed password. */
 #define HASH_PASSWORD_LEN (SHA256_BLOCK_SIZE * 2)
@@ -732,6 +733,57 @@ static int ACLSetSelectorCategory(aclSelector *selector, const char *category, i
     return C_OK;
 }
 
+/* Check if any ACL user has command rules referencing the specified module.
+ * If rule_out is not NULL, it will be set to a duplicate of the first matching
+ * rule.
+ * Returns 1 if any rules are found, 0 otherwise. */
+int ACLModuleHasCommandRules(const struct ValkeyModule *module, sds *rule_out) {
+    raxIterator ri;
+    raxStart(&ri, Users);
+    raxSeek(&ri, "^", NULL, 0);
+    while (raxNext(&ri)) {
+        user *u = ri.data;
+        listIter li;
+        listNode *ln;
+        listRewind(u->selectors, &li);
+        while ((ln = listNext(&li)) != NULL) {
+            aclSelector *selector = listNodeValue(ln);
+            if (sdslen(selector->command_rules) == 0) continue;
+
+            int argc = 0;
+            sds *argv = sdssplitargs(selector->command_rules, &argc);
+            if (!argv) continue;
+
+            for (int i = 0; i < argc; i++) {
+                sds rule = argv[i];
+                if (rule[0] != '+' && rule[0] != '-') continue;
+                if (rule[1] == '@') continue;
+
+                struct serverCommand *cmd = ACLLookupCommand(rule + 1);
+                if (!cmd) {
+                    const char *subsep = strchr(rule + 1, '|');
+                    if (subsep) {
+                        size_t base_len = (size_t)(subsep - (rule + 1));
+                        sds base = sdsnewlen(rule + 1, base_len);
+                        cmd = ACLLookupCommand(base);
+                        sdsfree(base);
+                    }
+                }
+                if (!cmd || !(cmd->flags & CMD_MODULE)) continue;
+                if (moduleFromCommand(cmd) != module) continue;
+
+                if (rule_out) *rule_out = sdsdup(rule);
+                sdsfreesplitres(argv, argc);
+                raxStop(&ri);
+                return 1;
+            }
+            sdsfreesplitres(argv, argc);
+        }
+    }
+    raxStop(&ri);
+    return 0;
+}
+
 /* This function returns an SDS string representing the specified selector ACL
  * rules related to command execution, in the same format you could set them
  * back using ACL SETUSER. The function will return just the set of rules needed

src/module.c

@@ -7026,6 +7026,13 @@ const char *moduleNameFromCommand(struct serverCommand *cmd) {
     return cp->module->name;
 }
 
+ValkeyModule *moduleFromCommand(struct serverCommand *cmd) {
+    serverAssert(cmd->proc == ValkeyModuleCommandDispatcher);
+
+    ValkeyModuleCommand *cp = cmd->module_cmd;
+    return cp->module;
+}
+
 /* Create a copy of a module type value using the copy callback. If failed
  * or not supported, produce an error reply and return NULL.
  */
@@ -12921,6 +12928,18 @@ static int moduleUnloadInternal(struct ValkeyModule *module, const char **errmsg
         return C_ERR;
     }
 
+    sds acl_rule = NULL;
+    if (ACLModuleHasCommandRules(module, &acl_rule)) {
+        serverLog(LL_WARNING,
+                  "Module %s unload blocked: An ACL user has reference to rule '%s'",
+                  module->name,
+                  acl_rule ? acl_rule : "unknown");
+        if (acl_rule) sdsfree(acl_rule);
+        *errmsg = "one or more ACL users reference commands from this module. "
+                  "Remove those ACL rules before unloading";
+        return C_ERR;
+    }
+
     /* Give module a chance to clean up. */
     const char *onUnloadNames[] = {"ValkeyModule_OnUnload", "RedisModule_OnUnload"};
     int (*onunload)(void *) = NULL;

src/module.h

@@ -193,6 +193,7 @@ moduleType *moduleTypeLookupModuleByNameIgnoreCase(const char *name);
 void moduleTypeNameByID(char *name, uint64_t moduleid);
 const char *moduleTypeModuleName(moduleType *mt);
 const char *moduleNameFromCommand(struct serverCommand *cmd);
+ValkeyModule *moduleFromCommand(struct serverCommand *cmd);
 void moduleCallCommandUnblockedHandler(client *c);
 int isModuleClientUnblocked(client *c);
 void unblockClientFromModule(client *c);

src/server.h

@@ -112,6 +112,8 @@ typedef struct serverObject robj;
 #include "crc64.h"
 
 struct hdr_histogram;
+struct ValkeyModule;
+
 
 /* helpers */
 #define numElements(x) (sizeof(x) / sizeof((x)[0]))
@@ -3277,6 +3279,7 @@ int isMutuallyExclusiveChildType(int type);
 extern rax *Users;
 extern user *DefaultUser;
 void ACLInit(void);
+int ACLModuleHasCommandRules(const struct ValkeyModule *module, sds *rule_out);
 /* Return values for ACLCheckAllPerm(). */
 #define ACL_OK 0                    /* Permission granted */
 #define ACL_DENIED_DB 1             /* Database access denied */

tests/unit/moduleapi/aclcheck.tcl

@@ -184,6 +184,44 @@ start_server {tags {"modules acl"}} {
     }
 }
 
+set subcommandsmodule [file normalize tests/modules/subcommands.so]
+start_server {tags {"modules acl"}} {
+    r module load $subcommandsmodule
+
+    test {Module unload blocked by ACL subcommand rule} {
+        r ACL SETUSER subcmduser on nopass +subcommands.sub|get_fullname
+        catch {r module unload subcommands} e
+        assert_match {*one or more ACL users reference commands from this module*} $e
+        r ACL DELUSER subcmduser
+    }
+
+    test {Module unload blocked by ACL base command rule} {
+        r ACL SETUSER basecmduser on nopass +subcommands.parent_get_fullname
+        catch {r module unload subcommands} e
+        assert_match {*one or more ACL users reference commands from this module*} $e
+        r ACL DELUSER basecmduser
+    }
+
+    test {Module unload blocked by ACL deny rule} {
+        r ACL SETUSER denycmduser on nopass -subcommands.parent_get_fullname
+        catch {r module unload subcommands} e
+        assert_match {*one or more ACL users reference commands from this module*} $e
+        r ACL DELUSER denycmduser
+    }
+
+    test {Module unload blocked by ACL selector rule} {
+        r ACL SETUSER selcmduser on nopass (+subcommands.parent_get_fullname)
+        catch {r module unload subcommands} e
+        assert_match {*one or more ACL users reference commands from this module*} $e
+        r ACL DELUSER selcmduser
+    }
+
+    test {Unload the module - subcommands} {
+        r ACL DELUSER subcmduser basecmduser denycmduser selcmduser
+        assert_equal {OK} [r module unload subcommands]
+    }
+}
+
 start_server {tags {"modules check access for keys with prefix"}} {
     r module load $testmodule