From 4f5de157316f00e046bc43687144fb8a6a81808f Mon Sep 17 00:00:00 2001 From: "Dmitry V. Levin" Date: Wed, 1 Mar 2023 08:00:00 +0000 Subject: [PATCH 1/8] udev_rules_parse_file: mark logged issues Traditionally, all issues found in udev rules by udev_rules_parse_file() are logged and ignored, so there was no mechanism to propagate the information about these issues back to the caller. Introduce such a mechanism by adding a new member to UdevRuleFile. This new member is a bitmask describing which log levels were used in messages logged with regards to the rule file. This mechanism is going to be used by udevadm verify in subsequent commits. --- src/udev/udev-rules.c | 58 ++++++++++++++++++++++++++++++++++++++----- 1 file changed, 52 insertions(+), 6 deletions(-) diff --git a/src/udev/udev-rules.c b/src/udev/udev-rules.c index 68c4bc903c..840448d65c 100644 --- a/src/udev/udev-rules.c +++ b/src/udev/udev-rules.c @@ -172,6 +172,7 @@ struct UdevRuleLine { struct UdevRuleFile { char *filename; UdevRuleLine *current_line; + unsigned issues; LIST_HEAD(UdevRuleLine, rule_lines); LIST_FIELDS(UdevRuleFile, rule_files); }; @@ -190,12 +191,14 @@ struct UdevRules { #define log_rule_full_errno_zerook(device, rules, level, error, fmt, ...) \ ({ \ UdevRules *_r = (rules); \ + int _lv = (level); \ UdevRuleFile *_f = _r ? _r->current_file : NULL; \ UdevRuleLine *_l = _f ? _f->current_line : NULL; \ const char *_n = _f ? _f->filename : NULL; \ \ + rule_file_mark_issue(_f, _lv); \ log_device_full_errno_zerook( \ - device, level, error, "%s:%u " fmt, \ + device, _lv, error, "%s:%u " fmt, \ strna(_n), _l ? _l->line_number : 0, \ ##__VA_ARGS__); \ }) @@ -254,6 +257,48 @@ struct UdevRules { "Invalid value \"%s\" for %s (char %zu: %s), ignoring.", \ value, key, offset, hint) +#define log_line_full_errno_zerook(rule_file, line_nr, level, error, fmt, ...) \ + ({ \ + UdevRuleFile *_f = (rule_file); \ + int _lv = (level); \ + const char *_n = _f ? _f->filename : NULL; \ + \ + rule_file_mark_issue(_f, _lv); \ + log_device_full_errno_zerook( \ + NULL, _lv, error, "%s:%u " fmt, \ + strna(_n), line_nr, \ + ##__VA_ARGS__); \ + }) + +#define log_line_full_errno(rule_file, line_nr, level, error, fmt, ...) \ + ({ \ + int _error = (error); \ + ASSERT_NON_ZERO(_error); \ + log_line_full_errno_zerook( \ + rule_file, line_nr, level, _error, \ + fmt, ##__VA_ARGS__); \ + }) + +#define log_line_full(rule_file, line_nr, level, ...) \ + (void) log_line_full_errno_zerook(rule_file, line_nr, level, 0, __VA_ARGS__) + +#define log_line_debug(rule_file, line_nr, ...) log_line_full(rule_file, line_nr, LOG_DEBUG, __VA_ARGS__) +#define log_line_info(rule_file, line_nr, ...) log_line_full(rule_file, line_nr, LOG_INFO, __VA_ARGS__) +#define log_line_notice(rule_file, line_nr, ...) log_line_full(rule_file, line_nr, LOG_NOTICE, __VA_ARGS__) +#define log_line_warning(rule_file, line_nr, ...) log_line_full(rule_file, line_nr, LOG_WARNING, __VA_ARGS__) +#define log_line_error(rule_file, line_nr, ...) log_line_full(rule_file, line_nr, LOG_ERR, __VA_ARGS__) + +#define log_line_debug_errno(rule_file, line_nr, error, ...) log_line_full_errno(rule_file, line_nr, LOG_DEBUG, error, __VA_ARGS__) +#define log_line_info_errno(rule_file, line_nr, error, ...) log_line_full_errno(rule_file, line_nr, LOG_INFO, error, __VA_ARGS__) +#define log_line_notice_errno(rule_file, line_nr, error, ...) log_line_full_errno(rule_file, line_nr, LOG_NOTICE, error, __VA_ARGS__) +#define log_line_warning_errno(rule_file, line_nr, error, ...) log_line_full_errno(rule_file, line_nr, LOG_WARNING, error, __VA_ARGS__) +#define log_line_error_errno(rule_file, line_nr, error, ...) log_line_full_errno(rule_file, line_nr, LOG_ERR, error, __VA_ARGS__) + +static void rule_file_mark_issue(UdevRuleFile *rule_file, uint8_t log_level) { + if (rule_file) + rule_file->issues |= (1U << log_level); +} + static void log_unknown_owner(sd_device *dev, UdevRules *rules, int error, const char *entity, const char *name) { if (IN_SET(abs(error), ENOENT, ESRCH)) log_rule_error(dev, rules, "Unknown %s '%s', ignoring", entity, name); @@ -1150,15 +1195,16 @@ static void rule_resolve_goto(UdevRuleFile *rule_file) { } if (!line->goto_line) { - log_error("%s:%u: GOTO=\"%s\" has no matching label, ignoring", - rule_file->filename, line->line_number, line->goto_label); + log_line_error(rule_file, line->line_number, + "GOTO=\"%s\" has no matching label, ignoring", + line->goto_label); SET_FLAG(line->type, LINE_HAS_GOTO, false); line->goto_label = NULL; if ((line->type & ~LINE_HAS_LABEL) == 0) { - log_notice("%s:%u: The line takes no effect any more, dropping", - rule_file->filename, line->line_number); + log_line_notice(rule_file, line->line_number, + "The line takes no effect any more, dropping"); if (line->type == LINE_HAS_LABEL) udev_rule_line_clear_tokens(line); else @@ -1263,7 +1309,7 @@ int udev_rules_parse_file(UdevRules *rules, const char *filename) { } if (ignore_line) - log_error("%s:%u: Line is too long, ignored", filename, line_nr); + log_line_error(rule_file, line_nr, "Line is too long, ignored"); else if (len > 0) (void) rule_add_line(rules, line, line_nr); From acdba85e0e2bf229f356ad09a2b880e89224715d Mon Sep 17 00:00:00 2001 From: "Dmitry V. Levin" Date: Thu, 2 Mar 2023 08:00:00 +0000 Subject: [PATCH 2/8] udevadm: introduce new 'verify' command We seem to have no tool to verify udev rule files. There is a simple udev rules syntax checker in the tree, test/rule-syntax-check.py, but it is too simple to detect less trivial issues not detected by udev, e.g. redundant comparisons (#26593) or labels without references. Such a tool would be beneficial not only for maintaining udev rules distributed along with udev, but also and even more so for maintaining third party udev rules that are more likely to have issues with syntax and semantic correctness. Implement a udev rules syntax and semantics checker in the form of 'udevadm verify [OPTIONS] FILE...' command that is based on udev_rules_parse_file() interface and would apply further checks on top of it in subsequent commits. Resolves: #26606 --- man/udevadm.xml | 36 ++++++++++ shell-completion/bash/udevadm | 24 ++++++- shell-completion/zsh/_udevadm | 9 +++ src/udev/meson.build | 1 + src/udev/udev-rules.c | 10 +++ src/udev/udev-rules.h | 1 + src/udev/udevadm-verify.c | 125 ++++++++++++++++++++++++++++++++++ src/udev/udevadm.c | 2 + src/udev/udevadm.h | 1 + 9 files changed, 208 insertions(+), 1 deletion(-) create mode 100644 src/udev/udevadm-verify.c diff --git a/man/udevadm.xml b/man/udevadm.xml index 33af155021..95b8da2b0a 100644 --- a/man/udevadm.xml +++ b/man/udevadm.xml @@ -48,6 +48,11 @@ udevadm test-builtin options command devpath + + udevadm verify + options + file + udevadm wait options device|syspath @@ -737,6 +742,37 @@ + + udevadm verify + <arg choice="opt"><replaceable>options</replaceable></arg> + <arg choice="plain" rep="repeat"><replaceable>file</replaceable></arg> + … + + + Verify syntactic and semantic correctness of udev rules files. + + Positional arguments should be used to specify one or more files to check. + + The exit status is 0 if all specified udev rules files + are syntactically and semantically correct, and a non-zero error code otherwise. + + + + + + + Specify when udevadm should resolve names of users + and groups. When set to early (the + default), names will be resolved when the rules are + parsed. When set to never, names will + never be resolved. + + + + + + + udevadm wait <arg choice="opt"><replaceable>options</replaceable></arg> diff --git a/shell-completion/bash/udevadm b/shell-completion/bash/udevadm index 99d4aa55c9..b6e14e1d36 100644 --- a/shell-completion/bash/udevadm +++ b/shell-completion/bash/udevadm @@ -64,11 +64,12 @@ _udevadm() { [MONITOR_ARG]='-s --subsystem-match -t --tag-match' [TEST]='-a --action -N --resolve-names' [TEST_BUILTIN]='-a --action' + [VERIFY]='-N --resolve-names' [WAIT]='-t --timeout --initialized=no --removed --settle' [LOCK]='-t --timeout -d --device -b --backing -p --print' ) - local verbs=(info trigger settle control monitor test-builtin test wait lock) + local verbs=(info trigger settle control monitor test-builtin test verify wait lock) local builtins=(blkid btrfs hwdb input_id keyboard kmod net_id net_setup_link path_id usb_id uaccess) for ((i=0; i < COMP_CWORD; i++)); do @@ -247,6 +248,27 @@ _udevadm() { fi ;; + 'verify') + if __contains_word "$prev" ${OPTS[VERIFY]}; then + case $prev in + -N|--resolve-names) + comps='early never' + ;; + *) + comps='' + ;; + esac + COMPREPLY=( $(compgen -W '$comps' -- "$cur") ) + return 0 + fi + + if [[ $cur = -* ]]; then + comps="${OPTS[COMMON]} ${OPTS[VERIFY]}" + else + comps=$( compgen -A file -- "$cur" ) + fi + ;; + 'wait') if __contains_word "$prev" ${OPTS[WAIT]}; then case $prev in diff --git a/shell-completion/zsh/_udevadm b/shell-completion/zsh/_udevadm index f7c3384eae..074d367a9d 100644 --- a/shell-completion/zsh/_udevadm +++ b/shell-completion/zsh/_udevadm @@ -104,6 +104,14 @@ _udevadm_test-builtin(){ fi } +(( $+functions[_udevadm_verify] )) || +_udevadm_verify(){ + _arguments \ + {-N+,--resolve-names=}'[When to resolve names.]:resolve:(early never)' \ + {-h,--help}'[Print help text.]' \ + '*::files:_files' +} + (( $+functions[_udevadm_wait] )) || _udevadm_wait(){ _arguments \ @@ -154,6 +162,7 @@ _udevadm_commands(){ 'monitor:listen to kernel and udev events' 'test:test an event run' 'test-builtin:test a built-in command' + 'verify:verify udev rules files' 'wait:wait for devices or device symlinks being created' 'lock:lock a block device and run a comand' ) diff --git a/src/udev/meson.build b/src/udev/meson.build index 3f4ab00ac9..1cac581e7f 100644 --- a/src/udev/meson.build +++ b/src/udev/meson.build @@ -12,6 +12,7 @@ udevadm_sources = files( 'udevadm-test-builtin.c', 'udevadm-trigger.c', 'udevadm-util.c', + 'udevadm-verify.c', 'udevadm-wait.c', 'udevd.c', ) diff --git a/src/udev/udev-rules.c b/src/udev/udev-rules.c index 840448d65c..c2e98e9a9e 100644 --- a/src/udev/udev-rules.c +++ b/src/udev/udev-rules.c @@ -1321,6 +1321,16 @@ int udev_rules_parse_file(UdevRules *rules, const char *filename) { return 0; } +unsigned udev_check_current_rule_file(UdevRules *rules) { + assert(rules); + + UdevRuleFile *rule_file = rules->current_file; + if (!rule_file) + return 0; + + return rule_file->issues; +} + UdevRules* udev_rules_new(ResolveNameTiming resolve_name_timing) { assert(resolve_name_timing >= 0 && resolve_name_timing < _RESOLVE_NAME_TIMING_MAX); diff --git a/src/udev/udev-rules.h b/src/udev/udev-rules.h index 860fe7c8e4..44f30e1c13 100644 --- a/src/udev/udev-rules.h +++ b/src/udev/udev-rules.h @@ -18,6 +18,7 @@ typedef enum { } UdevRuleEscapeType; int udev_rules_parse_file(UdevRules *rules, const char *filename); +unsigned udev_check_current_rule_file(UdevRules *rules); UdevRules* udev_rules_new(ResolveNameTiming resolve_name_timing); int udev_rules_load(UdevRules **ret_rules, ResolveNameTiming resolve_name_timing); UdevRules *udev_rules_free(UdevRules *rules); diff --git a/src/udev/udevadm-verify.c b/src/udev/udevadm-verify.c new file mode 100644 index 0000000000..8819b07238 --- /dev/null +++ b/src/udev/udevadm-verify.c @@ -0,0 +1,125 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later */ + +#include <errno.h> +#include <getopt.h> +#include <stddef.h> +#include <stdio.h> +#include <stdlib.h> +#include <unistd.h> + +#include "log.h" +#include "pretty-print.h" +#include "strv.h" +#include "udev-rules.h" +#include "udevadm.h" + +static ResolveNameTiming arg_resolve_name_timing = RESOLVE_NAME_EARLY; + +static int help(void) { + _cleanup_free_ char *link = NULL; + int r; + + r = terminal_urlify_man("udevadm", "8", &link); + if (r < 0) + return log_oom(); + + printf("%s verify [OPTIONS] FILE...\n" + "\n%sVerify udev rules files.%s\n\n" + " -h --help Show this help\n" + " -V --version Show package version\n" + " -N --resolve-names=early|never When to resolve names\n" + "\nSee the %s for details.\n", + program_invocation_short_name, + ansi_highlight(), + ansi_normal(), + link); + + return 0; +} + +static int parse_argv(int argc, char *argv[]) { + static const struct option options[] = { + { "help", no_argument, NULL, 'h' }, + { "version", no_argument, NULL, 'V' }, + { "resolve-names", required_argument, NULL, 'N' }, + {} + }; + + int c; + + assert(argc >= 0); + assert(argv); + + while ((c = getopt_long(argc, argv, "hVN:", options, NULL)) >= 0) + switch (c) { + case 'h': + return help(); + case 'V': + return print_version(); + case 'N': + arg_resolve_name_timing = resolve_name_timing_from_string(optarg); + if (arg_resolve_name_timing < 0) + return log_error_errno(arg_resolve_name_timing, + "--resolve-names= takes \"early\" or \"never\""); + /* + * In the verifier "late" has the effect of "never", + * and "never" would generate irrelevant diagnostics, + * so map "never" to "late". + */ + if (arg_resolve_name_timing == RESOLVE_NAME_NEVER) + arg_resolve_name_timing = RESOLVE_NAME_LATE; + break; + case '?': + return -EINVAL; + default: + assert_not_reached(); + } + + if (optind == argc) + return log_error_errno(SYNTHETIC_ERRNO(EINVAL), "No rules file specified."); + + return 1; +} + +static int verify_rules_file(UdevRules *rules, const char *fname) { + int r; + + r = udev_rules_parse_file(rules, fname); + if (r < 0) + return log_error_errno(r, "Failed to parse rules file %s: %m", fname); + + unsigned issues = udev_check_current_rule_file(rules); + unsigned mask = (1U << LOG_ERR) | (1U << LOG_WARNING); + if (issues & mask) + return log_error_errno(SYNTHETIC_ERRNO(EINVAL), + "%s: udev rules check failed", fname); + + return 0; +} + +static int verify_rules(UdevRules *rules, char **files) { + int r, rv = 0; + + STRV_FOREACH(fp, files) { + r = verify_rules_file(rules, *fp); + if (r < 0 && rv >= 0) + rv = r; + } + + return rv; +} + +int verify_main(int argc, char *argv[], void *userdata) { + _cleanup_(udev_rules_freep) UdevRules *rules = NULL; + int r; + + r = parse_argv(argc, argv); + if (r <= 0) + return r; + + rules = udev_rules_new(arg_resolve_name_timing); + if (!rules) + return -ENOMEM; + + return verify_rules(rules, strv_skip(argv, optind)); +} diff --git a/src/udev/udevadm.c b/src/udev/udevadm.c index b742c1a287..273d3c5b29 100644 --- a/src/udev/udevadm.c +++ b/src/udev/udevadm.c @@ -25,6 +25,7 @@ static int help(void) { { "monitor", "Listen to kernel and udev events" }, { "test", "Test an event run" }, { "test-builtin", "Test a built-in command" }, + { "verify", "Verify udev rules files" }, { "wait", "Wait for device or device symlink" }, { "lock", "Lock a block device" }, }; @@ -104,6 +105,7 @@ static int udevadm_main(int argc, char *argv[]) { { "test-builtin", VERB_ANY, VERB_ANY, 0, builtin_main }, { "wait", VERB_ANY, VERB_ANY, 0, wait_main }, { "lock", VERB_ANY, VERB_ANY, 0, lock_main }, + { "verify", VERB_ANY, VERB_ANY, 0, verify_main }, { "version", VERB_ANY, VERB_ANY, 0, version_main }, { "help", VERB_ANY, VERB_ANY, 0, help_main }, {} diff --git a/src/udev/udevadm.h b/src/udev/udevadm.h index 417611affe..7920a70d5b 100644 --- a/src/udev/udevadm.h +++ b/src/udev/udevadm.h @@ -13,6 +13,7 @@ int monitor_main(int argc, char *argv[], void *userdata); int hwdb_main(int argc, char *argv[], void *userdata); int test_main(int argc, char *argv[], void *userdata); int builtin_main(int argc, char *argv[], void *userdata); +int verify_main(int argc, char *argv[], void *userdata); int wait_main(int argc, char *argv[], void *userdata); int lock_main(int argc, char *argv[], void *userdata); From f94e9529fe2c7e1e470b5626e220be5ce443928b Mon Sep 17 00:00:00 2001 From: "Dmitry V. Levin" <ldv@strace.io> Date: Tue, 7 Mar 2023 08:00:00 +0000 Subject: [PATCH 3/8] rules: do not use blkid builtin if built without blkid support When built without blkid, then udev-builtin-blkid is not built, and the verifier warns about the unknown builtin: 60-persistent-storage.rules:114 Unknown builtin command: blkid --hint=session_offset=$env{ID_CDROM_MEDIA_SESSION_LAST_OFFSET} 60-persistent-storage.rules:117 Unknown builtin command: blkid --noraid 60-persistent-storage.rules:120 Unknown builtin command: blkid 60-persistent-storage.rules: udev rules check failed --- ...-persistent-storage.rules => 60-persistent-storage.rules.in} | 2 ++ rules.d/meson.build | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) rename rules.d/{60-persistent-storage.rules => 60-persistent-storage.rules.in} (99%) diff --git a/rules.d/60-persistent-storage.rules b/rules.d/60-persistent-storage.rules.in similarity index 99% rename from rules.d/60-persistent-storage.rules rename to rules.d/60-persistent-storage.rules.in index 3265cd23c5..498b24c22d 100644 --- a/rules.d/60-persistent-storage.rules +++ b/rules.d/60-persistent-storage.rules.in @@ -106,6 +106,7 @@ ENV{DEVTYPE}=="partition", ENV{ID_PATH_ATA_COMPAT}=="?*", SYMLINK+="disk/by-path KERNEL=="vd*[!0-9]", ENV{ID_PATH}=="pci-*", SYMLINK+="disk/by-path/virtio-$env{ID_PATH}" KERNEL=="vd*[0-9]", ENV{ID_PATH}=="pci-*", SYMLINK+="disk/by-path/virtio-$env{ID_PATH}-part%n" +{% if HAVE_BLKID %} # allow admin to disable probing the filesystem for slow devices like floppy disk drives ENV{UDEV_DISABLE_PERSISTENT_STORAGE_BLKID_FLAG}=="1", GOTO="persistent_storage_blkid_probe_end" @@ -120,6 +121,7 @@ KERNEL=="sr*", ENV{DISK_EJECT_REQUEST}!="?*", ENV{ID_CDROM_MEDIA_TRACK_COUNT_DAT KERNEL!="sr*|mmcblk[0-9]boot[0-9]", IMPORT{builtin}="blkid" LABEL="persistent_storage_blkid_probe_end" +{% endif %} # by-label/by-uuid links (filesystem metadata) ENV{ID_FS_USAGE}=="filesystem|other|crypto", ENV{ID_FS_UUID_ENC}=="?*", SYMLINK+="disk/by-uuid/$env{ID_FS_UUID_ENC}" diff --git a/rules.d/meson.build b/rules.d/meson.build index cba9dd4cc4..09edd58da2 100644 --- a/rules.d/meson.build +++ b/rules.d/meson.build @@ -15,7 +15,6 @@ rules = [ '60-input-id.rules', '60-persistent-alsa.rules', '60-persistent-input.rules', - '60-persistent-storage.rules', '60-persistent-storage-tape.rules', '60-persistent-v4l.rules', '60-sensor.rules', @@ -53,6 +52,7 @@ endforeach rules_in = [ ['50-udev-default.rules'], + ['60-persistent-storage.rules'], ['64-btrfs.rules'], ['99-systemd.rules'], From 9382642c409b2bba549a5453abbf1b2e94686171 Mon Sep 17 00:00:00 2001 From: "Dmitry V. Levin" <ldv@strace.io> Date: Fri, 3 Mar 2023 08:00:00 +0000 Subject: [PATCH 4/8] meson.build: check udev rules using udevadm verify Although udev rules are already being checked by rule-syntax-check.py script, also check them using udevadm verify which performs more thorough checks. --- meson.build | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/meson.build b/meson.build index ff8706d94c..6885884a00 100644 --- a/meson.build +++ b/meson.build @@ -3847,6 +3847,13 @@ udevadm = executable( install_dir : rootbindir) public_programs += udevadm +if want_tests != 'false' + test('udev-rules-check', + udevadm, + suite : 'dist-check', + args : ['verify', '--resolve-names=never', all_rules]) +endif + if conf.get('ENABLE_REPART') == 1 exe = executable( 'systemd-repart', From 45183cdd6dce5a03274994f3c992fbd86d510f50 Mon Sep 17 00:00:00 2001 From: "Dmitry V. Levin" <ldv@strace.io> Date: Sat, 4 Mar 2023 08:00:00 +0000 Subject: [PATCH 5/8] test: add a test for udevadm verify --- test/units/testsuite-17.11.sh | 180 ++++++++++++++++++++++++++++++++++ 1 file changed, 180 insertions(+) create mode 100755 test/units/testsuite-17.11.sh diff --git a/test/units/testsuite-17.11.sh b/test/units/testsuite-17.11.sh new file mode 100755 index 0000000000..b818deaecd --- /dev/null +++ b/test/units/testsuite-17.11.sh @@ -0,0 +1,180 @@ +#!/usr/bin/env bash +# SPDX-License-Identifier: LGPL-2.1-or-later +set -ex +set -o pipefail + +# Test for udevadm verify. + +udevadm verify -h +udevadm verify --help +udevadm verify -V +udevadm verify --version +udevadm verify /dev/null + +# No rules file specified. +(! udevadm verify) +# unrecognized option '--unknown' +(! udevadm verify --unknown) +# option requires an argument -- 'N' +(! udevadm verify -N) +# --resolve-names= takes "early" or "never" +(! udevadm verify -N now) +# option '--resolve-names' requires an argument +(! udevadm verify --resolve-names) +# --resolve-names= takes "early" or "never" +(! udevadm verify --resolve-names=now) +# Failed to parse rules file .: Is a directory +(! udevadm verify .) +# Failed to parse rules file .: Is a directory +(! udevadm verify /dev/null . /dev/null) + +# UDEV_LINE_SIZE 16384 +printf '%16383s\n' ' ' >sample.rules +udevadm verify sample.rules + +# Failed to parse rules file sample.rules: No buffer space available +printf '%16384s\n' ' ' >sample.rules +(! udevadm verify sample.rules) + +printf 'RUN+="/bin/true"%8175s\\\n' ' ' ' ' >sample.rules +echo >>sample.rules +udevadm verify sample.rules + +printf 'RUN+="/bin/true"%8176s\\\n' ' ' ' ' >sample.rules +echo >>sample.rules +cat >exp <<'EOF' +sample.rules:3 Line is too long, ignored +sample.rules: udev rules check failed +EOF +(! udevadm verify sample.rules 2>err) +diff exp err + +test_syntax_error() { + local rule msg + rule="$1"; shift + msg="$1"; shift + + printf '%s\n' "$rule" > sample.rules + cat >exp <<-EOF + sample.rules:1 $msg + sample.rules: udev rules check failed + EOF + (! udevadm verify sample.rules 2>err) + diff exp err +} + +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 '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.' +test_syntax_error 'KERNEL:="b"' 'Invalid operator for KERNEL.' +test_syntax_error 'KERNELS{a}=="b"' 'Invalid attribute for KERNELS.' +test_syntax_error 'KERNELS:="b"' 'Invalid operator for KERNELS.' +test_syntax_error 'SYMLINK{a}=="b"' 'Invalid attribute for SYMLINK.' +test_syntax_error 'SYMLINK:="%?"' 'Invalid value "%?" for SYMLINK (char 1: invalid substitution type), ignoring.' +test_syntax_error 'NAME{a}=="b"' 'Invalid attribute for NAME.' +test_syntax_error 'NAME-="b"' 'Invalid operator for NAME.' +test_syntax_error 'NAME+="a"' "NAME key takes '==', '!=', '=', or ':=' operator, assuming '='." +test_syntax_error 'NAME:=""' 'Ignoring NAME="", as udev will not delete any network interfaces.' +test_syntax_error 'NAME="%k"' 'Ignoring NAME="%k", as it will take no effect.' +test_syntax_error 'ENV=="b"' 'Invalid attribute for ENV.' +test_syntax_error 'ENV{a}-="b"' 'Invalid operator for ENV.' +test_syntax_error 'ENV{a}:="b"' "ENV key takes '==', '!=', '=', or '+=' operator, assuming '='." +test_syntax_error 'ENV{ACTION}="b"' "Invalid ENV attribute. 'ACTION' cannot be set." +test_syntax_error 'CONST=="b"' 'Invalid attribute for CONST.' +test_syntax_error 'CONST{a}=="b"' 'Invalid attribute for CONST.' +test_syntax_error 'CONST{arch}="b"' 'Invalid operator for CONST.' +test_syntax_error 'TAG{a}=="b"' 'Invalid attribute for TAG.' +test_syntax_error 'TAG:="a"' "TAG key takes '==', '!=', '=', or '+=' operator, assuming '='." +test_syntax_error 'TAG="%?"' 'Invalid value "%?" for TAG (char 1: invalid substitution type), ignoring.' +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 '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.' +test_syntax_error 'DRIVER:="b"' 'Invalid operator for DRIVER.' +test_syntax_error 'DRIVERS{a}=="b"' 'Invalid attribute for DRIVERS.' +test_syntax_error 'DRIVERS:="b"' 'Invalid operator for DRIVERS.' +test_syntax_error 'ATTR="b"' 'Invalid attribute for ATTR.' +test_syntax_error 'ATTR{%}="b"' 'Invalid attribute "%" for ATTR (char 1: invalid substitution type), ignoring.' +test_syntax_error 'ATTR{a}-="b"' 'Invalid operator for ATTR.' +test_syntax_error 'ATTR{a}+="b"' "ATTR key takes '==', '!=', or '=' operator, assuming '='." +test_syntax_error 'ATTR{a}="%?"' 'Invalid value "%?" for ATTR (char 1: invalid substitution type), ignoring.' +test_syntax_error 'SYSCTL=""' 'Invalid attribute for SYSCTL.' +test_syntax_error 'SYSCTL{%}="b"' 'Invalid attribute "%" for SYSCTL (char 1: invalid substitution type), ignoring.' +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{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 '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{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 '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{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.' +test_syntax_error 'OPTIONS{a}="b"' 'Invalid attribute for OPTIONS.' +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 '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.' +test_syntax_error 'OWNER+="0"' "OWNER key takes '=' or ':=' operator, assuming '='." +test_syntax_error 'OWNER="nosuchuser"' "Unknown user 'nosuchuser', ignoring" +test_syntax_error 'GROUP{a}="b"' 'Invalid attribute for GROUP.' +test_syntax_error 'GROUP-="b"' 'Invalid operator for GROUP.' +test_syntax_error 'GROUP!="b"' 'Invalid operator for GROUP.' +test_syntax_error 'GROUP+="0"' "GROUP key takes '=' or ':=' operator, assuming '='." +test_syntax_error 'GROUP="nosuchuser"' "Unknown group 'nosuchuser', ignoring" +test_syntax_error 'MODE{a}="b"' 'Invalid attribute for MODE.' +test_syntax_error 'MODE-="b"' 'Invalid operator for MODE.' +test_syntax_error 'MODE!="b"' 'Invalid operator for MODE.' +test_syntax_error 'MODE+="0"' "MODE key takes '=' or ':=' operator, assuming '='." +test_syntax_error 'MODE="%"' 'Invalid value "%" for MODE (char 1: invalid substitution type), ignoring.' +test_syntax_error 'SECLABEL="b"' 'Invalid attribute for SECLABEL.' +test_syntax_error 'SECLABEL{a}="%"' 'Invalid value "%" for SECLABEL (char 1: invalid substitution type), ignoring.' +test_syntax_error 'SECLABEL{a}!="b"' 'Invalid operator for SECLABEL.' +test_syntax_error 'SECLABEL{a}-="b"' 'Invalid operator for SECLABEL.' +test_syntax_error 'SECLABEL{a}:="b"' "SECLABEL key takes '=' or '+=' operator, assuming '='." +test_syntax_error 'RUN=="b"' 'Invalid operator for RUN.' +test_syntax_error 'RUN-="b"' 'Invalid operator for RUN.' +test_syntax_error 'RUN="%"' 'Invalid value "%" for RUN (char 1: invalid substitution type), ignoring.' +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" +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.' +test_syntax_error 'a="b"' "Invalid key 'a'" + +echo 'GOTO="a"' >sample.rules +cat >exp <<'EOF' +sample.rules:1 GOTO="a" has no matching label, ignoring +sample.rules:1 The line takes no effect any more, dropping +sample.rules: udev rules check failed +EOF +(! udevadm verify sample.rules 2>err) +diff exp err + +exit 0 From 7a771d1da64cb841bbd433ec9772cf897429c44e Mon Sep 17 00:00:00 2001 From: "Dmitry V. Levin" <ldv@strace.io> Date: Sun, 5 Mar 2023 08:00:00 +0000 Subject: [PATCH 6/8] udevadm-verify: check for unused labels Check for unused labels in the specified udev rules files, report such labels and exit with a non-zero exit code if any unused labels are found. --- src/udev/udev-rules.c | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/src/udev/udev-rules.c b/src/udev/udev-rules.c index c2e98e9a9e..21223e0118 100644 --- a/src/udev/udev-rules.c +++ b/src/udev/udev-rules.c @@ -137,6 +137,7 @@ typedef enum { LINE_HAS_GOTO = 1 << 3, /* has GOTO= */ LINE_HAS_LABEL = 1 << 4, /* has LABEL= */ LINE_UPDATE_SOMETHING = 1 << 5, /* has other TK_A_* or TK_M_IMPORT tokens */ + LINE_IS_REFERENCED = 1 << 6, /* is referenced by GOTO */ } UdevRuleLineType; typedef struct UdevRuleFile UdevRuleFile; @@ -1191,6 +1192,7 @@ static void rule_resolve_goto(UdevRuleFile *rule_file) { LIST_FOREACH(rule_lines, i, line->rule_lines_next) if (streq_ptr(i->label, line->goto_label)) { line->goto_line = i; + SET_FLAG(i->type, LINE_IS_REFERENCED, true); break; } @@ -1202,10 +1204,11 @@ static void rule_resolve_goto(UdevRuleFile *rule_file) { SET_FLAG(line->type, LINE_HAS_GOTO, false); line->goto_label = NULL; - if ((line->type & ~LINE_HAS_LABEL) == 0) { + if ((line->type & ~(LINE_HAS_LABEL|LINE_IS_REFERENCED)) == 0) { log_line_notice(rule_file, line->line_number, "The line takes no effect any more, dropping"); - if (line->type == LINE_HAS_LABEL) + /* LINE_IS_REFERENCED implies LINE_HAS_LABEL */ + if (line->type & LINE_HAS_LABEL) udev_rule_line_clear_tokens(line); else udev_rule_line_free(line); @@ -1321,6 +1324,14 @@ int udev_rules_parse_file(UdevRules *rules, const char *filename) { return 0; } +static void udev_check_rule_line(UdevRuleFile *rule_file, const UdevRuleLine *line) { + /* check for unused labels */ + if (FLAGS_SET(line->type, LINE_HAS_LABEL) && + !FLAGS_SET(line->type, LINE_IS_REFERENCED)) + log_line_warning(rule_file, line->line_number, + "LABEL=\"%s\" is unused.", line->label); +} + unsigned udev_check_current_rule_file(UdevRules *rules) { assert(rules); @@ -1328,6 +1339,9 @@ unsigned udev_check_current_rule_file(UdevRules *rules) { if (!rule_file) return 0; + LIST_FOREACH(rule_lines, line, rule_file->rule_lines) + udev_check_rule_line(rule_file, line); + return rule_file->issues; } From 5040401b43b7a3fe3dd372410ae4e3966ddb5d29 Mon Sep 17 00:00:00 2001 From: "Dmitry V. Levin" <ldv@strace.io> Date: Mon, 6 Mar 2023 08:00:00 +0000 Subject: [PATCH 7/8] test: check udevadm verify diagnostics of unused labels --- test/units/testsuite-17.11.sh | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/test/units/testsuite-17.11.sh b/test/units/testsuite-17.11.sh index b818deaecd..d68aa69d41 100755 --- a/test/units/testsuite-17.11.sh +++ b/test/units/testsuite-17.11.sh @@ -166,6 +166,7 @@ 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.' +test_syntax_error 'LABEL="b"' 'LABEL="b" is unused.' test_syntax_error 'a="b"' "Invalid key 'a'" echo 'GOTO="a"' >sample.rules @@ -177,4 +178,22 @@ EOF (! udevadm verify sample.rules 2>err) diff exp err +cat >sample.rules <<'EOF' +GOTO="a" +LABEL="a" +EOF +udevadm verify sample.rules + +cat >sample.rules <<'EOF' +GOTO="b" +LABEL="b" +LABEL="b" +EOF +cat >exp <<'EOF' +sample.rules:3 LABEL="b" is unused. +sample.rules: udev rules check failed +EOF +(! udevadm verify sample.rules 2>err) +diff exp err + exit 0 From b27e5e2aca6dcc97826399993fc5423d84585283 Mon Sep 17 00:00:00 2001 From: "Dmitry V. Levin" <ldv@strace.io> Date: Wed, 8 Mar 2023 08:00:00 +0000 Subject: [PATCH 8/8] udev_rules_parse_file: issue diagnostics about line continuation at EOF When udev rules file ends with a line continuation, the parser used to silently ignore the line without any diagnostics at all. It's time to break the vow of silence and let the parser issue some error diagnostics. --- src/udev/udev-rules.c | 5 +++++ test/units/testsuite-17.11.sh | 12 ++++++++++-- 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/src/udev/udev-rules.c b/src/udev/udev-rules.c index 21223e0118..9d831a6004 100644 --- a/src/udev/udev-rules.c +++ b/src/udev/udev-rules.c @@ -1279,6 +1279,7 @@ int udev_rules_parse_file(UdevRules *rules, const char *filename) { line_nr++; line = skip_leading_chars(buf, NULL); + /* Lines beginning with '#' are ignored regardless of line continuation. */ if (line[0] == '#') continue; @@ -1320,6 +1321,10 @@ int udev_rules_parse_file(UdevRules *rules, const char *filename) { ignore_line = false; } + if (continuation) + log_line_error(rule_file, line_nr, + "Unexpected EOF after line continuation, line ignored"); + rule_resolve_goto(rule_file); return 0; } diff --git a/test/units/testsuite-17.11.sh b/test/units/testsuite-17.11.sh index d68aa69d41..ed5da16c07 100755 --- a/test/units/testsuite-17.11.sh +++ b/test/units/testsuite-17.11.sh @@ -40,10 +40,18 @@ printf 'RUN+="/bin/true"%8175s\\\n' ' ' ' ' >sample.rules echo >>sample.rules udevadm verify sample.rules -printf 'RUN+="/bin/true"%8176s\\\n' ' ' ' ' >sample.rules +printf 'RUN+="/bin/true"%8176s\\\n #\n' ' ' ' ' >sample.rules echo >>sample.rules cat >exp <<'EOF' -sample.rules:3 Line is too long, ignored +sample.rules:5 Line is too long, ignored +sample.rules: udev rules check failed +EOF +(! udevadm verify sample.rules 2>err) +diff exp err + +printf '\\\n' >sample.rules +cat >exp <<'EOF' +sample.rules:1 Unexpected EOF after line continuation, line ignored sample.rules: udev rules check failed EOF (! udevadm verify sample.rules 2>err)