From 4c610884979bb79ef2eb278369aee6089a9660ca Mon Sep 17 00:00:00 2001 From: "Dmitry V. Levin" Date: Tue, 21 Mar 2023 08:00:00 +0000 Subject: [PATCH 1/8] rules: remove stray whitespace before comma between tokens --- rules.d/70-camera.rules | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rules.d/70-camera.rules b/rules.d/70-camera.rules index b1053f2cbb..b87096ece0 100644 --- a/rules.d/70-camera.rules +++ b/rules.d/70-camera.rules @@ -2,7 +2,7 @@ ACTION=="remove", GOTO="camera_end" -SUBSYSTEM=="video4linux", ENV{ID_BUS}="usb" , \ +SUBSYSTEM=="video4linux", ENV{ID_BUS}="usb", \ IMPORT{builtin}="hwdb 'camera:usb:v$env{ID_VENDOR_ID}p$env{ID_MODEL_ID}:name:$attr{name}:'", \ GOTO="camera_end" From 519044f565cf32f4ebd0ecca5eb162555aa006b2 Mon Sep 17 00:00:00 2001 From: "Dmitry V. Levin" Date: Sat, 25 Mar 2023 08:00:00 +0000 Subject: [PATCH 2/8] rules: add whitespace after comma before the line continuation --- rules.d/60-persistent-storage.rules.in | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/rules.d/60-persistent-storage.rules.in b/rules.d/60-persistent-storage.rules.in index 76621a467d..51589686af 100644 --- a/rules.d/60-persistent-storage.rules.in +++ b/rules.d/60-persistent-storage.rules.in @@ -41,7 +41,7 @@ KERNEL=="nvme*[0-9]n*[0-9]", ENV{DEVTYPE}=="disk", ATTRS{nsid}=="?*", ENV{ID_NSI # obsolete symlink that might get overridden on adding a new nvme controller, kept for backward compatibility KERNEL=="nvme*[0-9]n*[0-9]", ENV{DEVTYPE}=="disk", ENV{ID_MODEL}=="?*", ENV{ID_SERIAL_SHORT}=="?*", \ OPTIONS="string_escape=replace", ENV{ID_SERIAL}="$env{ID_MODEL}_$env{ID_SERIAL_SHORT}", SYMLINK+="disk/by-id/nvme-$env{ID_SERIAL}" -KERNEL=="nvme*[0-9]n*[0-9]", ENV{DEVTYPE}=="disk", ENV{ID_MODEL}=="?*", ENV{ID_SERIAL_SHORT}=="?*", ENV{ID_NSID}=="?*",\ +KERNEL=="nvme*[0-9]n*[0-9]", ENV{DEVTYPE}=="disk", ENV{ID_MODEL}=="?*", ENV{ID_SERIAL_SHORT}=="?*", ENV{ID_NSID}=="?*", \ OPTIONS="string_escape=replace", ENV{ID_SERIAL}="$env{ID_MODEL}_$env{ID_SERIAL_SHORT}_$env{ID_NSID}", SYMLINK+="disk/by-id/nvme-$env{ID_SERIAL}" KERNEL=="nvme*[0-9]n*[0-9]p*[0-9]", ENV{DEVTYPE}=="partition", ATTRS{serial}=="?*", ENV{ID_SERIAL_SHORT}="$attr{serial}" @@ -51,7 +51,7 @@ KERNEL=="nvme*[0-9]n*[0-9]p*[0-9]", ENV{DEVTYPE}=="partition", ATTRS{nsid}=="?*" # obsolete symlink that might get overridden on adding a new nvme controller, kept for backward compatibility KERNEL=="nvme*[0-9]n*[0-9]p*[0-9]", ENV{DEVTYPE}=="partition", ENV{ID_MODEL}=="?*", ENV{ID_SERIAL_SHORT}=="?*", \ OPTIONS="string_escape=replace", ENV{ID_SERIAL}="$env{ID_MODEL}_$env{ID_SERIAL_SHORT}", SYMLINK+="disk/by-id/nvme-$env{ID_SERIAL}-part%n" -KERNEL=="nvme*[0-9]n*[0-9]p*[0-9]", ENV{DEVTYPE}=="partition", ENV{ID_MODEL}=="?*", ENV{ID_SERIAL_SHORT}=="?*", ENV{ID_NSID}=="?*",\ +KERNEL=="nvme*[0-9]n*[0-9]p*[0-9]", ENV{DEVTYPE}=="partition", ENV{ID_MODEL}=="?*", ENV{ID_SERIAL_SHORT}=="?*", ENV{ID_NSID}=="?*", \ OPTIONS="string_escape=replace", ENV{ID_SERIAL}="$env{ID_MODEL}_$env{ID_SERIAL_SHORT}_$env{ID_NSID}", SYMLINK+="disk/by-id/nvme-$env{ID_SERIAL}-part%n" # virtio-blk From d656c496040921da0a292e5239e8661b1086b67b Mon Sep 17 00:00:00 2001 From: "Dmitry V. Levin" Date: Tue, 21 Mar 2023 08:00:00 +0000 Subject: [PATCH 3/8] testsuite-17.11.sh: add commas between tokens --- test/units/testsuite-17.11.sh | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/test/units/testsuite-17.11.sh b/test/units/testsuite-17.11.sh index 88172acde6..19ae35ce8f 100755 --- a/test/units/testsuite-17.11.sh +++ b/test/units/testsuite-17.11.sh @@ -132,7 +132,7 @@ cp "${workdir}/default_output_1_fail" "${exo}" assert_1 "${rules}" { - printf 'RUN+="/bin/true"%8175s\\\n' ' ' + printf 'RUN+="/bin/true",%8174s\\\n' ' ' printf 'RUN+="/bin/false"%8174s\\\n' ' ' echo } >"${rules}" @@ -201,7 +201,7 @@ test_syntax_error 'TAGS{a}=="b"' 'Invalid attribute for TAGS.' test_syntax_error 'TAGS:="a"' 'Invalid operator for TAGS.' test_syntax_error 'SUBSYSTEM{a}=="b"' 'Invalid attribute for SUBSYSTEM.' test_syntax_error 'SUBSYSTEM:="b"' 'Invalid operator for SUBSYSTEM.' -test_syntax_error 'SUBSYSTEM=="bus" NAME="b"' '"bus" must be specified as "subsystem".' +test_syntax_error 'SUBSYSTEM=="bus", NAME="b"' '"bus" must be specified as "subsystem".' test_syntax_error 'SUBSYSTEMS{a}=="b"' 'Invalid attribute for SUBSYSTEMS.' test_syntax_error 'SUBSYSTEMS:="b"' 'Invalid operator for SUBSYSTEMS.' test_syntax_error 'DRIVER{a}=="b"' 'Invalid attribute for DRIVER.' @@ -219,20 +219,20 @@ test_syntax_error 'SYSCTL{a}-="b"' 'Invalid operator for SYSCTL.' test_syntax_error 'SYSCTL{a}+="b"' "SYSCTL key takes '==', '!=', or '=' operator, assuming '='." test_syntax_error 'SYSCTL{a}="%?"' 'Invalid value "%?" for SYSCTL (char 1: invalid substitution type), ignoring.' test_syntax_error 'ATTRS=""' 'Invalid attribute for ATTRS.' -test_syntax_error 'ATTRS{%}=="b" NAME="b"' 'Invalid attribute "%" for ATTRS (char 1: invalid substitution type), ignoring.' +test_syntax_error 'ATTRS{%}=="b", NAME="b"' 'Invalid attribute "%" for ATTRS (char 1: invalid substitution type), ignoring.' test_syntax_error 'ATTRS{a}-="b"' 'Invalid operator for ATTRS.' -test_syntax_error 'ATTRS{device/}!="a" NAME="b"' "'device' link may not be available in future kernels." -test_syntax_error 'ATTRS{../}!="a" NAME="b"' 'Direct reference to parent sysfs directory, may break in future kernels.' +test_syntax_error 'ATTRS{device/}!="a", NAME="b"' "'device' link may not be available in future kernels." +test_syntax_error 'ATTRS{../}!="a", NAME="b"' 'Direct reference to parent sysfs directory, may break in future kernels.' test_syntax_error 'TEST{a}=="b"' "Failed to parse mode 'a': Invalid argument" -test_syntax_error 'TEST{0}=="%" NAME="b"' 'Invalid value "%" for TEST (char 1: invalid substitution type), ignoring.' +test_syntax_error 'TEST{0}=="%", NAME="b"' 'Invalid value "%" for TEST (char 1: invalid substitution type), ignoring.' test_syntax_error 'TEST{0644}="b"' 'Invalid operator for TEST.' test_syntax_error 'PROGRAM{a}=="b"' 'Invalid attribute for PROGRAM.' test_syntax_error 'PROGRAM-="b"' 'Invalid operator for PROGRAM.' -test_syntax_error 'PROGRAM=="%" NAME="b"' 'Invalid value "%" for PROGRAM (char 1: invalid substitution type), ignoring.' +test_syntax_error 'PROGRAM=="%", NAME="b"' 'Invalid value "%" for PROGRAM (char 1: invalid substitution type), ignoring.' test_syntax_error 'IMPORT="b"' 'Invalid attribute for IMPORT.' test_syntax_error 'IMPORT{a}="b"' 'Invalid attribute for IMPORT.' test_syntax_error 'IMPORT{a}-="b"' 'Invalid operator for IMPORT.' -test_syntax_error 'IMPORT{file}=="%" NAME="b"' 'Invalid value "%" for IMPORT (char 1: invalid substitution type), ignoring.' +test_syntax_error 'IMPORT{file}=="%", NAME="b"' 'Invalid value "%" for IMPORT (char 1: invalid substitution type), ignoring.' test_syntax_error 'IMPORT{builtin}!="foo"' 'Unknown builtin command: foo' test_syntax_error 'RESULT{a}=="b"' 'Invalid attribute for RESULT.' test_syntax_error 'RESULT:="b"' 'Invalid operator for RESULT.' @@ -241,7 +241,7 @@ test_syntax_error 'OPTIONS-="b"' 'Invalid operator for OPTIONS.' test_syntax_error 'OPTIONS!="b"' 'Invalid operator for OPTIONS.' test_syntax_error 'OPTIONS+="link_priority=a"' "Failed to parse link priority 'a': Invalid argument" test_syntax_error 'OPTIONS:="log_level=a"' "Failed to parse log level 'a': Invalid argument" -test_syntax_error 'OPTIONS="a" NAME="b"' "Invalid value for OPTIONS key, ignoring: 'a'" +test_syntax_error 'OPTIONS="a", NAME="b"' "Invalid value for OPTIONS key, ignoring: 'a'" test_syntax_error 'OWNER{a}="b"' 'Invalid attribute for OWNER.' test_syntax_error 'OWNER-="b"' 'Invalid operator for OWNER.' test_syntax_error 'OWNER!="b"' 'Invalid operator for OWNER.' @@ -268,8 +268,8 @@ test_syntax_error 'RUN="%"' 'Invalid value "%" for RUN (char 1: invalid substitu test_syntax_error 'RUN{builtin}+="foo"' "Unknown builtin command 'foo', ignoring" test_syntax_error 'GOTO{a}="b"' 'Invalid attribute for GOTO.' test_syntax_error 'GOTO=="b"' 'Invalid operator for GOTO.' -test_syntax_error 'NAME="a" GOTO="b"' 'GOTO="b" has no matching label, ignoring' -test_syntax_error 'GOTO="a" GOTO="b" +test_syntax_error 'NAME="a", GOTO="b"' 'GOTO="b" has no matching label, ignoring' +test_syntax_error 'GOTO="a", GOTO="b" LABEL="a"' 'Contains multiple GOTO keys, ignoring GOTO="b".' test_syntax_error 'LABEL{a}="b"' 'Invalid attribute for LABEL.' test_syntax_error 'LABEL=="b"' 'Invalid operator for LABEL.' @@ -280,12 +280,12 @@ test_syntax_error 'KERNEL=="abc", KERNEL!="abc", NAME="b"' 'conflicting match ex test_syntax_error 'KERNEL=="|a|b", KERNEL!="b|a|", NAME="c"' 'conflicting match expressions, the line takes no effect' test_syntax_error 'KERNEL=="a|b", KERNEL=="c|d|e", NAME="f"' '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}"' \ +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}"' \ +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' From 3b948231028395b187292a795c2be03546422d44 Mon Sep 17 00:00:00 2001 From: "Dmitry V. Levin" Date: Wed, 22 Mar 2023 08:00:00 +0000 Subject: [PATCH 4/8] udev-rules: move udev_rules_parse_file() after udev_check_rule_line() Move udev_rules_parse_file() after udev_check_rule_line() as the former us going to use the latter. --- src/udev/udev-rules.c | 236 +++++++++++++++++++++--------------------- 1 file changed, 118 insertions(+), 118 deletions(-) diff --git a/src/udev/udev-rules.c b/src/udev/udev-rules.c index 4859a142da..1d1c27edd0 100644 --- a/src/udev/udev-rules.c +++ b/src/udev/udev-rules.c @@ -1227,124 +1227,6 @@ static void rule_resolve_goto(UdevRuleFile *rule_file) { } } -int udev_rules_parse_file(UdevRules *rules, const char *filename, UdevRuleFile **ret) { - _cleanup_(udev_rule_file_freep) UdevRuleFile *rule_file = NULL; - _cleanup_free_ char *continuation = NULL, *name = NULL; - _cleanup_fclose_ FILE *f = NULL; - bool ignore_line = false; - unsigned line_nr = 0; - struct stat st; - int r; - - assert(rules); - assert(filename); - - f = fopen(filename, "re"); - if (!f) - return log_warning_errno(errno, "Failed to open %s, ignoring: %m", filename); - - if (fstat(fileno(f), &st) < 0) - return log_warning_errno(errno, "Failed to stat %s, ignoring: %m", filename); - - if (null_or_empty(&st)) { - log_debug("Skipping empty file: %s", filename); - if (ret) - *ret = NULL; - return 0; - } - - r = hashmap_put_stats_by_path(&rules->stats_by_path, filename, &st); - if (r < 0) - return log_warning_errno(errno, "Failed to save stat for %s, ignoring: %m", filename); - - (void) fd_warn_permissions(filename, fileno(f)); - - log_debug("Reading rules file: %s", filename); - - name = strdup(filename); - if (!name) - return log_oom(); - - rule_file = new(UdevRuleFile, 1); - if (!rule_file) - return log_oom(); - - *rule_file = (UdevRuleFile) { - .filename = TAKE_PTR(name), - .rules = rules, - }; - - LIST_APPEND(rule_files, rules->rule_files, rule_file); - - for (;;) { - _cleanup_free_ char *buf = NULL; - size_t len; - char *line; - - r = read_line(f, UDEV_LINE_SIZE, &buf); - if (r < 0) - return r; - if (r == 0) - break; - - line_nr++; - line = skip_leading_chars(buf, NULL); - - /* Lines beginning with '#' are ignored regardless of line continuation. */ - if (line[0] == '#') - continue; - - len = strlen(line); - - if (continuation && !ignore_line) { - if (strlen(continuation) + len >= UDEV_LINE_SIZE) - ignore_line = true; - - if (!strextend(&continuation, line)) - return log_oom(); - - if (!ignore_line) { - line = continuation; - len = strlen(line); - } - } - - if (len > 0 && line[len - 1] == '\\') { - if (ignore_line) - continue; - - line[len - 1] = '\0'; - if (!continuation) { - continuation = strdup(line); - if (!continuation) - return log_oom(); - } - - continue; - } - - if (ignore_line) - log_file_error(rule_file, line_nr, "Line is too long, ignored"); - else if (len > 0) - (void) rule_add_line(rule_file, line, line_nr); - - continuation = mfree(continuation); - ignore_line = false; - } - - if (continuation) - log_file_error(rule_file, line_nr, - "Unexpected EOF after line continuation, line ignored"); - - rule_resolve_goto(rule_file); - - if (ret) - *ret = rule_file; - - TAKE_PTR(rule_file); - return 1; -} - static bool token_data_is_string(UdevRuleTokenType type) { return IN_SET(type, TK_M_ENV, TK_M_CONST, @@ -1476,6 +1358,124 @@ static void udev_check_rule_line(UdevRuleLine *line) { udev_check_conflicts_duplicates(line); } +int udev_rules_parse_file(UdevRules *rules, const char *filename, UdevRuleFile **ret) { + _cleanup_(udev_rule_file_freep) UdevRuleFile *rule_file = NULL; + _cleanup_free_ char *continuation = NULL, *name = NULL; + _cleanup_fclose_ FILE *f = NULL; + bool ignore_line = false; + unsigned line_nr = 0; + struct stat st; + int r; + + assert(rules); + assert(filename); + + f = fopen(filename, "re"); + if (!f) + return log_warning_errno(errno, "Failed to open %s, ignoring: %m", filename); + + if (fstat(fileno(f), &st) < 0) + return log_warning_errno(errno, "Failed to stat %s, ignoring: %m", filename); + + if (null_or_empty(&st)) { + log_debug("Skipping empty file: %s", filename); + if (ret) + *ret = NULL; + return 0; + } + + r = hashmap_put_stats_by_path(&rules->stats_by_path, filename, &st); + if (r < 0) + return log_warning_errno(errno, "Failed to save stat for %s, ignoring: %m", filename); + + (void) fd_warn_permissions(filename, fileno(f)); + + log_debug("Reading rules file: %s", filename); + + name = strdup(filename); + if (!name) + return log_oom(); + + rule_file = new(UdevRuleFile, 1); + if (!rule_file) + return log_oom(); + + *rule_file = (UdevRuleFile) { + .filename = TAKE_PTR(name), + .rules = rules, + }; + + LIST_APPEND(rule_files, rules->rule_files, rule_file); + + for (;;) { + _cleanup_free_ char *buf = NULL; + size_t len; + char *line; + + r = read_line(f, UDEV_LINE_SIZE, &buf); + if (r < 0) + return r; + if (r == 0) + break; + + line_nr++; + line = skip_leading_chars(buf, NULL); + + /* Lines beginning with '#' are ignored regardless of line continuation. */ + if (line[0] == '#') + continue; + + len = strlen(line); + + if (continuation && !ignore_line) { + if (strlen(continuation) + len >= UDEV_LINE_SIZE) + ignore_line = true; + + if (!strextend(&continuation, line)) + return log_oom(); + + if (!ignore_line) { + line = continuation; + len = strlen(line); + } + } + + if (len > 0 && line[len - 1] == '\\') { + if (ignore_line) + continue; + + line[len - 1] = '\0'; + if (!continuation) { + continuation = strdup(line); + if (!continuation) + return log_oom(); + } + + continue; + } + + if (ignore_line) + log_file_error(rule_file, line_nr, "Line is too long, ignored"); + else if (len > 0) + (void) rule_add_line(rule_file, line, line_nr); + + continuation = mfree(continuation); + ignore_line = false; + } + + if (continuation) + log_file_error(rule_file, line_nr, + "Unexpected EOF after line continuation, line ignored"); + + rule_resolve_goto(rule_file); + + if (ret) + *ret = rule_file; + + TAKE_PTR(rule_file); + return 1; +} + unsigned udev_rule_file_get_issues(UdevRuleFile *rule_file) { assert(rule_file); From 9b419eb0b813868ed442e239661f78f8a92222ae Mon Sep 17 00:00:00 2001 From: "Dmitry V. Levin" Date: Wed, 22 Mar 2023 08:00:00 +0000 Subject: [PATCH 5/8] udev-rules: move udev_check_rule_line() invocation Move udev_check_rule_line() invocation from udev_rule_file_get_issues() to udev_rules_parse_file(), invoke udev_check_rule_line() only when udev_rules_parse_file() is called by udevadm verify. Subsequent commits are going to perform more checks invoked from udev_rules_parse_file(). --- src/udev/fuzz-udev-rules.c | 2 +- src/udev/udev-rules.c | 11 ++++++----- src/udev/udev-rules.h | 2 +- src/udev/udevadm-verify.c | 2 +- 4 files changed, 9 insertions(+), 8 deletions(-) diff --git a/src/udev/fuzz-udev-rules.c b/src/udev/fuzz-udev-rules.c index 1d487b94ee..aff176eb1b 100644 --- a/src/udev/fuzz-udev-rules.c +++ b/src/udev/fuzz-udev-rules.c @@ -27,7 +27,7 @@ int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) { fflush(f); assert_se(rules = udev_rules_new(RESOLVE_NAME_EARLY)); - r = udev_rules_parse_file(rules, filename, NULL); + r = udev_rules_parse_file(rules, filename, /* extra_checks = */ false, NULL); log_info_errno(r, "Parsing %s: %m", filename); assert_se(r >= 0 || /* OK */ r == -ENOBUFS); /* line length exceeded */ diff --git a/src/udev/udev-rules.c b/src/udev/udev-rules.c index 1d1c27edd0..23dd31229d 100644 --- a/src/udev/udev-rules.c +++ b/src/udev/udev-rules.c @@ -1358,7 +1358,7 @@ static void udev_check_rule_line(UdevRuleLine *line) { udev_check_conflicts_duplicates(line); } -int udev_rules_parse_file(UdevRules *rules, const char *filename, UdevRuleFile **ret) { +int udev_rules_parse_file(UdevRules *rules, const char *filename, bool extra_checks, UdevRuleFile **ret) { _cleanup_(udev_rule_file_freep) UdevRuleFile *rule_file = NULL; _cleanup_free_ char *continuation = NULL, *name = NULL; _cleanup_fclose_ FILE *f = NULL; @@ -1469,6 +1469,10 @@ int udev_rules_parse_file(UdevRules *rules, const char *filename, UdevRuleFile * rule_resolve_goto(rule_file); + if (extra_checks) + LIST_FOREACH(rule_lines, line, rule_file->rule_lines) + udev_check_rule_line(line); + if (ret) *ret = rule_file; @@ -1479,9 +1483,6 @@ int udev_rules_parse_file(UdevRules *rules, const char *filename, UdevRuleFile * unsigned udev_rule_file_get_issues(UdevRuleFile *rule_file) { assert(rule_file); - LIST_FOREACH(rule_lines, line, rule_file->rule_lines) - udev_check_rule_line(line); - return rule_file->issues; } @@ -1513,7 +1514,7 @@ int udev_rules_load(UdevRules **ret_rules, ResolveNameTiming resolve_name_timing return log_debug_errno(r, "Failed to enumerate rules files: %m"); STRV_FOREACH(f, files) { - r = udev_rules_parse_file(rules, *f, NULL); + r = udev_rules_parse_file(rules, *f, /* extra_checks = */ false, NULL); if (r < 0) log_debug_errno(r, "Failed to read rules file %s, ignoring: %m", *f); } diff --git a/src/udev/udev-rules.h b/src/udev/udev-rules.h index cecc285b2c..9b477fb235 100644 --- a/src/udev/udev-rules.h +++ b/src/udev/udev-rules.h @@ -18,7 +18,7 @@ typedef enum { _ESCAPE_TYPE_INVALID = -EINVAL, } UdevRuleEscapeType; -int udev_rules_parse_file(UdevRules *rules, const char *filename, UdevRuleFile **ret); +int udev_rules_parse_file(UdevRules *rules, const char *filename, bool extra_checks, UdevRuleFile **ret); unsigned udev_rule_file_get_issues(UdevRuleFile *rule_file); UdevRules* udev_rules_new(ResolveNameTiming resolve_name_timing); int udev_rules_load(UdevRules **ret_rules, ResolveNameTiming resolve_name_timing); diff --git a/src/udev/udevadm-verify.c b/src/udev/udevadm-verify.c index 2e947ef830..48aeb8f7e7 100644 --- a/src/udev/udevadm-verify.c +++ b/src/udev/udevadm-verify.c @@ -104,7 +104,7 @@ static int verify_rules_file(UdevRules *rules, const char *fname) { UdevRuleFile *file; int r; - r = udev_rules_parse_file(rules, fname, &file); + r = udev_rules_parse_file(rules, fname, /* extra_checks = */ true, &file); if (r < 0) return log_error_errno(r, "Failed to parse rules file %s: %m", fname); if (r == 0) /* empty file. */ From acc1954a03940b34aab4b814248e9ddf7e49a901 Mon Sep 17 00:00:00 2001 From: "Dmitry V. Levin" Date: Thu, 23 Mar 2023 08:00:00 +0000 Subject: [PATCH 6/8] udev-rules: check token delimiters When udev_rules_parse_file() is called by udevadm verify, issue warnings about the following conditions in udev rules: * the first token in the rule is preceded with a comma * the last token in the rule is followed by a comma * there is no comma between tokens * there is no whitespace between tokens * there is more than a single comma between tokens * there is whitespace between a token and a comma * there is no whitespace after comma --- src/udev/udev-rules.c | 52 +++++++++++++++++++++++++++++++++-- test/units/testsuite-17.11.sh | 31 +++++++++++++++++++++ 2 files changed, 81 insertions(+), 2 deletions(-) diff --git a/src/udev/udev-rules.c b/src/udev/udev-rules.c index 23dd31229d..ecf5989312 100644 --- a/src/udev/udev-rules.c +++ b/src/udev/udev-rules.c @@ -1069,6 +1069,51 @@ static UdevRuleOperatorType parse_operator(const char *op) { return _OP_TYPE_INVALID; } +static void check_token_delimiters(UdevRuleLine *rule_line, const char *line) { + assert(rule_line); + + size_t n_comma = 0; + bool ws_before_comma = false, ws_after_comma = false; + const char *p; + + for (p = line; !isempty(p); ++p) { + if (*p == ',') + ++n_comma; + else if (strchr(WHITESPACE, *p)) { + if (n_comma > 0) + ws_after_comma = true; + else + ws_before_comma = true; + } else + break; + } + + if (line == rule_line->line) { + /* this is the first token of the rule */ + if (n_comma > 0) + log_line_warning(rule_line, "Stray leading comma."); + } else if (isempty(p)) { + /* there are no more tokens in the rule */ + if (n_comma > 0) + log_line_warning(rule_line, "Stray trailing comma."); + } else { + /* single comma is expected */ + if (n_comma == 0) + log_line_warning(rule_line, "A comma between tokens is expected."); + else if (n_comma > 1) + log_line_warning(rule_line, "More than one comma between tokens."); + + /* whitespace after comma is expected */ + if (n_comma > 0) { + if (ws_before_comma) + log_line_warning(rule_line, "Stray whitespace before comma."); + if (!ws_after_comma) + log_line_warning(rule_line, "Whitespace after comma is expected."); + } else if (!ws_before_comma && !ws_after_comma) + log_line_warning(rule_line, "Whitespace between tokens is expected."); + } +} + static int parse_line(char **line, char **ret_key, char **ret_attr, UdevRuleOperatorType *ret_op, char **ret_value) { char *key_begin, *key_end, *attr, *tmp; UdevRuleOperatorType op; @@ -1140,7 +1185,7 @@ static void sort_tokens(UdevRuleLine *rule_line) { } } -static int rule_add_line(UdevRuleFile *rule_file, const char *line_str, unsigned line_nr) { +static int rule_add_line(UdevRuleFile *rule_file, const char *line_str, unsigned line_nr, bool extra_checks) { _cleanup_(udev_rule_line_freep) UdevRuleLine *rule_line = NULL; _cleanup_free_ char *line = NULL; char *p; @@ -1172,6 +1217,9 @@ static int rule_add_line(UdevRuleFile *rule_file, const char *line_str, unsigned char *key, *attr, *value; UdevRuleOperatorType op; + if (extra_checks) + check_token_delimiters(rule_line, p); + r = parse_line(&p, &key, &attr, &op, &value); if (r < 0) return log_line_error_errno(rule_line, r, "Invalid key/value pair, ignoring."); @@ -1457,7 +1505,7 @@ int udev_rules_parse_file(UdevRules *rules, const char *filename, bool extra_che if (ignore_line) log_file_error(rule_file, line_nr, "Line is too long, ignored"); else if (len > 0) - (void) rule_add_line(rule_file, line, line_nr); + (void) rule_add_line(rule_file, line, line_nr, extra_checks); continuation = mfree(continuation); ignore_line = false; diff --git a/test/units/testsuite-17.11.sh b/test/units/testsuite-17.11.sh index 19ae35ce8f..c7c793e71b 100755 --- a/test/units/testsuite-17.11.sh +++ b/test/units/testsuite-17.11.sh @@ -287,6 +287,15 @@ test_syntax_error 'KERNEL=="|a|b", KERNEL=="b|a|", NAME="c"' 'duplicate expressi # 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' +test_syntax_error ',ACTION=="a", NAME="b"' 'Stray leading comma.' +test_syntax_error ' ,ACTION=="a", NAME="b"' 'Stray leading comma.' +test_syntax_error ', ACTION=="a", NAME="b"' 'Stray leading comma.' +test_syntax_error 'ACTION=="a", NAME="b",' 'Stray trailing comma.' +test_syntax_error 'ACTION=="a", NAME="b", ' 'Stray trailing comma.' +test_syntax_error 'ACTION=="a" NAME="b"' 'A comma between tokens is expected.' +test_syntax_error 'ACTION=="a",, NAME="b"' 'More than one comma between tokens.' +test_syntax_error 'ACTION=="a" , NAME="b"' 'Stray whitespace before comma.' +test_syntax_error 'ACTION=="a",NAME="b"' 'Whitespace after comma is expected.' cat >"${rules}" <<'EOF' KERNEL=="a|b", KERNEL=="a|c", NAME="d" @@ -348,6 +357,28 @@ EOF cp "${workdir}/default_output_1_fail" "${exo}" assert_1 "${rules}" +cat >"${rules}" <<'EOF' +ACTION=="a"NAME="b" +EOF +cat >"${exp}" <"${rules}" <<'EOF' +ACTION=="a" ,NAME="b" +EOF +cat >"${exp}" <"${workdir}/${exp}" cd - From 860e2a11154b4065691c882dcf05af3f5a14485a Mon Sep 17 00:00:00 2001 From: "Dmitry V. Levin" Date: Fri, 24 Mar 2023 08:00:00 +0000 Subject: [PATCH 7/8] udev-rules: extend the check for conflicting expressions Log an error when a rule line contains the following kind of conflicting match expressions: KEY=="foo*", KEY=="bar*" --- src/udev/udev-rules.c | 27 ++++++++++++++++++++++----- test/units/testsuite-17.11.sh | 4 ++++ 2 files changed, 26 insertions(+), 5 deletions(-) diff --git a/src/udev/udev-rules.c b/src/udev/udev-rules.c index ecf5989312..c234c265ee 100644 --- a/src/udev/udev-rules.c +++ b/src/udev/udev-rules.c @@ -1348,17 +1348,34 @@ static bool nulstr_tokens_conflict(const UdevRuleToken *a, const UdevRuleToken * a->op == b->op && a->op == OP_MATCH && a->match_type == b->match_type && - a->match_type == MATCH_TYPE_PLAIN && a->attr_subst_type == b->attr_subst_type && a->attr_match_remove_trailing_whitespace == b->attr_match_remove_trailing_whitespace && token_type_and_data_eq(a, b))) return false; - NULSTR_FOREACH(i, a->value) - if (nulstr_contains(b->value, i)) - return false; + if (a->match_type == MATCH_TYPE_PLAIN) { + NULSTR_FOREACH(i, a->value) + if (nulstr_contains(b->value, i)) + return false; + return true; + } - return true; + if (a->match_type == MATCH_TYPE_GLOB) { + NULSTR_FOREACH(i, a->value) { + size_t i_n = strcspn(i, GLOB_CHARS); + if (i_n == 0) + return false; + NULSTR_FOREACH(j, b->value) { + size_t j_n = strcspn(j, GLOB_CHARS); + if (j_n == 0 || strneq(i, j, MIN(i_n, j_n))) + return false; + } + + } + return true; + } + + return false; } static void udev_check_unused_labels(UdevRuleLine *line) { diff --git a/test/units/testsuite-17.11.sh b/test/units/testsuite-17.11.sh index c7c793e71b..65c47dbcf9 100755 --- a/test/units/testsuite-17.11.sh +++ b/test/units/testsuite-17.11.sh @@ -282,6 +282,8 @@ test_syntax_error 'KERNEL=="a|b", KERNEL=="c|d|e", NAME="f"' 'conflicting match # 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 'ACTION=="a*", ACTION=="bc*", NAME="d"' 'conflicting match expressions, the line takes no effect' +test_syntax_error 'ACTION=="a*|bc*", ACTION=="d*|ef*", NAME="g"' '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 @@ -302,6 +304,8 @@ KERNEL=="a|b", KERNEL=="a|c", NAME="d" KERNEL=="a|b", KERNEL!="a|c", NAME="d" KERNEL!="a", KERNEL!="b", NAME="c" KERNEL=="|a", KERNEL=="|b", NAME="c" +KERNEL=="*", KERNEL=="a*", NAME="b" +KERNEL=="a*", KERNEL=="c*|ab*", NAME="d" EOF assert_0 "${rules}" From ebb00082d8f595ab59ccf92556397b6eaa04b133 Mon Sep 17 00:00:00 2001 From: "Dmitry V. Levin" Date: Mon, 27 Mar 2023 08:00:00 +0000 Subject: [PATCH 8/8] udev-rules: fix grammar in diagnostics about lines that have no effect Fixes: 25de7aa7b90c ("udev: modernize udev-rules.c") --- src/udev/udev-rules.c | 6 +++--- test/units/testsuite-17.11.sh | 22 +++++++++++----------- 2 files changed, 14 insertions(+), 14 deletions(-) diff --git a/src/udev/udev-rules.c b/src/udev/udev-rules.c index c234c265ee..f7a1bb3bb0 100644 --- a/src/udev/udev-rules.c +++ b/src/udev/udev-rules.c @@ -1232,7 +1232,7 @@ static int rule_add_line(UdevRuleFile *rule_file, const char *line_str, unsigned } if (rule_line->type == 0) { - log_line_warning(rule_line, "The line takes no effect, ignoring."); + log_line_warning(rule_line, "The line has no effect, ignoring."); return 0; } @@ -1264,7 +1264,7 @@ static void rule_resolve_goto(UdevRuleFile *rule_file) { line->goto_label = NULL; if ((line->type & ~(LINE_HAS_LABEL|LINE_IS_REFERENCED)) == 0) { - log_line_notice(line, "The line takes no effect any more, dropping"); + log_line_notice(line, "The line has no effect any more, dropping."); /* LINE_IS_REFERENCED implies LINE_HAS_LABEL */ if (line->type & LINE_HAS_LABEL) udev_rule_line_clear_tokens(line); @@ -1411,7 +1411,7 @@ static void udev_check_conflicts_duplicates(UdevRuleLine *line) { } if (new_conflicts) { conflicts = new_conflicts; - log_line_error(line, "conflicting match expressions, the line takes no effect"); + log_line_error(line, "conflicting match expressions, the line has no effect"); } if (conflicts && duplicates) return; diff --git a/test/units/testsuite-17.11.sh b/test/units/testsuite-17.11.sh index 65c47dbcf9..ba21c9ab87 100755 --- a/test/units/testsuite-17.11.sh +++ b/test/units/testsuite-17.11.sh @@ -173,7 +173,7 @@ EOF test_syntax_error '=' 'Invalid key/value pair, ignoring.' test_syntax_error 'ACTION{a}=="b"' 'Invalid attribute for ACTION.' test_syntax_error 'ACTION:="b"' 'Invalid operator for ACTION.' -test_syntax_error 'ACTION=="b"' 'The line takes no effect, ignoring.' +test_syntax_error 'ACTION=="b"' 'The line has no effect, ignoring.' test_syntax_error 'DEVPATH{a}=="b"' 'Invalid attribute for DEVPATH.' test_syntax_error 'DEVPATH:="b"' 'Invalid operator for DEVPATH.' test_syntax_error 'KERNEL{a}=="b"' 'Invalid attribute for KERNEL.' @@ -275,15 +275,15 @@ test_syntax_error 'LABEL{a}="b"' 'Invalid attribute for LABEL.' test_syntax_error 'LABEL=="b"' 'Invalid operator for LABEL.' 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' -test_syntax_error 'KERNEL=="a|b", KERNEL=="c|d|e", NAME="f"' 'conflicting match expressions, the line takes no effect' +test_syntax_error 'KERNEL=="", KERNEL=="?*", NAME="a"' 'conflicting match expressions, the line has no effect' +test_syntax_error 'KERNEL=="abc", KERNEL!="abc", NAME="b"' 'conflicting match expressions, the line has no effect' +test_syntax_error 'KERNEL=="|a|b", KERNEL!="b|a|", NAME="c"' 'conflicting match expressions, the line has no effect' +test_syntax_error 'KERNEL=="a|b", KERNEL=="c|d|e", NAME="f"' 'conflicting match expressions, the line has 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 'ACTION=="a*", ACTION=="bc*", NAME="d"' 'conflicting match expressions, the line takes no effect' -test_syntax_error 'ACTION=="a*|bc*", ACTION=="d*|ef*", NAME="g"' 'conflicting match expressions, the line takes no effect' + 'conflicting match expressions, the line has no effect' +test_syntax_error 'ACTION=="a*", ACTION=="bc*", NAME="d"' 'conflicting match expressions, the line has no effect' +test_syntax_error 'ACTION=="a*|bc*", ACTION=="d*|ef*", NAME="g"' 'conflicting match expressions, the line has 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 @@ -312,7 +312,7 @@ assert_0 "${rules}" echo 'GOTO="a"' >"${rules}" cat >"${exp}" <"${exp}" <"${exp}" <