fix(image): bind hardlinks to the file they name when indexing a layer - #670
Merged
Conversation
…truth Adds a hardlink fixture image and tests over it that state what a container actually serves for each shape. These are the spec, not a snapshot, so the cases we get wrong fail. Fixed in the following commit. The layouts come from a real `docker build`, so they are measured rather than assumed. Two properties fell out that are easy to guess wrong and worth having written down: - the data section goes to the alphabetically FIRST name, not the one created first, so `printf > orig.txt && ln orig.txt hard.txt` puts the bytes on `hard.txt` and emits `orig.txt` as the hardlink header - `ln` across layers cannot produce a hardlink header at all; overlayfs copies the file up, so the layer diff carries a full regular-file copy. a hardlink header's target is therefore always in the same layer, and always precedes it The rule the cases encode is that a hardlink is a second name for an inode, fixed when the link is created. Replacing, overwriting or deleting the other name later does not change what this name refers to. Currently failing: - `/c6/sym.txt` serves nothing. `ln` onto a symlink hardlinks the symlink inode, so the name is a symlink and should read through to the real file - `/c10/z.txt` resolves to a path that was whiteouted out of the squash - every hardlinked name reports size 0, because the size comes from a tar header that has no data section - hardlinked names are unreachable by MIME search, so a hardlinked binary is silently skipped by anything that enumerates that way Two layers of coverage: an integration test over the real fixture across every image source, and a docker-free equivalent in the unit suite that builds the same layers in-process (which can also express layouts docker will not emit). The synthetic layer helpers are lifted from #665, which introduced them. The fixture directory is named `image-hardlinks` deliberately: the CI cache fingerprint in testdata/Makefile globs `image-*`, so a fixture named anything else builds fine locally but silently breaks cache invalidation. Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
A hardlink is a second name for an inode that already exists in the same layer, not a path to look up later. We store only the link path and re-resolve it against the squashed tree, which makes a hardlink behave like a symlink and gets four things wrong: - two names that end up naming each other read as a link cycle, aborting resolution for callers that walk files. this is what uv and pip produce: install a file, hardlink it into a content-addressed cache, then replace the installed name with a symlink aimed at the cache name - if a later layer overwrites or replaces the other name, we report that layer's contents. on disk the hardlink still refers to the original inode - if a later layer deletes the other name, we report the file as missing, though the container serves it fine - the entry reports size 0 and no MIME type, because a hardlink tar header has no data section. anything enumerating by MIME type skips the file silently Binding when the layer tar is read instead of during path resolution fixes all four, and leaves pkg/filetree untouched. The catalog is keyed by file.ID and is never squashed or merged, so what a name refers to cannot be changed by a later layer, which is exactly the guarantee the filesystem gives. Two measured properties make it safe: a hardlink header's target is always in the same layer (`ln` across layers forces an overlayfs copy-up, so the tar carries a full copy instead), and it always precedes the hardlink header. Mode, uid, gid and mtime are left alone; tar already records those correctly. Type is adopted too, so a hardlink naming a symlink is a symlink, which is what the kernel reports and what the container serves. One visible consequence: a hardlinked name is now catalogued as the file it names, so `TypeHardLink` no longer appears for well-formed images and such a name resolves to itself rather than to the other name. That is what syft #5029 wanted from the other side, and its `hardLinkAtPath` workaround becomes redundant rather than broken. Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
/bin/busybox and /bin/[ are two names for one inode, and neither is a link to the other, so /bin/busybox now resolves to itself. Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
spiffcs
approved these changes
Aug 17, 2026
spiffcs
left a comment
spiffcs
left a comment
Contributor
There was a problem hiding this comment.
@wagoodman I'm good with this over my original approach. I noticed this is in draft. Is there anything else you wanted me to help add?
wagoodman
marked this pull request as ready for review
August 18, 2026 12:37
adopting the target's metadata also handed its size to the layer size accumulator, so every hardlinked name counted the same bytes again. those bytes exist once on disk and once in the tar, and a hardlink header has no data section at all. `busybox:latest` has 410 hardlink headers over 16 regular ones, which took its reported layer size from 4.4 MB to 431 MB, and `Image.Metadata.Size` sums that. syft surfaces both as `imageSize`. the accumulator now reads `entry.Header.Size` rather than `metadata.Size()`. the two are the same value for everything except an adopted hardlink, and reading the header directly means a later mutation can't silently change what gets summed. the per-file `Size()` is unchanged and still reports the size of the file a name refers to. Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
…read adoption rewrites `metadata.Type`, and that type picks which of `AddFile` / `AddSymLink` / `AddDir` the builder calls. each of those errors out if a node already sits at that path with a different type, and the indexer passes that straight up, so `Image.Read()` fails for the whole image. a duplicate tar member is enough to trip it: a hardlink with a dangling linkname lands as a hardlink node, and a second member for the same path that does adopt then collides with it. this reads fine on `main`, and `Linkname` is attacker-controlled on an untrusted image, so a shape docker will never emit shouldn't be able to abort the read. a conflict now costs that one entry its adoption instead. the retry is gated on whether adoption happened rather than on the error text, since `pkg/filetree` returns bare `fmt.Errorf` here with nothing to match on. an adopted hardlink is the only entry whose claimed type differs from its own header, so it's the only one whose add can fail for a reason we introduced. anything else still propagates, including a failure on the un-adopted retry. Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
Contributor
Author
|
I want to spend more time with this... I think it's the right direction, but not sure it's complete yet. |
tar records no file type bits on a link header, so keeping the whole mode from that header left `Type` and `Mode()` describing different things: a hardlink naming a symlink came out as `TypeSymLink` with a mode that `file.TypeFromMode` read back as a regular file. two names for one inode share a mode on disk, including its type. permission bits still come from the name's own header, which is what tar does record correctly. Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
indexing a hardlink read `openerByID` directly while other writes to it go through the mutex, so calling `Layer.Read` concurrently against one shared catalog races. in Go a map read against a concurrent write is a fatal throw, not something a caller can recover from. nothing does that today, since `Image.Read` walks layers in order, but `Layer.Read` is exported and takes the catalog as a parameter, which is exactly the concurrency the mutex is there to allow. Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
a hardlink name is now indexed as the file it refers to, so nothing resolves one as a link: `ResolveLinkByLayerSquash` and `ResolveLinkByImageSquash` only handle symlinks now. also drops "verbatim" from the link name lookup, which claimed the opposite of what the code does. the lookup retries following ancestor symlinks on a miss, matching what link(2) does when the archive is extracted for real. Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
`AddHardLink` and `AddDir` both reported "is NOT a symlink file" on a type conflict, which is misleading in exactly the situation where the message is the only thing you have to go on. Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
two malformed shapes made a hardlink name come out as a directory: an empty link name cleans to the tree root, and a link name can point at a directory outright. `link(2)` returns EPERM for a directory, so no filesystem holds either one. both now fall back to the entry's own header, alongside the existing case of a link name that isn't in the layer at all. Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
a hardlink naming another hardlink took its description wholesale, including a link destination that was never in its own header, and reported success so nothing logged it. chaining onto a name that already failed just spreads the failure. it now falls back like the other malformed shapes, keeping the link destination its own header gives it. Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
`Sys()` is left as the un-adopted header on purpose, and reads as a contradiction next to the adopted fields unless it says so. also writes down why only the current layer is searched for a link name, since the obvious next step, walking each lower tree in turn, resolves names an intervening whiteout already deleted. Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
4 tasks
wagoodman
added a commit
to anchore/syft
that referenced
this pull request
Aug 20, 2026
temporary pin to anchore/stereoscope#670 so the hardlink changes can be exercised end to end. needs re-pinning to a release tag before merge. pulls transitive bumps along with it: docker/cli, docker/go-connections and gabriel-vasile/mimetype. Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
wagoodman
added a commit
to anchore/syft
that referenced
this pull request
Aug 20, 2026
temporary pin to anchore/stereoscope#670 so the hardlink changes can be exercised end to end. needs re-pinning to a release tag before merge. pulls transitive bumps along with it: docker/cli, docker/go-connections and gabriel-vasile/mimetype. Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
wagoodman
added a commit
to anchore/syft
that referenced
this pull request
Aug 20, 2026
* chore(deps): pin stereoscope to the hardlink-inode-adoption branch temporary pin to anchore/stereoscope#670 so the hardlink changes can be exercised end to end. needs re-pinning to a release tag before merge. pulls transitive bumps along with it: docker/cli, docker/go-connections and gabriel-vasile/mimetype. Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com> * fix: drop the image resolver hardlink workaround stereoscope now adopts a hardlink's target at index time, so a hardlinked name arrives already described as a regular file with the target's size, mime type and content. the resolver-side fix from #5029 sat on top of that doing the same job a second time, gated on a file type that no longer shows up, so both `resolveHardLinkTarget` implementations and the extra tree walk they cost on every `FilesByPath` are gone. `file.NewVirtualLocationFromImage` stays as-is. it is exported and syft is v1, so it keeps working for anyone using it, it just has no callers in syft now. this picks up `FilesByMIMEType`, which #5029 explicitly could not fix, so hardlinked names now reach mime-driven catalogers. on images built around multi-call binaries (busybox and friends) that is a lot more file and executable entries than before; distro and toolchain images move by about one entry. one shape regresses: a hardlink stereoscope could not adopt (a link name that is absent, empty, names a directory, names another un-adopted link, or points into a lower layer) keeps `TypeHardLink`, and syft goes back to collapsing it onto its target's path. no mainstream builder emits the cross-layer case. Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com> * bump stereoscope to main Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com> --------- Signed-off-by: Alex Goodman <wagoodman@users.noreply.github.com>
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
2 participants
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.
This is an alternative to #665 that came up during its review.
A hardlink is a second name for a file that already exists, not a path to look up later. We store only the link path and re-resolve it against the squashed tree, which makes a hardlink behave like a symlink and gets a few things wrong:
uvandpipproduce (install a file, hardlink it into a content-addressed cache, then a later layer replaces the installed name with a symlink aimed at the cache name) and it aborts resolution for anything walking filesThe fix binds the name to the file it refers to when the layer tar is read instead of re-resolving during path resolution, so
pkg/filetreeis untouched. The catalog is keyed byfile.IDand is never squashed or merged, so a later layer can't change what a name refers to, which is the guarantee the filesystem actually gives.One visible change worth calling out: a hardlinked name is now catalogued as the file it names, so
file.TypeHardLinkno longer appears for well-formed images and such a name resolves to itself rather than to the other name. On disk neither name is "the hardlink", so this is closer to the truth and it's what syft #5029 wanted from the other side, but it does mean nothing can answer "is this path also known as something else" anymore.Tests come in two layers, a real
docker buildfixture over 11 hardlink shapes across every image source plus a docker-free equivalent in the unit suite, and the first commit is deliberately red so the broken cases are visible before the fix lands.