Define native WSL layout and state identity - #83
Conversation
CherylSnowVeil
left a comment
There was a problem hiding this comment.
Review: Define native WSL layout and state identity
Initial review of 6e4c10b against main (21297d4). Scope is tight: internal/hostenv/wsl_layout.go plus its test and the contract docs. I verified the pinned test vector independently — sha256("container-bin/wsl2-state/v1\x00" + "Ubuntu-24.04\x00" + machineID + "\x00" + "1000")[:16] does produce wsl2-98d6411d95851ca46df32643077388c2 — and CI is green.
Findings
1. [important] Distro identity is trimmed before it reaches the validator — internal/hostenv/hostenv.go:92, internal/hostenv/wsl_layout.go:45,82
classify stores Distro: strings.TrimSpace(distro), so through the real Current() path the padding rejection in validateDistroIdentity is unreachable. Two distinct WSL_DISTRO_NAME values ("Ubuntu" vs " Ubuntu") yield the same Runtime.Distro and therefore the same StateNamespace. That contradicts the stated contract — "the exact case-sensitive WSL_DISTRO_NAME value" and "ContainerBin does not normalize identities" in docs/wsl.md — and once volumes are namespaced by this value, two separately registered distributions would silently share ContainerBin state. This is the same issue already raised in the unresolved review thread on wsl_layout.go:46; confirming it still stands at this head. Note the new test exercises NativeWSLLayout directly with a padded Distro, which gives false confidence because classify can never produce one. Remediation: preserve the raw WSL_DISTRO_NAME in Runtime.Distro (keeping whitespace-only handling correct for the Windows interop-marker and missing-distro checks), and add a test that routes a padded distro through classify.
2. [important] Windows-filesystem home detection is a /mnt prefix check only — wsl_layout.go:106, docs/wsl.md:52
The contract is to reject Windows-filesystem homes, but WSL's DrvFs automount root is configurable via /etc/wsl.conf (automount.root, e.g. /windir/ or /), and a home can also reach DrvFs through a symlinked parent. A home like /windir/c/Users/alice is canonical, absolute, and not under /mnt, so it is accepted — registry, lockfile and private state would then land on a Windows filesystem despite the check. A string prefix check cannot fully deliver this contract; before the frontend is enabled the enforcement needs a filesystem-level check (statfs f_type, /proc/mounts, or parsing automount.root). At minimum the doc should mark the current check as best-effort so the later filesystem slice doesn't treat it as complete.
3. [nit] Misleading error for padded distro names — wsl_layout.go:82-84
A padded but otherwise valid distro name (e.g. " Ubuntu") fails with "not canonical UTF-8". The value is valid UTF-8; it fails the whitespace rule. A separate message for the padding check would be clearer.
Summary
The derivation itself is sound: an unambiguous \x00-delimited identity (distro cannot contain NUL since control characters are rejected), a versioned domain separator, a 128-bit-truncated SHA-256 producing an opaque Docker-name-safe namespace, strict canonical home validation with no ../trailing-slash/double-slash bypasses, and canonical machine-id validation including the all-zero case. Both important findings are cases where the stated contract is stronger than what the code enforces on the real path. Neither is exploitable today because the WSL frontend remains gated, but both should be resolved — or explicitly deferred — before the namespace wiring slice lands.
CherylSnowVeil
left a comment
There was a problem hiding this comment.
Re-review: Define native WSL layout and state identity
Reviewed current head ab8a1e4 against main (21297d4). CI is green. This re-review first validates the findings from my previous round on 6e4c10b, then covers a complete fresh pass over the current diff.
Validation of previous findings
1. Distro identity trimmed before the validator — FIXED. classify now stores the raw WSL_DISTRO_NAME (internal/hostenv/hostenv.go:92), so validateDistroIdentity in NativeWSLLayout can no longer be bypassed by padding. The Windows path uses the raw distro != "" test, so a whitespace-only WSL_DISTRO_NAME on Windows still fails closed as an interop marker rather than being silently ignored — the new windows whitespace distro marker case covers this. requireFrontend uses strings.TrimSpace(info.Distro) == "" only for the missing-identity diagnostic, which preserves the intended semantics for whitespace-only values. The new TestClassifiedPaddedDistroDoesNotCollapseStateIdentity exercises the real classify path instead of hand-constructing a Runtime, which resolves the false-confidence concern from last round.
2. [important] Windows-filesystem home detection is still a /mnt prefix check — STILL PRESENT. validateWSLHome (internal/hostenv/wsl_layout.go:105-107) is unchanged, and docs/wsl.md:50-53 still states the stronger contract — "a canonical absolute Linux path in the distribution filesystem" — than the code can deliver. DrvFs automount root is configurable via /etc/wsl.conf (automount.root), and a home can also reach a Windows filesystem through a symlinked parent; neither is detectable by a string prefix check. Since the frontend remains gated this is not exploitable today, but the gap should be explicitly captured — either mark the check as best-effort in docs/wsl.md, or add filesystem-level home detection (statfs//proc/mounts/automount.root) to the open enforcement item list so the later slice doesn't treat this as already done.
3. [nit] Misleading error for padded distro names — STILL PRESENT. validateDistroIdentity still reports "not canonical UTF-8" for whitespace-padded input (wsl_layout.go:82-84). Still optional; a distinct message would be clearer but this is cosmetic.
Fresh review of the current head
No new actionable findings. Specifically verified:
- The removed
interop = strings.TrimSpace(interop)only affects the emptiness test; a whitespaceWSL_INTEROPnow counts as a marker, which is the fail-closed direction. - The raw
Distrois only consumed byNativeWSLLayout(which validates before hashing) and the gated diagnostic, which uses%qso control characters cannot inject terminal escapes. - The
\x00-delimited identity remains unambiguous: distro control characters (including NUL) are rejected, machine ID is fixed-width hex, and the UID is decimal. - Interior spaces and non-control Unicode in a distro name are hashed raw — consistent with the documented "does not normalize identities" contract; two spellings produce distinct namespaces rather than collapsing.
- The new regression test asserts the raw value survives
classifyand that layout derivation rejects it before namespace computation. - Docs (
architecture.md,roadmap-decisions.md,wsl.md) are consistent with the implemented layout, tuple, and gating state.
Summary
The head commit correctly and minimally resolves the main defect from the previous round, with appropriate regression coverage. One important finding remains open (the /mnt prefix check vs. the documented distribution-filesystem contract) — it is non-blocking today only because the WSL frontend is still gated, but it should be resolved or explicitly deferred in the docs before the filesystem-enforcement slice lands. One optional nit also remains.
Summary
This slice starts from current
mainafter #77 and is independent of the open project-overlay and self-update PRs.Validation
gofmt/git diff --checkgo vet ./...go test -race ./...python -m unittest -v internal/registry/pipx_wrapper_test.py internal/cli/pipx_discovery_test.pygo list -deps ./...v0.0.0-citestversion smokeRoadmap: #2 (WSL2 native config/shim/state layout foundation). Remaining WSL filesystem enforcement, namespace wiring, Docker Desktop integration, path/signal work, and real E2E qualification stay open.