udev-rules: check for conflicting and duplicate expressions

Log an error when a rule line contains conflicting match expressions, e.g.
  NAME=="value", NAME!="value"

Log a warning when a rule line contains duplicate expressions, e.g.
  NAME=="value", NAME=="value"

diff --git a/src/udev/udev-rules.c b/src/udev/udev-rules.c
index 04220d97dbbf4f995513948915acabda9ae0ca1a..71c09492202014a501c487c011997679607e2e9d 100644 (file)
--- a/src/udev/udev-rules.c
+++ b/src/udev/udev-rules.c
@@ -1343,15 +1343,91 @@ int udev_rules_parse_file(UdevRules *rules, const char *filename, UdevRuleFile *
         return 1;
 }
 
-static void udev_check_rule_line(UdevRuleLine *line) {
+static bool token_data_is_string(UdevRuleTokenType type) {
+        return IN_SET(type, TK_M_ENV,
+                            TK_M_CONST,
+                            TK_M_ATTR,
+                            TK_M_SYSCTL,
+                            TK_M_PARENTS_ATTR,
+                            TK_A_SECLABEL,
+                            TK_A_ENV,
+                            TK_A_ATTR,
+                            TK_A_SYSCTL);
+}
+
+static bool token_type_and_data_eq(const UdevRuleToken *a, const UdevRuleToken *b) {
+        assert(a);
+        assert(b);
+
+        return a->type == b->type &&
+               (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) {
+        assert(a);
+        assert(b);
+
+        if (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);
+}
+
+static bool conflicting_op(UdevRuleOperatorType a, UdevRuleOperatorType b) {
+        return (a == OP_MATCH && b == OP_NOMATCH) ||
+               (a == OP_NOMATCH && b == OP_MATCH);
+}
+
+/* test whether all fields besides UdevRuleOperatorType of two tokens match */
+static bool tokens_eq(const UdevRuleToken *a, const UdevRuleToken *b) {
+        assert(a);
+        assert(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_data_eq(a, b);
+}
+
+static void udev_check_unused_labels(UdevRuleLine *line) {
         assert(line);
 
-        /* check for unused labels */
         if (FLAGS_SET(line->type, LINE_HAS_LABEL) &&
             !FLAGS_SET(line->type, LINE_IS_REFERENCED))
                 log_line_warning(line, "LABEL=\"%s\" is unused.", line->label);
 }
 
+static void udev_check_conflicts_duplicates(UdevRuleLine *line) {
+        assert(line);
+
+        bool conflicts = false, duplicates = false;
+
+        LIST_FOREACH(tokens, token, line->tokens)
+                LIST_FOREACH(tokens, i, token->tokens_next) {
+                        if (!tokens_eq(token, i))
+                                continue;
+                        if (!duplicates && token->op == i->op) {
+                                duplicates = true;
+                                log_line_warning(line, "duplicate expressions");
+                        }
+                        if (!conflicts && conflicting_op(token->op, i->op)) {
+                                conflicts = true;
+                                log_line_error(line, "conflicting match expressions, the line takes no effect");
+                        }
+                        if (conflicts && duplicates)
+                                return;
+                }
+}
+
+static void udev_check_rule_line(UdevRuleLine *line) {
+        udev_check_unused_labels(line);
+        udev_check_conflicts_duplicates(line);
+}
+
 unsigned udev_rule_file_get_issues(UdevRuleFile *rule_file) {
         assert(rule_file);
 

diff --git a/test/units/testsuite-17.11.sh b/test/units/testsuite-17.11.sh
index 363040c28c92f63377b89bebe22d9e70b3ab945e..8bcf517ea1069aa04749fc3c016aca2232beff5c 100755 (executable)
--- a/test/units/testsuite-17.11.sh
+++ b/test/units/testsuite-17.11.sh
@@ -99,8 +99,11 @@ printf '%16384s\n' ' ' >"${rules}"
 echo "Failed to parse rules file ${rules}: No buffer space available" >"${exp}"
 assert_1 "${rules}"
 
-printf 'RUN+="/bin/true"%8175s\\\n' ' ' ' ' >"${rules}"
-echo >>"${rules}"
+{
+  printf 'RUN+="/bin/true"%8175s\\\n' ' '
+  printf 'RUN+="/bin/false"%8174s\\\n' ' '
+  echo
+} >"${rules}"
 assert_0 "${rules}"
 
 printf 'RUN+="/bin/true"%8176s\\\n #\n' ' ' ' ' >"${rules}"
@@ -236,6 +239,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'
+# 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'
+# 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'
 
 echo 'GOTO="a"' >"${rules}"
 cat >"${exp}" <<EOF
@@ -275,6 +287,16 @@ ${rules}: udev rules check failed
 EOF
 assert_1 "${rules}"
 
+cat >"${rules}" <<'EOF'
+KERNEL!="", KERNEL=="?*", KERNEL=="", NAME="a"
+EOF
+cat >"${exp}" <<EOF
+${rules}:1 duplicate expressions
+${rules}:1 conflicting match expressions, the line takes no effect
+${rules}: udev rules check failed
+EOF
+assert_1 "${rules}"
+
 # udevadm verify --root
 sed "s|sample-[0-9]*.rules|${workdir}/${rules_dir}/&|" sample-*.exp >"${workdir}/${exp}"
 cd -