diffApply: set dir opaque when overwriting whiteout

Before this, there was a bug triggered under the following conditions:
1. An overlay snapshotter was being used, which caused the optimization
   of preparing a new snapshot off of the base layers to be triggered
2. The base layers contained a directory that had contents
3. One subsequent layer deleted that directory w/out recreating it
4. A later layer recreated the directory

In this case, what happened was a whiteout device would be created as
part of 3 above but then in step 4 the whiteout device would be removed
and replaced with a plain directory. The problem is that such a
directory doesn't block out the files from step 2 and it doesn't know
about them because they are in a lowerdir (not the upperdir being
applied to).

The simplest fix, which this commit implements, is to just set the
directory created in step 4 as opaque, which enables the correct
behavior of blocking out files below it.

This was missed in test coverage before because tests for opaque
handling always combined 3+4 into one layer, whereas the bug requires
they be separate layers. A new integration test has been added to cover
this case.

Signed-off-by: Erik Sipsma <erik@sipsma.dev>

client/mergediff_test.go

@@ -1070,6 +1070,31 @@ func diffOpTestCases() (tests []integration.Test) {
 		}
 	}()...)
 
+	// Regression tests
+	tests = append(tests, func() []integration.Test {
+		base := func() llb.State {
+			return llb.Scratch().File(llb.Mkdir("/dir", 0755))
+		}
+		return []integration.Test{
+			verifyContents{
+				// Verifies that when a directory with contents is used a a base layer
+				// in a merge, subsequent merges that first delete the dir (resulting in
+				// a whiteout device w/ overlay snapshotters) and then recreate the dir
+				// correctly set it as opaque.
+				name: "TestDiffMergeOpaqueRegression",
+				state: llb.Merge([]llb.State{
+					base().File(llb.Mkfile("/dir/a", 0644, nil)),
+					base().File(llb.Rm("/dir")),
+					base().File(llb.Mkfile("/dir/b", 0644, nil)),
+				}),
+				contents: apply(
+					fstest.CreateDir("/dir", 0755),
+					fstest.CreateFile("/dir/b", nil, 0644),
+				),
+			},
+		}
+	}()...)
+
 	return tests
 }
 

snapshot/diffapply_unix.go

