Skip to content

fix(acl): prevent NULL pointer dereference on malformed selector in ACL file #4214

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
magic-peach wants to merge 1 commit into
base: unstable
Choose a base branch
from
Open

fix(acl): prevent NULL pointer dereference on malformed selector in ACL file #4214

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/acl.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2580,6 +2580,8 @@ static sds ACLLoadFromFile(const char *filename) {
if (!acl_args) {
errors = sdscatprintf(errors, "%s:%d: Unmatched parenthesis in selector definition.", server.acl_filename,

@valkey-review-bot valkey-review-bot Bot Jul 19, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Every other error message in this function ends with a trailing ". " separator (src/acl.c:2537, 2559, 2569, 2600); this one doesn't. Now that this path no longer crashes, the message actually surfaces, and the missing space makes the concatenated output run together with the next line's error or with the final suffix appended at src/acl.c:2679: ...selector definition.WARNING: ACL errors detected....

Suggested change
errors = sdscatprintf(errors, "%s:%d: Unmatched parenthesis in selector definition.", server.acl_filename,
errors = sdscatprintf(errors, "%s:%d: Unmatched parenthesis in selector definition. ", server.acl_filename,

linenum);
sdsfreesplitres(argv, argc);
continue;

@valkey-review-bot valkey-review-bot Bot Jul 19, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This deserves a regression test, since the crash is trivially reachable from an aclfile: ACLMergeSelectorArguments leaves *merged_argc at the partial count on the NULL return (src/acl.c:2286-2291), so any line with a complete argument before the unmatched open paren (e.g. user paren on (+get) segfaulted in the loop below. tests/unit/acl.tcl already has aclfile-backed start_server blocks that rewrite the file and reload (e.g. the duplicate-users test around tests/unit/acl.tcl:1316) — append a user paren on (+get line to the acl file, catch {r ACL LOAD} err, assert_match "*Unmatched parenthesis*" $err, and verify the server still answers r PING.

}

int syntax_error = 0;
Expand Down
Loading