Skip to content

fix: read /etc/os-release with one parser that matches sh - #544

Open
rominf wants to merge 1 commit into
mainfrom
fix/os-release-one-parser
Open

rominf wants to merge 1 commit into
mainfrom
fix/os-release-one-parser

Conversation

@rominf

@rominf rominf commented Oct 5, 2026

Copy link
Copy Markdown
Collaborator

Fixes #543.

/etc/os-release was parsed in four places, each with its own trim_matches, and against sh sourcing the same file they disagreed with each other:

file sh driver plan rocm examine OpenMPI hint
ID='ubuntu' ubuntu 'ubuntu' 'ubuntu' ubuntu
ID=debian then ID=ubuntu ubuntu debian ubuntu debian

So a spec-valid single-quoted file read as an unsupported distro, and on a file with a duplicated ID, rocm examine reported Ubuntu while the driver plan was built for Debian 12.

The fix

All four readers now go through rocm_core::os_release, which keeps one rule: the value sh would assign, or nothing.

  • For a file of blank lines, comments and spec-valid assignments it returns sh's value: one matching quote pair removed, \" \\ \` \$ decoded inside double quotes, nothing decoded inside single quotes, \x meaning x in a bare value, a # comment after a value, the last assignment winning.
  • Any other line makes the whole file unreadable. To sh such a line is a command or the start of one (export ID=…, X=1; ID=…, unset ID, an unterminated quote that swallows the lines after it), and a command can change any variable — so reading past it would report values sh does not assign. CRLF files are unreadable too: sh assigns ubuntu\r, which matches no distro.
  • An unreadable file is reported, not hidden. The driver plan says /etc/os-release line N is not a plain assignment: "<line>"; no driver commands were planned because the distro cannot be read. instead of "this distro is not supported", and rocm examine records the same line in probe_failures.

Real files

Checked against dash: all 50 container images, and 396 of the 397 files in which-distro/os-release at 10dfd5bc, read exactly as dash reads them. The one exception is a CRLF file, which reads as unreadable. No file read a value dash does not assign.

Behaviour changes

  • rocm examine reports single-quoted and duplicated keys as sh reads them, and an unreadable file as a probe failure naming its line.
  • The driver plan — and the package installs it approves in ensure_openmpi_for_vllm and ensure_torch_runtime_dep — read the same distro rocm examine reports.
  • The OpenMPI hint no longer accepts ID =x; to sh that runs a command named ID.

Tests

  • The parser against /bin/sh itself, with an empty environment, a sentinel HOME and a PATH that does not exist: every spec-valid encoding of every value up to three characters, and every right-hand side up to three characters over a quote-, backslash- and newline-heavy alphabet, checked for the key it sets and a key after it. Each must read as sh reads it or not at all; a list of well-formed cases must read as a value; no earlier value may come back.
  • Nine mutations of the parser's rules each turn the suite red.
  • End to end through the driver plan: every supported distro rewritten in single quotes plans exactly as in double quotes; a duplicated ID plans from its last assignment; an unreadable file is refused with the message above and no commands. rocm examine's probe failure is asserted together with the state it describes.

Scenarios

There is no e2e scenario in this PR. /etc/os-release is read from its real path, and the seam that lets a scenario plant one is the host-root re-rooting in #510. Scenarios for a single-quoted file and a duplicated ID, through rocm install driver --dkms --dry-run, are written against that seam and follow once it lands.

Verification

On this commit: cargo fmt --check, both CI clippy -D warnings invocations, cargo test -p rocm-core --lib (474 passed), -p rocm --bin rocm (974 passed), -p xtask (255 passed), xtask manifest --check, tpn --check, check-crate-edges, and cargo xtask e2e (153 scenarios; the only failures are the two already marked expected). The new end-to-end tests were run against the code before this change and fail there for the reasons described above.