@@ -30,7 +30,7 @@ import (
 // that accounts for any hardlinks made from existing snapshots. ctx is expected to have a temporary lease
 // associated with it.
 func (sn *mergeSnapshotter) diffApply(ctx context.Context, dest Mountable, diffs ...Diff) (_ snapshots.Usage, rerr error) {
-	a, err := applierFor(dest, sn.tryCrossSnapshotLink)
+	a, err := applierFor(dest, sn.tryCrossSnapshotLink, sn.userxattr)
 	if err != nil {
 		return snapshots.Usage{}, errors.Wrapf(err, "failed to create applier")
 	}
@@ -112,8 +112,9 @@ type change struct {
 
 type changeApply struct {
 	*change
-	dstPath string
+	dstPath   string
-	dstStat *syscall.Stat_t
+	dstStat   *syscall.Stat_t
+	setOpaque bool
 }
 
 type inode struct {
@@ -137,12 +138,14 @@ type applier struct {
 	lowerdirs            []string // ordered highest -> lowest, the order we want to check them in
 	crossSnapshotLinks   map[inode]struct{}
 	createWhiteoutDelete bool
+	userxattr            bool
 	dirModTimes          map[string]unix.Timespec // map of dstPath -> mtime that should be set on that subPath
 }
 
-func applierFor(dest Mountable, tryCrossSnapshotLink bool) (_ *applier, rerr error) {
+func applierFor(dest Mountable, tryCrossSnapshotLink, userxattr bool) (_ *applier, rerr error) {
 	a := &applier{
 		dirModTimes: make(map[string]unix.Timespec),
+		userxattr:   userxattr,
 	}
 	defer func() {
 		if rerr != nil {
@@ -263,6 +266,12 @@ func (a *applier) applyDelete(ctx context.Context, ca *changeApply) (bool, error
 		return false, nil
 	}
 
+	if overwrite && a.createWhiteoutDelete && isWhiteoutDevice(ca.dstStat) && ca.srcStat.Mode&unix.S_IFMT == unix.S_IFDIR {
+		// If we are overwriting a whiteout device with a directory, we need this new dir to be opaque
+		// so that any files from lowerdirs under it are not visible.
+		ca.setOpaque = true
+	}
+
 	if err := os.RemoveAll(ca.dstPath); err != nil {
 		return false, errors.Wrap(err, "failed to remove during apply")
 	}
@@ -376,9 +385,9 @@ func (a *applier) applyCopy(ctx context.Context, ca *changeApply) error {
 		}
 		for _, xattr := range xattrs {
 			if isOpaqueXattr(xattr) {
-				// Don't recreate opaque xattrs during merge. These should only be set when using overlay snapshotters,
+				// Don't recreate opaque xattrs during merge based on the source file. The differs take care of converting
-				// in which case we are converting from the "opaque whiteout" format to the "explicit whiteout" format during
+				// source path from the "opaque whiteout" format to the "explicit whiteout" format. The only time we set
-				// the merge (as taken care of by the overlay differ).
+				// opaque xattrs is handled after this loop below.
 				continue
 			}
 			xattrVal, err := sysx.LGetxattr(ca.srcPath, xattr)
@@ -392,6 +401,14 @@ func (a *applier) applyCopy(ctx context.Context, ca *changeApply) error {
 		}
 	}
 
+	if ca.setOpaque {
+		// This is set in the case where we are creating a directory that is replacing a whiteout device
+		xattr := opaqueXattr(a.userxattr)
+		if err := sysx.LSetxattr(ca.dstPath, xattr, []byte{'y'}, 0); err != nil {
+			return errors.Wrapf(err, "failed to set opaque xattr %q of path %s", xattr, ca.dstPath)
+		}
+	}
+
 	if err := os.Lchown(ca.dstPath, int(ca.srcStat.Uid), int(ca.srcStat.Gid)); err != nil {
 		return errors.Wrap(err, "failed to chown during apply")
 	}
@@ -773,8 +790,13 @@ func safeJoin(root, path string) (string, error) {
 	return filepath.Join(parent, base), nil
 }
 
+const (
+	trustedOpaqueXattr = "trusted.overlay.opaque"
+	userOpaqueXattr    = "user.overlay.opaque"
+)
+
 func isOpaqueXattr(s string) bool {
-	for _, k := range []string{"trusted.overlay.opaque", "user.overlay.opaque"} {
+	for _, k := range []string{trustedOpaqueXattr, userOpaqueXattr} {
 		if s == k {
 			return true
 		}
@@ -782,6 +804,13 @@ func isOpaqueXattr(s string) bool {
 	return false
 }
 
+func opaqueXattr(userxattr bool) string {
+	if userxattr {
+		return userOpaqueXattr
+	}
+	return trustedOpaqueXattr
+}
+
 // needsUserXAttr checks whether overlay mounts should be provided the userxattr option. We can't use
 // NeedsUserXAttr from the overlayutils package directly because we don't always have direct knowledge
 // of the root of the snapshotter state (such as when using a remote snapshotter). Instead, we create
@@ -820,3 +849,8 @@ func needsUserXAttr(ctx context.Context, sn Snapshotter, lm leases.Manager) (boo
 	}
 	return userxattr, nil
 }
+
+func isWhiteoutDevice(st *syscall.Stat_t) bool {
+	// it's a whiteout if it's a char device and has a major/minor of 0/0
+	return st != nil && st.Mode&unix.S_IFMT == unix.S_IFCHR && st.Rdev == unix.Mkdev(0, 0)
+}

snapshot/merge.go

@@ -58,9 +58,12 @@ type mergeSnapshotter struct {
 	// Whether we should try to implement merges by hardlinking between underlying directories
 	tryCrossSnapshotLink bool
 
-	// Whether the snapshotter is overlay-based, which enables some some optimizations like
+	// Whether the optimization of preparing on top of base layers is supported (see Merge method).
-	// using the first merge input as the parent snapshot.
+	skipBaseLayers bool
-	overlayBased bool
+
+	// Whether we should use the "user.*" namespace when writing overlay xattrs. If false,
+	// "trusted.*" is used instead.
+	userxattr bool
 }
 
 func NewMergeSnapshotter(ctx context.Context, sn Snapshotter, lm leases.Manager) MergeSnapshotter {
@@ -68,16 +71,24 @@ func NewMergeSnapshotter(ctx context.Context, sn Snapshotter, lm leases.Manager)
 	_, tryCrossSnapshotLink := hardlinkMergeSnapshotters[name]
 	_, overlayBased := overlayBasedSnapshotters[name]
 
+	skipBaseLayers := overlayBased // default to skipping base layer for overlay-based snapshotters
+	var userxattr bool
 	if overlayBased && userns.RunningInUserNS() {
 		// When using an overlay-based snapshotter, if we are running rootless on a pre-5.11
 		// kernel, we will not have userxattr. This results in opaque xattrs not being visible
 		// to us and thus breaking the overlay-optimized differ.
-		userxattr, err := needsUserXAttr(ctx, sn, lm)
+		var err error
+		userxattr, err = needsUserXAttr(ctx, sn, lm)
 		if err != nil {
 			bklog.G(ctx).Debugf("failed to check user xattr: %v", err)
 			tryCrossSnapshotLink = false
+			skipBaseLayers = false
 		} else {
 			tryCrossSnapshotLink = userxattr
+			// Disable skipping base layers when in pre-5.11 rootless mode. Skipping the base layers
+			// necessitates the ability to set opaque xattrs sometimes, which only works in 5.11+
+			// kernels that support userxattr.
+			skipBaseLayers = userxattr
 		}
 	}
 
@@ -85,13 +96,14 @@ func NewMergeSnapshotter(ctx context.Context, sn Snapshotter, lm leases.Manager)
 		Snapshotter:          sn,
 		lm:                   lm,
 		tryCrossSnapshotLink: tryCrossSnapshotLink,
-		overlayBased:         overlayBased,
+		skipBaseLayers:       skipBaseLayers,
+		userxattr:            userxattr,
 	}
 }
 
 func (sn *mergeSnapshotter) Merge(ctx context.Context, key string, diffs []Diff, opts ...snapshots.Opt) error {
 	var baseKey string
-	if sn.overlayBased {
+	if sn.skipBaseLayers {
 		// Overlay-based snapshotters can skip the base snapshot of the merge (if one exists) and just use it as the
 		// parent of the merge snapshot. Other snapshotters will start empty (with baseKey set to "").
 		// Find the baseKey by following the chain of diffs for as long as it follows the pattern of the current lower