Merge pull request #1953 from akihikodaki/overlay

overlay: Fix root directoy state with extended attributes

drivers/graphtest/graphtest_unix.go

@@ -44,13 +44,7 @@ type Driver struct {
 	refCount int
 }
 
-func newDriver(t testing.TB, name string, options []string) *Driver {
-	root, err := os.MkdirTemp("", "storage-graphtest-")
-	require.NoError(t, err)
-	runroot, err := os.MkdirTemp("", "storage-graphtest-")
-	require.NoError(t, err)
-
-	require.NoError(t, os.MkdirAll(root, 0o755))
+func newGraphDriver(t testing.TB, name string, options []string, root string, runroot string) graphdriver.Driver {
 	d, err := graphdriver.GetDriver(name, graphdriver.Options{DriverOptions: options, Root: root, RunRoot: runroot})
 	if err != nil {
 		t.Logf("graphdriver: %v\n", err)
@@ -63,7 +57,17 @@ func newDriver(t testing.TB, name string, options []string) *Driver {
 		}
 		t.Fatal(err)
 	}
-	return &Driver{d, root, runroot, 1}
+	return d
+}
+
+func newDriver(t testing.TB, name string, options []string) *Driver {
+	root, err := os.MkdirTemp("", "storage-graphtest-")
+	require.NoError(t, err)
+	runroot, err := os.MkdirTemp("", "storage-graphtest-")
+	require.NoError(t, err)
+
+	require.NoError(t, os.MkdirAll(root, 0o755))
+	return &Driver{newGraphDriver(t, name, options, root, runroot), root, runroot, 1}
 }
 
 func cleanup(t testing.TB, d *Driver) {
@@ -84,6 +88,13 @@ func GetDriver(t testing.TB, name string, options ...string) graphdriver.Driver
 	return drv
 }
 
+func ReconfigureDriver(t testing.TB, name string, options ...string) {
+	if err := drv.Cleanup(); err != nil {
+		t.Fatal(err)
+	}
+	drv.Driver = newGraphDriver(t, name, options, drv.root, drv.runroot)
+}
+
 // PutDriver removes the driver if it is no longer used and updates the reference count.
 func PutDriver(t testing.TB) {
 	if drv == nil {

drivers/overlay/overlay.go

@@ -1049,14 +1049,23 @@ func (d *Driver) create(id, parent string, opts *graphdriver.CreateOpts, readOnl
 	if err := idtools.MkdirAllAndChownNew(path.Dir(dir), 0o755, idPair); err != nil {
 		return err
 	}
+
+	st := idtools.Stat{IDs: idPair, Mode: defaultPerms}
+
 	if parent != "" {
 		parentBase := d.dir(parent)
-		st, err := system.Stat(filepath.Join(parentBase, "diff"))
-		if err != nil {
-			return err
+		parentDiff := filepath.Join(parentBase, "diff")
+		if xSt, err := idtools.GetContainersOverrideXattr(parentDiff); err == nil {
+			st = xSt
+		} else {
+			systemSt, err := system.Stat(parentDiff)
+			if err != nil {
+				return err
+			}
+			st.IDs.UID = int(systemSt.UID())
+			st.IDs.GID = int(systemSt.GID())
+			st.Mode = os.FileMode(systemSt.Mode())
 		}
-		rootUID = int(st.UID())
-		rootGID = int(st.GID())
 	}
 
 	if err := fileutils.Lexists(dir); err == nil {
@@ -1102,22 +1111,21 @@ func (d *Driver) create(id, parent string, opts *graphdriver.CreateOpts, readOnl
 		}
 	}
 
-	perms := defaultPerms
+	forcedSt := st
 	if d.options.forceMask != nil {
-		perms = *d.options.forceMask
+		forcedSt.IDs = idPair
+		forcedSt.Mode = *d.options.forceMask
 	}
 
-	if parent != "" {
-		parentBase := d.dir(parent)
-		st, err := system.Stat(filepath.Join(parentBase, "diff"))
-		if err != nil {
-			return err
-		}
-		perms = os.FileMode(st.Mode())
+	diff := path.Join(dir, "diff")
+	if err := idtools.MkdirAs(diff, forcedSt.Mode, forcedSt.IDs.UID, forcedSt.IDs.GID); err != nil {
+		return err
 	}
 
-	if err := idtools.MkdirAs(path.Join(dir, "diff"), perms, rootUID, rootGID); err != nil {
-		return err
+	if d.options.forceMask != nil {
+		if err := idtools.SetContainersOverrideXattr(diff, st); err != nil {
+			return err
+		}
 	}
 
 	lid := generateID(idLength)
@@ -1132,16 +1140,16 @@ func (d *Driver) create(id, parent string, opts *graphdriver.CreateOpts, readOnl
 		return err
 	}
 
-	if err := idtools.MkdirAs(path.Join(dir, "work"), 0o700, rootUID, rootGID); err != nil {
+	if err := idtools.MkdirAs(path.Join(dir, "work"), 0o700, forcedSt.IDs.UID, forcedSt.IDs.GID); err != nil {
 		return err
 	}
-	if err := idtools.MkdirAs(path.Join(dir, "merged"), 0o700, rootUID, rootGID); err != nil {
+	if err := idtools.MkdirAs(path.Join(dir, "merged"), 0o700, forcedSt.IDs.UID, forcedSt.IDs.GID); err != nil {
 		return err
 	}
 
 	// if no parent directory, create a dummy lower directory and skip writing a "lowers" file
 	if parent == "" {
-		return idtools.MkdirAs(path.Join(dir, "empty"), 0o700, rootUID, rootGID)
+		return idtools.MkdirAs(path.Join(dir, "empty"), 0o700, forcedSt.IDs.UID, forcedSt.IDs.GID)
 	}
 
 	lower, err := d.getLower(parent)

drivers/overlay/overlay_test.go

@@ -4,12 +4,16 @@
 package overlay
 
 import (
+	"os"
+	"os/exec"
 	"testing"
 
 	graphdriver "github.com/containers/storage/drivers"
 	"github.com/containers/storage/drivers/graphtest"
 	"github.com/containers/storage/pkg/archive"
 	"github.com/containers/storage/pkg/reexec"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
 const driverName = "overlay"
@@ -31,6 +35,31 @@ func skipIfNaive(t *testing.T) {
 	}
 }
 
+// Ensure that a layer created with force_mask will keep the root directory mode
+// with user.containers.override_stat. This preserved mode should also be
+// inherited by the upper layer, whether force_mask is set or not.
+//
+// This test is placed before TestOverlaySetup() because it uses driver options
+// different from the other tests.
+func TestContainersOverlayXattr(t *testing.T) {
+	path, err := exec.LookPath("fuse-overlayfs")
+	if err != nil {
+		t.Skipf("fuse-overlayfs unavailable")
+	}
+
+	driver := graphtest.GetDriver(t, driverName, "force_mask=700", "mount_program="+path)
+	defer graphtest.PutDriver(t)
+	require.NoError(t, driver.Create("lower", "", nil))
+	graphtest.ReconfigureDriver(t, driverName, "mount_program="+path)
+	require.NoError(t, driver.Create("upper", "lower", nil))
+
+	root, err := driver.Get("upper", graphdriver.MountOpts{})
+	require.NoError(t, err)
+	fi, err := os.Stat(root)
+	require.NoError(t, err)
+	assert.Equal(t, 0o555&os.ModePerm, fi.Mode()&os.ModePerm, root)
+}
+
 // This avoids creating a new driver for each test if all tests are run
 // Make sure to put new tests between TestOverlaySetup and TestOverlayTeardown
 func TestOverlaySetup(t *testing.T) {

pkg/archive/archive.go

@@ -701,8 +701,11 @@ func createTarFile(path, extractDir string, hdr *tar.Header, reader io.Reader, L
 	}
 
 	if forceMask != nil && (hdr.Typeflag != tar.TypeSymlink || runtime.GOOS == "darwin") {
-		value := fmt.Sprintf("%d:%d:0%o", hdr.Uid, hdr.Gid, hdrInfo.Mode()&0o7777)
-		if err := system.Lsetxattr(path, idtools.ContainersOverrideXattr, []byte(value), 0); err != nil {
+		value := idtools.Stat{
+			IDs:  idtools.IDPair{UID: hdr.Uid, GID: hdr.Gid},
+			Mode: hdrInfo.Mode() & 0o7777,
+		}
+		if err := idtools.SetContainersOverrideXattr(path, value); err != nil {
 			return err
 		}
 	}
@@ -1114,11 +1117,13 @@ loop:
 	}
 
 	if options.ForceMask != nil {
-		value := "0:0:0755"
+		value := idtools.Stat{Mode: 0o755}
 		if rootHdr != nil {
-			value = fmt.Sprintf("%d:%d:0%o", rootHdr.Uid, rootHdr.Gid, rootHdr.Mode)
+			value.IDs.UID = rootHdr.Uid
+			value.IDs.GID = rootHdr.Gid
+			value.Mode = os.FileMode(rootHdr.Mode)
 		}
-		if err := system.Lsetxattr(dest, idtools.ContainersOverrideXattr, []byte(value), 0); err != nil {
+		if err := idtools.SetContainersOverrideXattr(dest, value); err != nil {
 			return err
 		}
 	}

pkg/chunked/storage_linux.go

@@ -1263,9 +1263,12 @@ func (c *chunkedDiffer) ApplyDiff(dest string, options *archive.TarOptions, diff
 	if options.ForceMask != nil {
 		uid, gid, mode, err := archive.GetFileOwner(dest)
 		if err == nil {
-			value := fmt.Sprintf("%d:%d:0%o", uid, gid, mode)
-			if err := unix.Setxattr(dest, containersOverrideXattr, []byte(value), 0); err != nil {
-				return output, &fs.PathError{Op: "setxattr", Path: dest, Err: err}
+			value := idtools.Stat{
+				IDs:  idtools.IDPair{UID: int(uid), GID: int(gid)},
+				Mode: os.FileMode(mode),
+			}
+			if err := idtools.SetContainersOverrideXattr(dest, value); err != nil {
+				return output, err
 			}
 		}
 	}

pkg/idtools/idtools.go

@@ -367,21 +367,71 @@ func checkChownErr(err error, name string, uid, gid int) error {
 	return err
 }
 
+// Stat contains file states that can be overriden with ContainersOverrideXattr.
+type Stat struct {
+	IDs  IDPair
+	Mode os.FileMode
+}
+
+// GetContainersOverrideXattr will get and decode ContainersOverrideXattr.
+func GetContainersOverrideXattr(path string) (Stat, error) {
+	var stat Stat
+	xstat, err := system.Lgetxattr(path, ContainersOverrideXattr)
+	if err != nil {
+		return stat, err
+	}
+
+	attrs := strings.Split(string(xstat), ":")
+	if len(attrs) != 3 {
+		return stat, fmt.Errorf("The number of clons in %s does not equal to 3",
+			ContainersOverrideXattr)
+	}
+
+	value, err := strconv.ParseUint(attrs[0], 10, 32)
+	if err != nil {
+		return stat, fmt.Errorf("Failed to parse UID: %w", err)
+	}
+
+	stat.IDs.UID = int(value)
+
+	value, err = strconv.ParseUint(attrs[0], 10, 32)
+	if err != nil {
+		return stat, fmt.Errorf("Failed to parse GID: %w", err)
+	}
+
+	stat.IDs.GID = int(value)
+
+	value, err = strconv.ParseUint(attrs[2], 8, 32)
+	if err != nil {
+		return stat, fmt.Errorf("Failed to parse mode: %w", err)
+	}
+
+	stat.Mode = os.FileMode(value)
+
+	return stat, nil
+}
+
+// SetContainersOverrideXattr will encode and set ContainersOverrideXattr.
+func SetContainersOverrideXattr(path string, stat Stat) error {
+	value := fmt.Sprintf("%d:%d:0%o", stat.IDs.UID, stat.IDs.GID, stat.Mode)
+	return system.Lsetxattr(path, ContainersOverrideXattr, []byte(value), 0)
+}
+
 func SafeChown(name string, uid, gid int) error {
 	if runtime.GOOS == "darwin" {
-		var mode uint64 = 0o0700
+		var mode os.FileMode = 0o0700
 		xstat, err := system.Lgetxattr(name, ContainersOverrideXattr)
 		if err == nil {
 			attrs := strings.Split(string(xstat), ":")
 			if len(attrs) == 3 {
 				val, err := strconv.ParseUint(attrs[2], 8, 32)
 				if err == nil {
-					mode = val
+					mode = os.FileMode(val)
 				}
 			}
 		}
-		value := fmt.Sprintf("%d:%d:0%o", uid, gid, mode)
-		if err = system.Lsetxattr(name, ContainersOverrideXattr, []byte(value), 0); err != nil {
+		value := Stat{IDPair{uid, gid}, mode}
+		if err = SetContainersOverrideXattr(name, value); err != nil {
 			return err
 		}
 		uid = os.Getuid()
@@ -397,19 +447,19 @@ func SafeChown(name string, uid, gid int) error {
 
 func SafeLchown(name string, uid, gid int) error {
 	if runtime.GOOS == "darwin" {
-		var mode uint64 = 0o0700
+		var mode os.FileMode = 0o0700
 		xstat, err := system.Lgetxattr(name, ContainersOverrideXattr)
 		if err == nil {
 			attrs := strings.Split(string(xstat), ":")
 			if len(attrs) == 3 {
 				val, err := strconv.ParseUint(attrs[2], 8, 32)
 				if err == nil {
-					mode = val
+					mode = os.FileMode(val)
 				}
 			}
 		}
-		value := fmt.Sprintf("%d:%d:0%o", uid, gid, mode)
-		if err = system.Lsetxattr(name, ContainersOverrideXattr, []byte(value), 0); err != nil {
+		value := Stat{IDPair{uid, gid}, mode}
+		if err = SetContainersOverrideXattr(name, value); err != nil {
 			return err
 		}
 		uid = os.Getuid()