From c43ff248f94266cfc93e300a2d3d163ed805e55b Mon Sep 17 00:00:00 2001 From: "Dmitry V. Levin" Date: Thu, 16 Mar 2023 08:00:00 +0000 Subject: [PATCH 1/3] udev-rules: fix matching of token types that support alternative patterns For those token types that support matching of alternative patterns, their token values are interpreted as nulstr, so make sure the parser does the right thing and makes these token values terminated by two subsequent NULs so they could be safely interpreted as nulstr. Before this fix, the following rules would result to "echo foo" invocation: ENV{foo}=", RUN" ENV{foo}=="bar", RUN+="echo foo" because the value of `ENV{foo}` is treated as nulstr, and it used to match against alternative patterns, in this case `bar`, `, RUN`, and `="echo foo`. Fixes: 25de7aa7b90c ("udev: modernize udev-rules.c") --- src/shared/udev-util.c | 19 ++++++++++++++----- src/test/test-udev-util.c | 5 +++++ 2 files changed, 19 insertions(+), 5 deletions(-) diff --git a/src/shared/udev-util.c b/src/shared/udev-util.c index a4b17be097..646eaf9135 100644 --- a/src/shared/udev-util.c +++ b/src/shared/udev-util.c @@ -347,11 +347,10 @@ int udev_rule_parse_value(char *str, char **ret_value, char **ret_endpos) { str += is_escaped; if (str[0] != '"') return -EINVAL; - str++; if (!is_escaped) { /* unescape double quotation '\"'->'"' */ - for (i = j = str; *i != '"'; i++, j++) { + for (j = str, i = str + 1; *i != '"'; i++, j++) { if (*i == '\0') return -EINVAL; if (i[0] == '\\' && i[1] == '"') @@ -359,12 +358,17 @@ int udev_rule_parse_value(char *str, char **ret_value, char **ret_endpos) { *j = *i; } j[0] = '\0'; + /* + * The return value must be terminated by two subsequent NULs + * so it could be safely interpreted as nulstr. + */ + j[1] = '\0'; } else { _cleanup_free_ char *unescaped = NULL; ssize_t l; /* find the end position of value */ - for (i = str; *i != '"'; i++) { + for (i = str + 1; *i != '"'; i++) { if (i[0] == '\\') i++; if (*i == '\0') @@ -372,12 +376,17 @@ int udev_rule_parse_value(char *str, char **ret_value, char **ret_endpos) { } i[0] = '\0'; - l = cunescape_length(str, i - str, 0, &unescaped); + l = cunescape_length(str + 1, i - (str + 1), 0, &unescaped); if (l < 0) return l; - assert(l <= i - str); + assert(l <= i - (str + 1)); memcpy(str, unescaped, l + 1); + /* + * The return value must be terminated by two subsequent NULs + * so it could be safely interpreted as nulstr. + */ + str[l + 1] = '\0'; } *ret_value = str; diff --git a/src/test/test-udev-util.c b/src/test/test-udev-util.c index 1db2dad4ff..4be3694e9e 100644 --- a/src/test/test-udev-util.c +++ b/src/test/test-udev-util.c @@ -24,6 +24,11 @@ static void test_udev_rule_parse_value_one(const char *in, const char *expected_ } else { assert_se(streq_ptr(value, expected_value)); assert_se(endpos == str + strlen(in)); + /* + * The return value must be terminated by two subsequent NULs + * so it could be safely interpreted as nulstr. + */ + assert_se(value[strlen(value) + 1] == '\0'); } } From 8c499a61c46eb434db04d3ee4b116a0a755b3797 Mon Sep 17 00:00:00 2001 From: "Dmitry V. Levin" Date: Sun, 19 Mar 2023 08:00:00 +0000 Subject: [PATCH 2/3] Revert "udev: prepare memory for extra NUL termination for NULSTR" This reverts commit cd3c8a117ccf3505e49d34324473e2175ef0a9ce which was papering over the bug instead of a proper fix made by the previous commit. --- src/udev/udev-rules.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/udev/udev-rules.c b/src/udev/udev-rules.c index 71c0949220..a1c17a24e8 100644 --- a/src/udev/udev-rules.c +++ b/src/udev/udev-rules.c @@ -1148,9 +1148,7 @@ static int rule_add_line(UdevRuleFile *rule_file, const char *line_str, unsigned if (isempty(line_str)) return 0; - /* We use memdup_suffix0() here, since we want to add a second NUL byte to the end, since possibly - * some parsers might turn this into a "nulstr", which requires an extra NUL at the end. */ - line = memdup_suffix0(line_str, strlen(line_str) + 1); + line = strdup(line_str); if (!line) return log_oom(); From 5004aa84ac33887fa87c15caf41e87b7e857a331 Mon Sep 17 00:00:00 2001 From: "Dmitry V. Levin" Date: Fri, 17 Mar 2023 08:00:00 +0000 Subject: [PATCH 3/3] udev-rules: fix check for conflicting and duplicate expressions Fix check for conflicting and duplicate expressions of types that support alternative patterns. Fixes: 3ec58d0cd8f6 ("udev-rules: check for conflicting and duplicate expressions") --- src/udev/udev-rules.c | 28 +++++++++++++++++++++++----- test/units/testsuite-17.11.sh | 8 ++++++++ 2 files changed, 31 insertions(+), 5 deletions(-) diff --git a/src/udev/udev-rules.c b/src/udev/udev-rules.c index a1c17a24e8..f708f79c9a 100644 --- a/src/udev/udev-rules.c +++ b/src/udev/udev-rules.c @@ -477,6 +477,10 @@ static UdevRuleSubstituteType rule_get_substitution_type(const char *str) { return SUBST_TYPE_PLAIN; } +static bool type_has_nulstr_value(UdevRuleTokenType type) { + return type < TK_M_TEST || type == TK_M_RESULT; +} + static int rule_line_add_token(UdevRuleLine *rule_line, UdevRuleTokenType type, UdevRuleOperatorType op, char *value, void *data) { _cleanup_(udev_rule_token_freep) UdevRuleToken *token = NULL; UdevRuleMatchType match_type = _MATCH_TYPE_INVALID; @@ -505,7 +509,7 @@ static int rule_line_add_token(UdevRuleLine *rule_line, UdevRuleTokenType type, else match_type = MATCH_TYPE_PLAIN; - if (type < TK_M_TEST || type == TK_M_RESULT) { + if (type_has_nulstr_value(type)) { /* Convert value string to nulstr. */ bool bar = true, empty = false; char *a, *b; @@ -1361,18 +1365,32 @@ static bool token_type_and_data_eq(const UdevRuleToken *a, const UdevRuleToken * (token_data_is_string(a->type) ? streq_ptr(a->data, b->data) : (a->data == b->data)); } -static bool token_value_eq(const UdevRuleToken *a, const UdevRuleToken *b) { +static bool nulstr_eq(const char *a, const char *b) { + NULSTR_FOREACH(i, a) + if (!nulstr_contains(b, i)) + return false; + + NULSTR_FOREACH(i, b) + if (!nulstr_contains(a, i)) + return false; + + return true; +} + +static bool token_type_and_value_eq(const UdevRuleToken *a, const UdevRuleToken *b) { assert(a); assert(b); - if (a->match_type != b->match_type) + if (a->type != b->type || + a->match_type != b->match_type) return false; /* token value is ignored for certain match types */ if (IN_SET(a->match_type, MATCH_TYPE_EMPTY, MATCH_TYPE_SUBSYSTEM)) return true; - return streq_ptr(a->value, b->value); + return type_has_nulstr_value(a->type) ? nulstr_eq(a->value, b->value) : + streq_ptr(a->value, b->value); } static bool conflicting_op(UdevRuleOperatorType a, UdevRuleOperatorType b) { @@ -1387,7 +1405,7 @@ static bool tokens_eq(const UdevRuleToken *a, const UdevRuleToken *b) { return a->attr_subst_type == b->attr_subst_type && a->attr_match_remove_trailing_whitespace == b->attr_match_remove_trailing_whitespace && - token_value_eq(a, b) && + token_type_and_value_eq(a, b) && token_type_and_data_eq(a, b); } diff --git a/test/units/testsuite-17.11.sh b/test/units/testsuite-17.11.sh index 50e1820957..5f067f02b2 100755 --- a/test/units/testsuite-17.11.sh +++ b/test/units/testsuite-17.11.sh @@ -277,14 +277,22 @@ test_syntax_error 'LABEL="b"' 'LABEL="b" is unused.' test_syntax_error 'a="b"' "Invalid key 'a'" test_syntax_error 'KERNEL=="", KERNEL=="?*", NAME="a"' 'conflicting match expressions, the line takes no effect' test_syntax_error 'KERNEL=="abc", KERNEL!="abc", NAME="b"' 'conflicting match expressions, the line takes no effect' +test_syntax_error 'KERNEL=="|a|b", KERNEL!="b|a|", NAME="c"' 'conflicting match expressions, the line takes no effect' # shellcheck disable=SC2016 test_syntax_error 'ENV{DISKSEQ}=="?*", ENV{DEVTYPE}!="partition", ENV{DISKSEQ}!="?*" ENV{ID_IGNORE_DISKSEQ}!="1", SYMLINK+="disk/by-diskseq/$env{DISKSEQ}"' \ 'conflicting match expressions, the line takes no effect' test_syntax_error 'KERNEL!="", KERNEL=="?*", NAME="a"' 'duplicate expressions' +test_syntax_error 'KERNEL=="|a|b", KERNEL=="b|a|", NAME="c"' 'duplicate expressions' # shellcheck disable=SC2016 test_syntax_error 'ENV{DISKSEQ}=="?*", ENV{DEVTYPE}!="partition", ENV{DISKSEQ}=="?*" ENV{ID_IGNORE_DISKSEQ}!="1", SYMLINK+="disk/by-diskseq/$env{DISKSEQ}"' \ 'duplicate expressions' +cat >"${rules}" <<'EOF' +KERNEL=="a|b", KERNEL=="a|c", NAME="d" +KERNEL=="a|b", KERNEL!="a|c", NAME="d" +EOF +assert_0 "${rules}" + echo 'GOTO="a"' >"${rules}" cat >"${exp}" <