Skip to content

Path validators reject specifier leading paths #512

Description

@SJrX

Path validators reject specifier-leading paths that systemd accepts

Four value validators require the path to start with a literal /, but systemd expands
%-specifiers before it checks that the path is absolute. Any value that begins with a
specifier is therefore flagged as invalid in the editor on a unit systemd loads fine.

Affected (all use RegexTerminal("/\\S*", "/\\S*")):

Validator Setting(s)
ConfigParseWorkingDirectoryOptionValue WorkingDirectory=
ConfigParseUnitEnvFileOptionValue EnvironmentFile=
ConfigParseUnitMountsForOptionValue RequiresMountsFor=, WantsMountsFor=
ConfigParseExecInputOptionValue StandardInput=file:PATH

Why it's wrong

Every one of these calls unit_path_printf() and only then path_simplify_and_warn(..., PATH_CHECK_ABSOLUTE), so the absolute check runs against the expanded string:

  • config_parse_working_directory — src/core/load-fragment.c
  • config_parse_unit_env_file — src/core/load-fragment.c
  • config_parse_unit_mounts_for — src/core/load-fragment.c
  • config_parse_exec_input — src/core/load-fragment.c (the file: branch)

%S/foo expands to /var/lib/foo, %h/.config/x to /home/user/.config/x, and so on.
The plugin cannot expand specifiers, so it has to accept a leading %X as a possible
absolute path.

Evidence

46 occurrences across a 6,680-unit Debian corpus, all currently flagged. Examples:

  WorkingDirectory=%S/osmocom              x22  (osmo-trx, osmo-cbc, ...)
  WorkingDirectory=%S/labgrid-coordinator  x2
  WorkingDirectory=%E/wireguard
  WorkingDirectory = %h                    x2   (bare specifier, no trailing path)
  EnvironmentFile=-%t/<name>/env_session.conf  x6
  EnvironmentFile=-%h/.config/gpu-screen-recorder.env
  EnvironmentFile=-%E/yubikey-touch-detector/service.conf
  RequiresMountsFor=%f                     x4   (systemd's own systemd-loop@.service)
  RequiresMountsFor=%t/containers          x2

Suggested fix

grammar/Combinators.kt already has the right building blocks from #509. The start rule
is PATH_START = (?:/|%\S), and there are two shapes depending on whether the setting
splits its value:

  • UNIT_PATH — the setting hands the parser the whole rvalue (no splitting, no unquoting,
    no unescaping). Use for WorkingDirectory=, EnvironmentFile= and StandardInput=file:.
  • QUOTABLE_UNIT_PATH — the setting splits with extract_first_word(). Use for
    RequiresMountsFor= / WantsMountsFor=, which pass EXTRACT_UNQUOTE.

Two things to preserve while doing it:

  1. WorkingDirectory= also accepts a bare ~, and an optional leading - before the
    specifier. EnvironmentFile= also takes a leading -, but note systemd strips it from
    the expanded string (path_simplify_and_warn(n[0] == '-' ? n + 1 : n, ...)).
  2. RequiresMountsFor= currently has the same space/quoting bug Symlinks= had before
    More validators #509extract_first_word(..., EXTRACT_UNQUOTE) means "/mnt/My Data" is one valid
    entry. Switching it to QUOTABLE_UNIT_PATH fixes both bugs at once.

Verification

ConditionAndAssertInspectionTest / UnitPathAndUnitNameInspectionTest have the pattern to
copy: assert the specifier forms accepted, relative paths still rejected, and (for
RequiresMountsFor=) quoted entries containing spaces accepted. Run the suite under both
engines — ./gradlew test and ./gradlew test -Dsystemd.unit.grammarParseEngine=true.

All references above are to systemd a8e93919c3, the commit recorded in
systemd-build/build/last_commit_hash.

One thing I'd flag if you're triaging: RequiresMountsFor=%f comes from systemd's own shipped systemd-loop@.service, so this fires on units people actually have installed, not just hand-written ones.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions