From cf4153c6c6ac3c95e20ac4c3e1bc9d39e2517ba0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=A0imon=20Brauner?= Date: Wed, 22 Jul 2026 17:58:18 +0200 Subject: [PATCH] Preserve dir mode and ownership in RUN --mount MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes: podman-container-tools#6747 Fixes: podman-container-tools/podman#27777 Signed-off-by: Šimon Brauner --- add.go | 16 ++++---- digester.go | 96 ++++++++++++++++++++++++++++++++++-------------- digester_test.go | 24 ++++++++---- image.go | 49 +++++++++++++++++------- tests/bud.bats | 24 ++++++++++++ 5 files changed, 152 insertions(+), 57 deletions(-) diff --git a/add.go b/add.go index e73aa8846d..2bccfef987 100644 --- a/add.go +++ b/add.go @@ -741,18 +741,18 @@ func (b *Builder) Add(destination string, extract bool, options AddAndCopyOption renamedItems := 0 writer := io.WriteCloser(pipeWriter) if renameTarget != "" { - writer = newTarFilterer(writer, func(hdr *tar.Header) (bool, bool, io.Reader) { + writer = newTarFilterer(writer, func(hdr *tar.Header) (tarFilterAction, bool, io.Reader) { hdr.Name = renameTarget renamedItems++ - return false, false, nil + return tarFilterKeep, false, nil }) } if options.Parents { parentsPrefixToRemove, parentsToSkip := getParentsPrefixToRemoveAndParentsToSkip(src, options.ContextDir) - writer = newTarFilterer(writer, func(hdr *tar.Header) (bool, bool, io.Reader) { + writer = newTarFilterer(writer, func(hdr *tar.Header) (tarFilterAction, bool, io.Reader) { if slices.Contains(parentsToSkip, hdr.Name) && hdr.Typeflag == tar.TypeDir { - return true, false, nil + return tarFilterSkip, false, nil } hdr.Name = strings.TrimPrefix(hdr.Name, parentsPrefixToRemove) hdr.Name = strings.TrimPrefix(hdr.Name, "/") @@ -761,14 +761,14 @@ func (b *Builder) Add(destination string, extract bool, options AddAndCopyOption hdr.Linkname = strings.TrimPrefix(hdr.Linkname, "/") } if hdr.Name == "" { - return true, false, nil + return tarFilterSkip, false, nil } - return false, false, nil + return tarFilterKeep, false, nil }) } - writer = newTarFilterer(writer, func(_ *tar.Header) (bool, bool, io.Reader) { + writer = newTarFilterer(writer, func(_ *tar.Header) (tarFilterAction, bool, io.Reader) { itemsCopied++ - return false, false, nil + return tarFilterKeep, false, nil }) getOptions := copier.GetOptions{ UIDMap: srcUIDMap, diff --git a/digester.go b/digester.go index 61c7605053..eb04c00445 100644 --- a/digester.go +++ b/digester.go @@ -6,6 +6,7 @@ import ( "fmt" "hash" "io" + "path" "strings" "sync" "time" @@ -13,6 +14,14 @@ import ( digest "github.com/opencontainers/go-digest" ) +type tarFilterAction int + +const ( + tarFilterKeep tarFilterAction = iota + tarFilterSkip + tarFilterDefer +) + type digester interface { io.WriteCloser ContentType() string @@ -95,7 +104,7 @@ func (t *tarFilterer) Close() error { // Note: if "filter" indicates that a given item should be skipped, there is no // guarantee that there will not be a subsequent item of type TypeLink, which // is a hard link, which points to the skipped item as the link target. -func newTarFilterer(writeCloser io.WriteCloser, filter func(hdr *tar.Header) (skip, replaceContents bool, replacementContents io.Reader)) io.WriteCloser { +func newTarFilterer(writeCloser io.WriteCloser, filter func(hdr *tar.Header) (action tarFilterAction, replaceContents bool, replacementContents io.Reader)) io.WriteCloser { pipeReader, pipeWriter := io.Pipe() tarWriter := tar.NewWriter(writeCloser) filterer := &tarFilterer{ @@ -105,44 +114,75 @@ func newTarFilterer(writeCloser io.WriteCloser, filter func(hdr *tar.Header) (sk filterer.closedLock.Lock() closed := filterer.closed filterer.closedLock.Unlock() + var deferred []*tar.Header + var tarReader *tar.Reader + writeEntry := func(hdr *tar.Header, replaceContents bool, replacementContents io.Reader) error { + if err := tarWriter.WriteHeader(hdr); err != nil { + return fmt.Errorf("writing tar header for %q: %w", hdr.Name, err) + } + if hdr.Size != 0 { + var n int64 + var copyErr error + if replaceContents { + n, copyErr = io.CopyN(tarWriter, replacementContents, hdr.Size) + } else { + n, copyErr = io.Copy(tarWriter, tarReader) + } + if copyErr != nil { + return fmt.Errorf("copying content for %q: %w", hdr.Name, copyErr) + } + if n != hdr.Size { + return fmt.Errorf("filtering content for %q: expected %d bytes, got %d bytes", hdr.Name, hdr.Size, n) + } + } + if err := tarWriter.Flush(); err != nil { + return fmt.Errorf("flushing tar item padding for %q: %w", hdr.Name, err) + } + return nil + } for !closed { - tarReader := tar.NewReader(pipeReader) + tarReader = tar.NewReader(pipeReader) hdr, err := tarReader.Next() for err == nil { - var skip, replaceContents bool + action := tarFilterKeep + var replaceContents bool var replacementContents io.Reader if filter != nil { - skip, replaceContents, replacementContents = filter(hdr) + action, replaceContents, replacementContents = filter(hdr) } - if !skip { - if err = tarWriter.WriteHeader(hdr); err != nil { - err = fmt.Errorf("writing tar header for %q: %w", hdr.Name, err) - break - } - if hdr.Size != 0 { - var n int64 - var copyErr error - if replaceContents { - n, copyErr = io.CopyN(tarWriter, replacementContents, hdr.Size) + switch action { + case tarFilterDefer: + hdrCopy := *hdr + deferred = append(deferred, &hdrCopy) + case tarFilterKeep: + // Emit deferred ancestors before the child + // because tar extractors create missing parent + // directories with default ownership otherwise. + nameSpec := path.Clean(strings.TrimRight(hdr.Name, "/")) + var remaining []*tar.Header + for _, d := range deferred { + deferredName := path.Clean(strings.TrimRight(d.Name, "/")) + if strings.HasPrefix(nameSpec, deferredName+"/") { + if err = writeEntry(d, false, nil); err != nil { + break + } } else { - n, copyErr = io.Copy(tarWriter, tarReader) - } - if copyErr != nil { - err = fmt.Errorf("copying content for %q: %w", hdr.Name, copyErr) - break - } - if n != hdr.Size { - err = fmt.Errorf("filtering content for %q: expected %d bytes, got %d bytes", hdr.Name, hdr.Size, n) - break + remaining = append(remaining, d) } } - if err = tarWriter.Flush(); err != nil { - err = fmt.Errorf("flushing tar item padding for %q: %w", hdr.Name, err) - break + deferred = remaining + + // Emit the child. + if err == nil { + err = writeEntry(hdr, replaceContents, replacementContents) } } + if err != nil { + break + } hdr, err = tarReader.Next() } + deferred = nil if !errors.Is(err, io.EOF) { filterer.err = fmt.Errorf("reading tar archive: %w", err) break @@ -174,12 +214,12 @@ type tarDigester struct { tarFilterer io.WriteCloser } -func modifyTarHeaderForDigesting(hdr *tar.Header) (skip, replaceContents bool, replacementContents io.Reader) { +func modifyTarHeaderForDigesting(hdr *tar.Header) (action tarFilterAction, replaceContents bool, replacementContents io.Reader) { zeroTime := time.Time{} hdr.ModTime = zeroTime hdr.AccessTime = zeroTime hdr.ChangeTime = zeroTime - return false, false, nil + return tarFilterKeep, false, nil } func newTarDigester(contentType string) digester { diff --git a/digester_test.go b/digester_test.go index 4441cf3907..1e339371b0 100644 --- a/digester_test.go +++ b/digester_test.go @@ -123,9 +123,9 @@ func TestCompositeDigester(t *testing.T) { } if filtered { // wrap the WriteCloser in another WriteCloser - hasher = newTarFilterer(hasher, func(hdr *tar.Header) (bool, bool, io.Reader) { + hasher = newTarFilterer(hasher, func(hdr *tar.Header) (tarFilterAction, bool, io.Reader) { hdr.ModTime = zero - return false, false, nil + return tarFilterKeep, false, nil }) require.NotNil(t, hasher, "newTarFilterer returned a null WriteCloser?") } @@ -192,7 +192,7 @@ func TestTarFilterer(t *testing.T) { name string input, output map[string]string breakAfter int - filter func(*tar.Header) (bool, bool, io.Reader) + filter func(*tar.Header) (tarFilterAction, bool, io.Reader) }{ { name: "none", @@ -216,7 +216,7 @@ func TestTarFilterer(t *testing.T) { "file a": "content a", "file b": "content b", }, - filter: func(*tar.Header) (bool, bool, io.Reader) { return false, false, nil }, + filter: func(*tar.Header) (tarFilterAction, bool, io.Reader) { return tarFilterKeep, false, nil }, }, { name: "skip", @@ -227,7 +227,15 @@ func TestTarFilterer(t *testing.T) { output: map[string]string{ "file a": "content a", }, - filter: func(hdr *tar.Header) (bool, bool, io.Reader) { return hdr.Name == "file b", false, nil }, + filter: func(hdr *tar.Header) (tarFilterAction, bool, io.Reader) { + var action tarFilterAction + if hdr.Name == "file b" { + action = tarFilterSkip + } else { + action = tarFilterKeep + } + return action, false, nil + }, }, { name: "replace", @@ -242,13 +250,13 @@ func TestTarFilterer(t *testing.T) { "file c": "content c", }, breakAfter: 2, - filter: func(hdr *tar.Header) (bool, bool, io.Reader) { + filter: func(hdr *tar.Header) (tarFilterAction, bool, io.Reader) { if hdr.Name == "file b" { content := "content b+c" hdr.Size = int64(len(content)) - return false, true, strings.NewReader(content) + return tarFilterKeep, true, strings.NewReader(content) } - return false, false, nil + return tarFilterKeep, false, nil }, }, } diff --git a/image.go b/image.go index df31f918ff..e123c55704 100644 --- a/image.go +++ b/image.go @@ -879,6 +879,8 @@ func (i containerImageRef) filterExclusionsByImage(ctx context.Context, exclusio if exclusion.Owner != nil && (int64(exclusion.Owner.UID) != stat.UID && int64(exclusion.Owner.GID) != stat.GID) { continue } + exclusion.Mode = &stat.Mode + exclusion.Owner = &idtools.IDPair{UID: int(stat.UID), GID: int(stat.GID)} paths = append(paths, exclusion) } } @@ -1051,6 +1053,7 @@ func (i *containerImageRef) NewImageSource(ctx context.Context, _ *types.SystemC var rc io.ReadCloser var errChan chan error var layerExclusions []copier.ConditionalRemovePath + var layerPullUps []copier.EnsureParentPath if i.confidentialWorkload.Convert { // Convert the root filesystem into an encrypted disk image. rc, err = i.extractConfidentialWorkloadFS(i.confidentialWorkload) @@ -1086,14 +1089,13 @@ func (i *containerImageRef) NewImageSource(ctx context.Context, _ *types.SystemC if layerID == i.layerID { // We need to filter out any mount targets that we created. layerExclusions = append(slices.Clone(i.layerExclusions), i.layerMountTargets...) - // And we _might_ need to filter out directories that modified - // by creating and removing mount targets, _if_ they were the - // same in the base image for this stage. - layerPullUps, err := i.filterExclusionsByImage(ctx, i.layerPullUps, i.fromImageID) + // Parent directories that were modified by creating and + // removing mount targets should have their ownership + // and mode corrected rather than being excluded. + layerPullUps, err = i.filterExclusionsByImage(ctx, i.layerPullUps, i.fromImageID) if err != nil { return nil, fmt.Errorf("checking which exclusions are in base image %q: %w", i.fromImageID, err) } - layerExclusions = append(layerExclusions, layerPullUps...) } // Extract this layer, one of possibly many. rc, err = i.store.Diff("", layerID, diffOptions) @@ -1137,7 +1139,7 @@ func (i *containerImageRef) NewImageSource(ctx context.Context, _ *types.SystemC // Use specified timestamps in the layer, if we're doing that for history // entries. nestedWriteCloser := ioutils.NewWriteCloserWrapper(writer, writeCloser.Close) - writeCloser, err = makeFilteredLayerWriteCloser(nestedWriteCloser, i.layerModTime, i.layerLatestModTime, layerExclusions, i.os == "windows") + writeCloser, err = makeFilteredLayerWriteCloser(nestedWriteCloser, i.layerModTime, i.layerLatestModTime, layerExclusions, layerPullUps, i.os == "windows") if err != nil { return nil, fmt.Errorf("creating filter write closer %s: %w", what, err) } @@ -1422,8 +1424,8 @@ func (i *containerImageRef) makeExtraImageContentDiff(includeFooter bool, timest // no later than layerLatestModTime (if a value is provided for it). // This implies that if both values are provided, the archive's timestamps will // be set to the earlier of the two values. -func makeFilteredLayerWriteCloser(wc io.WriteCloser, layerModTime, layerLatestModTime *time.Time, exclusions []copier.ConditionalRemovePath, windows bool) (io.WriteCloser, error) { - if layerModTime == nil && layerLatestModTime == nil && len(exclusions) == 0 && !windows { +func makeFilteredLayerWriteCloser(wc io.WriteCloser, layerModTime, layerLatestModTime *time.Time, exclusions []copier.ConditionalRemovePath, pullUps []copier.EnsureParentPath, windows bool) (io.WriteCloser, error) { + if layerModTime == nil && layerLatestModTime == nil && len(exclusions) == 0 && len(pullUps) == 0 && !windows { return wc, nil } exclusionsMap := make(map[string]copier.ConditionalRemovePath) @@ -1434,10 +1436,18 @@ func makeFilteredLayerWriteCloser(wc io.WriteCloser, layerModTime, layerLatestMo } exclusionsMap[pathSpec] = exclusionSpec } + pullUpsMap := make(map[string]copier.EnsureParentPath) + for _, pullUpSpec := range pullUps { + pathSpec := strings.Trim(path.Clean(pullUpSpec.Path), "/") + if pathSpec == "" { + continue + } + pullUpsMap[pathSpec] = pullUpSpec + } var initialized bool - wc = newTarFilterer(wc, func(hdr *tar.Header) (skip, replaceContents bool, replacementContents io.Reader) { + wc = newTarFilterer(wc, func(hdr *tar.Header) (action tarFilterAction, replaceContents bool, replacementContents io.Reader) { modTime := hdr.ModTime - if layerModTime != nil || layerLatestModTime != nil || len(exclusions) != 0 { + if layerModTime != nil || layerLatestModTime != nil || len(exclusions) != 0 || len(pullUps) != 0 { // Changing a zeroed field to a non-zero field can affect the // format that the library uses for writing the header, so only // change fields that are already set to avoid changing the @@ -1448,8 +1458,21 @@ func makeFilteredLayerWriteCloser(wc io.WriteCloser, layerModTime, layerLatestMo if (conditions.ModTime == nil || conditions.ModTime.Equal(modTime)) && (conditions.Owner == nil || (conditions.Owner.UID == hdr.Uid && conditions.Owner.GID == hdr.Gid)) && (conditions.Mode == nil || (*conditions.Mode&os.ModePerm == os.FileMode(hdr.Mode)&os.ModePerm)) { - return true, false, nil + return tarFilterSkip, false, nil + } + } + // Correct the ownership and mode of pulled-up parent + // directories, but defer writing them until a child + // entry passes through the filter. + if pullUpSpec, ok := pullUpsMap[nameSpec]; ok { + if pullUpSpec.Owner != nil { + hdr.Uid = pullUpSpec.Owner.UID + hdr.Gid = pullUpSpec.Owner.GID + } + if pullUpSpec.Mode != nil { + hdr.Mode = int64(*pullUpSpec.Mode & os.ModePerm) } + return tarFilterDefer, false, nil } } if layerModTime != nil { @@ -1499,7 +1522,7 @@ func makeFilteredLayerWriteCloser(wc io.WriteCloser, layerModTime, layerLatestMo hdr.PAXRecords[keyCreationTime] = fmt.Sprintf("%d.%09d", hdr.ModTime.Unix(), hdr.ModTime.Nanosecond()) } } - return false, false, nil + return tarFilterKeep, false, nil }) if windows { // prep the archive by writing the Files/ and Hives/ directories to the writer. @@ -1588,7 +1611,7 @@ func (b *Builder) makeLinkedLayerInfos(layers []LinkedLayer, layerType string, l digester := digest.Canonical.Digester() sizeCountedFile := ioutils.NewWriteCounter(io.MultiWriter(digester.Hash(), f)) - wc, err := makeFilteredLayerWriteCloser(ioutils.NopWriteCloser(sizeCountedFile), layerModTime, layerLatestModTime, nil, false) + wc, err := makeFilteredLayerWriteCloser(ioutils.NopWriteCloser(sizeCountedFile), layerModTime, layerLatestModTime, nil, nil, false) if err != nil { return err } diff --git a/tests/bud.bats b/tests/bud.bats index 5b39ef7e7e..c677404b5a 100644 --- a/tests/bud.bats +++ b/tests/bud.bats @@ -9388,6 +9388,30 @@ _EOF diff -u ${TEST_SCRATCH_DIR}/squashed-layered-image-rootfs.txt ${TEST_SCRATCH_DIR}/squashed-not-layered-image-rootfs.txt } +# https://github.com/containers/buildah/issues/6747 +@test "bud --layers should preserve parent directory ownership with RUN --mount" { + case "${STORAGE_DRIVER}" in + overlay) ;; + *) skip "bug only reproduces with overlay, not ${STORAGE_DRIVER}" ;; + esac + + _prefetch ubuntu + local contextdir=${TEST_SCRATCH_DIR}/context + mkdir -p ${contextdir} + + cat > ${contextdir}/Containerfile << '_EOF' +FROM ubuntu +RUN mkdir -p /home/testuser && chown 1234:5678 /home/testuser +USER 1234 +WORKDIR /home/testuser +RUN mkdir -p somedir +RUN --mount=type=cache,target=.cache touch somedir +RUN test "$(stat -c '%u:%g' .)" = "1234:5678" +_EOF + + run_buildah build --no-cache --layers -t test-mount-ownership ${contextdir} +} + @test "bud COPY one file to ..../. creates the destination directory" { _prefetch busybox local contextdir=${TEST_SCRATCH_DIR}/context