From 90ab91f00b64843f6201bf352bb71aef743b6ca3 Mon Sep 17 00:00:00 2001 From: "zhaozhao.zz" Date: Wed, 2 Aug 2023 15:46:06 +0800 Subject: [PATCH] fix false success and a memory leak for ACL selector with bad parenthesis combination (#12452) When doing merge selector, we should check whether the merge has started (i.e., whether open_bracket_start is -1) every time. Otherwise, encountering an illegal selector pattern could succeed and also cause memory leaks, for example: ``` acl setuser test1 (+PING (+SELECT (+DEL ) ``` The above would leak memory and succeed with only DEL being applied, and would now error after the fix. Co-authored-by: Oran Agra --- src/acl.c | 3 ++- tests/unit/acl-v2.tcl | 9 +++++++++ 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/src/acl.c b/src/acl.c index aa42c58dc..07840406a 100644 --- a/src/acl.c +++ b/src/acl.c @@ -1990,7 +1990,8 @@ sds *ACLMergeSelectorArguments(sds *argv, int argc, int *merged_argc, int *inval for (int j = 0; j < argc; j++) { char *op = argv[j]; - if (op[0] == '(' && op[sdslen(op) - 1] != ')') { + if (open_bracket_start == -1 && + (op[0] == '(' && op[sdslen(op) - 1] != ')')) { selector = sdsdup(argv[j]); open_bracket_start = j; continue; diff --git a/tests/unit/acl-v2.tcl b/tests/unit/acl-v2.tcl index af23d745e..b259c2716 100644 --- a/tests/unit/acl-v2.tcl +++ b/tests/unit/acl-v2.tcl @@ -47,6 +47,15 @@ start_server {tags {"acl external:skip"}} { catch {r ACL SETUSER selector-syntax on (&* &fail)} e assert_match "*ERR Error in ACL SETUSER modifier '(*)*Adding a pattern after the*" $e + catch {r ACL SETUSER selector-syntax on (+PING (+SELECT (+DEL} e + assert_match "*ERR Unmatched parenthesis in acl selector*" $e + + catch {r ACL SETUSER selector-syntax on (+PING (+SELECT (+DEL ) ) ) } e + assert_match "*ERR Error in ACL SETUSER modifier*" $e + + catch {r ACL SETUSER selector-syntax on (+PING (+SELECT (+DEL ) } e + assert_match "*ERR Error in ACL SETUSER modifier*" $e + assert_equal "" [r ACL GETUSER selector-syntax] }