fix(acl): prevent NULL pointer dereference on malformed selector in ACL file#4214
fix(acl): prevent NULL pointer dereference on malformed selector in ACL file#4214magic-peach wants to merge 1 commit into
Conversation
…CL file Signed-off-by: Akanksha Trehun <akankshatrehun@gmail.com>
📝 WalkthroughWalkthrough
ChangesACL loading
Estimated code review effort: 1 (Trivial) | ~5 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/acl.c (1)
2583-2584: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd a regression test for malformed ACL files.
The cleanup and
continuepath is correct, but please add atests/Tcl integration test that loads an ACL file containing an unmatched selector parenthesis, verifies the expected error, confirms the server remains responsive, and checks that the previous ACL rules remain active.As per coding guidelines: “Code changes should include relevant tests when the repository has a matching test location.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/acl.c` around lines 2583 - 2584, Add a Tcl integration regression test under tests/ for loading an ACL file with an unmatched selector parenthesis. Assert the expected load error, verify the server remains responsive afterward, and confirm the previously loaded ACL rules are still enforced; keep the existing cleanup and continue behavior unchanged.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/acl.c`:
- Around line 2583-2584: Add a Tcl integration regression test under tests/ for
loading an ACL file with an unmatched selector parenthesis. Assert the expected
load error, verify the server remains responsive afterward, and confirm the
previously loaded ACL rules are still enforced; keep the existing cleanup and
continue behavior unchanged.
![valkey-review-bot[bot]](https://avatars.githubusercontent.com/in/3633497?s=60&v=4){kind=small}
There was a problem hiding this comment.
The fix is correct: ACLMergeSelectorArguments frees the partial result but leaves *merged_argc at the partial count when it returns NULL (src/acl.c:2286-2291), so the old fall-through dereferenced the NULL acl_args whenever at least one complete argument preceded the unmatched parenthesis (e.g. user u on (+get). The added cleanup matches every sibling error path in this loop, and the created user is discarded with the fresh Users rax on the error exit, so there is no leak. Two minor notes inline.
| @@ -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, | |||
There was a problem hiding this comment.
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....
| 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, |
| errors = sdscatprintf(errors, "%s:%d: Unmatched parenthesis in selector definition.", server.acl_filename, | ||
| linenum); | ||
| sdsfreesplitres(argv, argc); | ||
| continue; |
There was a problem hiding this comment.
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.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## unstable #4214 +/- ##
============================================
+ Coverage 76.81% 76.87% +0.06%
============================================
Files 162 162
Lines 81455 81457 +2
============================================
+ Hits 62570 62621 +51
+ Misses 18885 18836 -49
🚀 New features to boost your workflow:
|
1 participant
Summary
Bug
When
ACLMergeSelectorArguments()returns NULL due to an unmatched parenthesis, the error message is set but execution continues to a for loop that dereferencesacl_args(which is NULL), causing a server crash.Fix
Add
continueafter the error message to skip the processing loop when acl_args is NULL.