Locally, cli_progress::tests::animated_spinner_keeps_ticking_without_progress_calls — which needs three 5 ms ticks inside 60 ms — failed in two full runs on a heavily loaded machine and passed five of five on its own; it is unchanged by this PR. Not run locally: the Windows lane.

Four places parsed `/etc/os-release` — the driver plan (which also picks the
distro for the package installs it approves in `ensure_openmpi_for_vllm` and
`ensure_torch_runtime_dep`), `rocm examine`, the OpenMPI install hint, and
the `PRETTY_NAME` reader — each with its own `trim_matches`. Against `sh`
sourcing the same file, they disagreed with each other:

  file                      sh       driver plan  rocm examine  OpenMPI hint
  ID='ubuntu'               ubuntu   'ubuntu'     'ubuntu'      ubuntu
  ID=debian, then ID=ubuntu ubuntu   debian       ubuntu        debian

`os-release(5)` allows single quotes, so a spec-valid single-quoted file read
as an unsupported distro. And on a file with a duplicated `ID`,
`rocm examine` reported Ubuntu while the driver plan was built for Debian 12.

All four now read through `rocm_core::os_release`, which keeps one rule: the
value `sh` would assign, or nothing. For a file of blank lines, comments and
spec-valid assignments it returns `sh`'s value — one matching quote pair
removed, `\"`, `\\`, `` \` `` and `\$` decoded inside double quotes, nothing
decoded inside single quotes, `\x` meaning `x` in a bare value, a `#` comment
after a value, the last assignment of a key winning.

Any other line makes the whole file unreadable, not just the key it names.
To `sh` such a line is a command or the start of one — `export ID=…` and
`X=1; ID=…` assign, `unset ID` removes, an unterminated quote swallows the
lines after it — and a command can change any variable, including ones set
before it. Reading past it would mean reporting values `sh` does not assign.
A CRLF file is unreadable too: `sh` assigns `ubuntu\r`, which matches no
distro.

An unreadable file is reported, not hidden. `parse` names the first
offending line, and the driver plan's reason becomes `/etc/os-release line N
is not a plain assignment: "<line>"; no driver commands were planned because
the distro cannot be read.` instead of "this distro is not supported", while
`rocm examine` records the same line in `probe_failures`.

Behaviour changes:
- `rocm examine` reports single-quoted and duplicated keys as `sh` reads
  them, and an unreadable file as a probe failure naming its line.
- The driver plan, and the package installs it approves, read the same
  distro `rocm examine` reports.
- The OpenMPI hint no longer accepts `ID =x`; to `sh` that runs a command
  named `ID`.

Real files: run against `dash`, all 50 container images checked and 396 of
the 397 files in which-distro/os-release (10dfd5bc) read exactly as `dash`
reads them; the one exception is a CRLF file, which reads as unreadable.
No file read a value `dash` does not assign.

Tests:
- Against `/bin/sh` itself (empty environment, a sentinel `HOME`, a `PATH`
  that does not exist): every spec-valid encoding of every value up to three
  characters, and every right-hand side up to three characters over a
  quote-, backslash- and newline-heavy alphabet, checked for the key it sets
  and a key after it. Each must read as `sh` reads it or not at all, a list
  of well-formed cases must read as a value, and no earlier value may come
  back.
- Nine mutations of the parser's rules each turn the suite red.
- End to end through the driver plan: every supported distro rewritten in
  single quotes plans exactly as in double quotes; a duplicated `ID` plans
  from its last assignment; an unreadable file is refused with the message
  above and no commands. `rocm examine`'s probe failure is asserted with its
  state. These fail on the code before this change.

No e2e scenario here: `/etc/os-release` is read from its real path, and the
seam that lets a scenario plant one is the host-root re-rooting proposed in
#510. Scenarios for a single-quoted file and a duplicated `ID` are written
against that seam and follow once it lands.

Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
@rominf
rominf requested a review from a team as a code owner October 5, 2026 11:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

/etc/os-release is parsed four different ways, and they disagree on spec-valid files

1 participant