Conversation
…s and tunnel endpoints
Fifteen more (parser, ltype) pairs that had no value validator, picked by how often
the keys that resolve to them occur in a 6,680-unit corpus of Debian unit files.
Together they cover ~2,600 key occurrences in that corpus. Every grammar was written
against systemd a8e93919c3 — the same commit the plugin's gperf/man data is generated
from — and cites the C function it mirrors.
[Unit] conditions (config_parse_unit_condition_path / _string):
condition_path (*) every path-valued Condition…=/Assert…=; one wildcard entry
because all twelve ltypes parse identically
CONDITION_ARCHITECTURE architecture_table + "native"
CONDITION_VIRTUALIZATION booleans, vm/container/private-users, virtualization_table
CONDITION_SECURITY the closed list in condition_test_security
CONDITION_CAPABILITY one capability name, or a number in 0…CAP_LIMIT
Enumerations and address forms:
config_parse_netdev_kind [NetDev] Kind=
config_parse_set_status Success/RestartPrevent/RestartForceExitStatus=
config_parse_name_policy [Link] NamePolicy= (has kernel/keep, unlike
AlternativeNamesPolicy=)
config_parse_wireguard_peer_key base64 32-byte key, or @credential
config_parse_tunnel_local_address address, "any", or a local-address type
config_parse_tunnel_remote_address address or "any" — no local-address types
config_parse_address_generation_type prefixstable/eui64/static IPv6 tokens
Also fixes two pre-existing mis-mappings. CONDITION_CPU_FEATURE and
CONDITION_CONTROL_GROUP_CONTROLLER were both pointed at the boolean condition grammar,
which flagged every legitimate value — ConditionCPUFeature= alone produced 110 false
positives in the corpus. CPU features have no closed list (has_cpu_with_flag() reads
/proc/cpuinfo), so that one now only pins the shape: exactly one token, which is real,
since the parameter is never split.
The shared conditionPath()/conditionString() helpers spell the trigger/negate marker
combinations out as alternatives instead of wrapping each marker in a ZeroOrOne.
ZeroOrOne probes its inner combinator a second time to report reachable input, and the
classic engine derives its error TextRange from that number, so a value like
`!!/some/path` produced a range past the end of the value and tripped an assertion.
ConfigParseUnitConditionStringOptionValue now uses the helper too, fixing the same
latent crash there.
Checked for false positives by running every registered grammar over the whole corpus:
of 16,786 grammar-validated key occurrences, these validators reject 129, and all 129
are deliberately-malformed fixtures — KDE's syntax-highlighting test input, systemd's
own negative prefixstable/vti test cases, and one unsubstituted @etc@ autoconf template.
Whole suite green under both the classic and the list-of-successes engine.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…p] and unit paths
Thirty-four more (parser, ltype) pairs, taking the undocumented-validator count from 325 to 291.
As with the previous batch, every grammar is written against systemd a8e93919c3 — the commit the
plugin's gperf/man data is generated from — and names the C function it mirrors.
config_parse_routing_policy_rule, config_parse_route_section, config_parse_address_section and
config_parse_nexthop_section are dispatchers: the ltype indexes a ConfigSectionParser table and each
entry names the parser that actually reads the value. These commits mirror the tables entry by entry
rather than giving a whole section one grammar.
[RoutingPolicyRule] From=/To= (address + optional prefix), IncomingInterface=/OutgoingInterface=,
Priority=, GoTo=, TypeOfService=, SuppressPrefixLength=,
SuppressInterfaceGroup=, SourcePort=/DestinationPort=, Family=, Type=,
Invert=, L3MasterDevice=
[Route] Destination=/Source=, PreferredSource=, GatewayOnLink=, Scope=
[Address] Peer=, Broadcast=, PreferredLifetime=
[NextHop] Gateway=
networkd/udev [Match] Name= and .link OriginalName=, [BridgeVLAN] VLAN=, [Tunnel] Key=,
[IPv6Prefix] *LifetimeSec=, MTUBytes= for all three ltypes
unit files [Path] watch settings, [Socket] Symlinks=, [Socket] Service=,
[Service] Sockets=
Fixes two more pre-existing mis-mappings of the same shape as the CONDITION_CPU_FEATURE one. Every
[Address] ltype shared the uint32 grammar written for RouteMetric=, and every [NextHop] ltype shared
the boolean grammar written for OnLink=. Between them they flagged `AddPrefixRoute=no`,
`HomeAddress=yes`, `ManageTemporaryAddress=yes`, `DuplicateAddressDetection=ipv4`, `[NextHop] Id=20`,
`Family=ipv4` and `Group=1:3 20:1` — about 100 false positives in the corpus, all on values straight
out of systemd's own test-network fixtures.
Two things learned from checking against that corpus, now folded into shared combinators:
unsignedNumber() — systemd's safe_atou* helpers pass base 0 to strtoul, so a number may be written
in hex or octal. IntegerTerminal only reads decimal, so `TypeOfService=0x08` (from systemd's
25-fibrule-uidrange.network) would have been flagged. Only the decimal branch carries the numeric
bounds; the other two err towards accepting rather than flagging.
INTERFACE_NAME / ALTERNATIVE_INTERFACE_NAME — ifname_valid_full(), including the rules that a name
which is entirely digits is refused (it would look like an ifindex), as are `.`, `..`, `all` and
`default`, and that the length cap is 15 or 127 depending on the caller's flags.
[Address] Address= keeps its existing NetworkAddressOptionValue, which is deliberately stricter than
in_addr_prefix_from_string_auto (it wants an IPv4 prefix of at least /8 and an explicit IPv6 prefix)
and has tests asserting that; Peer= is the same table entry and now shares it.
Re-ran every registered grammar over the 6,680-unit corpus: of 18,331 grammar-validated key
occurrences these validators reject 28, and all 28 are correct — KDE's syntax-highlighting test
input, systemd's own `hoge`/`foofoo`/`::1` negative cases, a fuzz corpus file, and one genuine bug in
a shipped Debian unit (openqa-minion-restart.path sets PathChanged= to an unsubstituted relative
path, which systemd rejects). Whole suite green under both engines.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… sources per file
A code review found ten confirmed defects in the two preceding commits. Six were false
positives — values systemd accepts that the plugin flagged — which is the failure mode
that matters most here, since a spurious red squiggle on a working unit file is worse
than no validation at all.
The root cause was method, not carelessness: the grammars were derived from the
config_parse_* entry points but the reading often stopped there instead of following
through to the leaf helper, and the corpus of real Debian units was allowed to stand in
as evidence of correctness. A corpus can show that a grammar rejects something real; it
cannot show that a grammar accepts everything legal, because legal-but-unused spellings
simply aren't in it. Every fix below is derived from the C at a8e93919c3 — the commit
systemd-build/build/last_commit_hash pins — with the man page as a cross-check.
False positives fixed:
paths config_parse_unit_condition_path and config_parse_path_spec hand the
parser the whole rvalue: no splitting, no unquoting, no unescaping.
path_is_valid_full() restricts no characters, so `/mnt/My Data` is one
perfectly good path. The old grammar stopped at the first unescaped
space (a convention copied from the plugin's own PathOptionValue, not
from systemd) and wrongly honoured backslash escapes. Split into
UNIT_PATH and QUOTABLE_UNIT_PATH, since [Socket] Symlinks= IS split,
with EXTRACT_UNQUOTE, and so does take quotes and escapes.
numbers safe_atou* pass base 0 to strtoul, so hex and octal are legal wherever
they are used. New UnsignedNumberTerminal parses in the actual base, so
the range check survives the change of spelling — the previous
alternation could only bound its decimal branch and silently let
`TypeOfService=0400` (256) through.
MTU parse_mtu bounds the value, not the spelling: `IPv6MTUBytes=1K` is 1024
bytes and under IPV6_MIN_MTU. New ByteSizeTerminal evaluates the suffix.
Peer= takes the faithful grammar rather than inheriting Address='s stricter
one; a bare `Peer=2001:db8::2` is accepted by systemd.
DAD config_parse_address_dad tries parse_boolean() FIRST and accepts it with
a "For historical reasons" warning. Booleans are now accepted and marked
deprecated, which repeats systemd's own advice instead of erroring. The
man page documents only the four family names — the source wins.
Match Name= `!` is consumed with a bare p += invert and the following
extract_first_word skips whitespace, so `Name=! eth0` is valid.
capabilities the gperf table is built with --ignore-case and capability_to_name()
renders lower case, so `cap_sys_admin` is as canonical as the upper-case
form. FlexibleLiteralChoiceTerminal grew an ignoreCase flag rather than
duplicating 41 names.
Beyond the ten findings, [Address] Address= had the same class of bug and predates this
work: it demanded /8…/32 for IPv4 and /64…/128 for IPv6, so it flagged
`Address=2600::1/0` — a line from systemd's own test/test-network/conf/25-veth-peer.network.
The prefix ranges are now the full family ranges. Requiring an explicit IPv6 prefix is
kept, because systemd warns about that itself. InvalidValueForNetworkAddressesTest is
updated accordingly (the `/7` case it asserted was wrong).
Every file the two commits touched now opens with a reference block naming the man page,
the parsing function, and the table or helper that decides the accepted set, all as links
pinned to a8e93919c3 so they stay valid. Combinators.kt and AiGenerated.kt additionally
record the two traps that produced most of these defects: AlternativeCombinator is
first-full-match with no backtracking under the classic engine, and a duplicate key in
AiGenerated.kt's mapOf silently wins.
Whole suite green under both engines. Re-running every registered grammar over the
6,680-unit corpus, the validators from these commits now reject only values that are
genuinely invalid: deliberately-malformed fixtures, plus an unsubstituted @etc@ template
and one relative PathChanged= in shipped Debian units.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Unit Test Results (grammar engine true)1 258 tests 1 258 ✅ 1m 3s ⏱️ Results for commit cfa0695. |
Unit Test Results (grammar engine false)1 258 tests 1 258 ✅ 1m 0s ⏱️ Results for commit cfa0695. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.