erofs: first batch of #2408 follow-ups - #2410
Conversation
isOpaqueDir returned false whenever statOn failed, so a layer whose xattr region could not be read was treated as a layer that hides nothing. That fails open: the entries the opaque directory was meant to hide from lower layers get listed instead, and nothing tells the caller the answer was a guess. Layers come from an OCI image that may be untrusted, so a damaged or hostile one should not be able to widen the merged view by being unreadable. Return (bool, error) and propagate anything that is not fs.ErrNotExist. Absent is the one benign case -- a layer that does not have the directory genuinely is not opaque there. Both callers, lookup and mergeDir, already return errors. The new test puts the unreadable layer in the middle of three on purpose: that is the only position where isOpaqueDir is the first thing to fail. With the broken layer on top, lookup's ancestor stat raises first and the fixture would pass either way. Against the old code it reports ReadDir(etc) = [foo secret] with a nil error, which is the leak. Refs chainguard-dev#2408
An application/vnd.erofs+zstd layer is a spec-legal EROFS image that apko cannot read yet, but it hit the mediaType check and was told the command "only handles EROFS images" -- which is both wrong and no help in working out what to do next. Match any "application/vnd.erofs+<codec>" mediaType, name the codec, and point at chainguard-dev#2406, which is where reading them will come from. The check is on the suffix rather than a list of media types because the draft spec's set of codecs is not something apko should pin down here. Refs chainguard-dev#2408
generateIndexWithMediaType copies the finished config's os.features onto the index platform descriptor for every format, and BuildImageFromLayers DeepCopies the base image's config. So a plain tar build on a base image that already declares os.features now surfaces those features on the index descriptor, changing that index's digest. Spec §5.4 asks for exactly this, but the test added with the feature covered only the empty-base case, leaving the interaction to be rediscovered by whoever next changes either side. Also use require.Nil rather than require.Empty for the "tar builds declare nothing" assertion, which is what it means to check -- that the propagation does not invent an empty slice. Harmless either way, since os.features carries omitempty in go-containerregistry and an empty-but-present slice cannot change a digest. Refs chainguard-dev#2408
Two claims in the tree are stronger than what the spec says or what the code does. Spec §3.7's materialize-or-fail rule governs cross-layer hardlinks. For links within a single layer the spec says nothing, so apko materializing them is conformant but not blessed by that section. The squash message for chainguard-dev#2249 has this right; the comment in erofs.go and the docs bullet do not. docs/erofs.md said `apko erofs ls` presents "the merged view the kernel would assemble". It approximates it, and diverges in two corners: a middle-layer whiteout at a directory's own name does not cut off lower layers when a higher layer recreates the directory, and opacity is not inherited by descendant directories. Both need two or more layers, so neither is reachable for an image apko produces today. Name them and link chainguard-dev#2408, where the fix is tracked, rather than let the doc promise kernel parity. Refs chainguard-dev#2408
mattmoor
left a comment
mattmoor
left a comment
There was a problem hiding this comment.
All four commits verified against the code, and the PR body's claims are accurate.
The isOpaqueDir change is correct: fs.ErrNotExist is the right benign case, both callers propagate, and the middle-layer placement of the broken fixture (so the test discriminates against the old code instead of passing via lookup's ancestor stat) is exactly the right way to avoid a vacuous regression test. The +zstd error still refuses the layer, and the suffix-match rationale (don't pin the draft spec's codec set into pkg/build/types to phrase one error) is sound. The base-image os.features test pins the digest-affecting case I asked about on #2249, and require.Nil is the assertion the comment always meant. The §3.7 and ls docs rewordings match the spec text and the actual divergences, now named and linked to #2408.
This answers both threads left open on #2249; I'll resolve them pointing here. One symmetry note left inline on stack.go, take or leave. The make lint breakage on main is worth its own one-liner whenever.
isOpaqueDir surfaces stat failures, but isWhiteout a screen above still returned false on an e.Info() error, so the two tombstone checks had opposite error philosophies. The whiteout failure mode is milder -- an unreadable char device is treated as live, occupying the name rather than leaking lower entries -- but it is still a guess about what the merged view shows, made from an inode we could not read. Give isWhiteout the same (bool, error) signature and propagate from both callers, lookup and mergeDir. The test fixture grows an infoErr map so overlayEntry.Info() can fail for a named path. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
mattmoor
left a comment
mattmoor
left a comment
There was a problem hiding this comment.
Re-approving at 30f66a5. The new commit is the suggested treatment done right: isWhiteout matches isOpaqueDir's (bool, error) shape with both callers propagating, the cheap Type() path still avoids the Info() call for non-chardevs and single-layer stacks, and the mirrored test pins both ReadDir and Stat surfacing the error. The absent fs.ErrNotExist special case is correct here: DirEntry.Info() can only return not-exist for a file removed between the directory read and the stat, which cannot happen in an immutable EROFS layer.
2 participants
First batch of the EROFS follow-ups from #2408 — the items that touch merged
mainonly, so none of them wait on the descoped mount/umount or layering codecoming back. One real behavior change, two tests, two rewords.
isOpaqueDirfails closed on an unreadable layerisOpaqueDirreturnedfalsewheneverstatOnfailed, so a layer whose xattrregion could not be read was treated as one that hides nothing. That fails
open: the entries an opaque directory was meant to hide from lower layers get
listed anyway, and nothing tells the caller the answer was a guess. Layers come
from an OCI image that may be untrusted, so a damaged or hostile one should not
be able to widen the merged view by being unreadable.
It now returns
(bool, error)and propagates anything that is notfs.ErrNotExist— absent is the one benign case, since a layer that lacks thedirectory genuinely is not opaque there. Both callers (
lookup,mergeDir)already returned errors.
The test puts the unreadable layer in the middle of three on purpose: that is
the only position where
isOpaqueDiris the first thing to fail. With thebroken layer on top,
lookup's ancestor stat raises first and the fixture wouldpass either way. Against the old code it reports
ReadDir(etc) = [foo secret], <nil>— the lower layer'ssecretleaking intothe merged view.
application/vnd.erofs+zstdgets an accurate errorA compressed layer is a spec-legal EROFS image apko cannot read yet, but it hit
the mediaType check and was told the command "only handles EROFS images". Now
any
application/vnd.erofs+<codec>names its codec and points at #2406.Matching on the suffix rather than adding a media-type constant is deliberate:
the draft spec's set of codecs isn't something apko should pin down in
pkg/build/typesto produce one error message.os.featurespropagation from a base image config, pinnedgenerateIndexWithMediaTypecopies the finished config'sos.featuresonto theindex platform descriptor for every format, and
BuildImageFromLayersDeepCopies the base image's config. So a plain tar build on a base image
that already declares
os.featuressurfaces them on the index descriptor,changing that index's digest. Spec §5.4 asks for this and #2249's squash message
calls it out, but the test added there covered only the empty-base case.
Also
require.Empty→require.Nilfor the "tar builds declare nothing"assertion, which is what it means to check — that the propagation doesn't invent
an empty slice. Harmless either way:
os.featurescarriesomitemptyingo-containerregistry, so an empty-but-present slice can't change a digest.
Two claims softened to match reality
cross-layer hardlinks; for links within one layer the spec is silent, so
apko materializing them is conformant but not blessed by that section. Add erofs support to apko. #2249's
squash message has this right;
pkg/build/erofs.goanddocs/erofs.mddidnot.
docs/erofs.mdpromised kernel parity forapko erofs ls("the mergedview the kernel would assemble"). It approximates it, and diverges in two
corners: a middle-layer whiteout at a directory's own name doesn't cut off
lower layers when a higher layer recreates the directory, and opacity isn't
inherited by descendant directories. Both need 2+ layers, so neither is
reachable for an image apko produces today. The doc now names them and links
EROFS follow-ups: mount/umount hardening, layer splitting, go-erofs bump #2408, where the algorithm fix (a per-directory layer horizon) is tracked.
Verification
go test ./...passes,golangci-lint run -nreports 0 issues,gofmt -lisclean.
Unrelated note found along the way:
make lintis currently broken onmain—its target installs
github.com/golangci/golangci-lint/cmd/golangci-lint/v2@v2.2.1, which no longerresolves (
invalid version: unknown revision cmd/golangci-lint/v2.2.1). CIisn't affected, since
verify.yamlusesgolangci-lint-action. Left alonehere; happy to fix it separately.
Not in this PR
The rest of #2408: restoring mount/umount (with state-file containment,
partial-unmount recovery, a read-only default and a
Drivertest seam),restoring multi-layer splitting, the layer-horizon fix, and the go-erofs bump.
The pre-descope tree those two build on is preserved at
https://github.com/smoser/apko/tree/feat/apko-erofs-full.
Refs #2408
🤖 Generated with Claude Code