Merge pull request apptainer#5674 from dctrud/issue5668

unsquashfs - don't extract xattrs where not supported by fs

e2e/imgbuild/imgbuild.go

@@ -1089,5 +1089,6 @@ func E2ETests(env e2e.TestEnv) testhelper.Tests {
 		"issue 5250":                      c.issue5250,                 // https://github.com/sylabs/singularity/issues/5250
 		"issue 5315":                      c.issue5315,                 // https://github.com/sylabs/singularity/issues/5315
 		"issue 5435":                      c.issue5435,                 // https://github.com/hpcng/singularity/issues/5435
+		"issue 5668":                      c.issue5668,                 // https://github.com/hpcng/singularity/issues/5435
 	}
 }

e2e/imgbuild/regressions.go

@@ -450,3 +450,23 @@ func (c *imgBuildTests) issue5250(t *testing.T) {
 		),
 	)
 }
+
+// Check that unsquashfs (SIF -> sandbox) works on a tmpfs, that will not support
+// user xattrs. Our home dir in the e2e test is a tmpfs bound over our real home dir
+// so we can use that.
+func (c *imgBuildTests) issue5668(t *testing.T) {
+	e2e.EnsureImage(t, c.env)
+	home, err := os.UserHomeDir()
+	if err != nil {
+		t.Fatalf("Could not get home dir: %v", err)
+	}
+	sbDir, sbCleanup := e2e.MakeTempDir(t, home, "issue-5668-", "")
+	defer e2e.Privileged(sbCleanup)(t)
+	c.env.RunSingularity(
+		t,
+		e2e.WithProfile(e2e.UserProfile),
+		e2e.WithCommand("build"),
+		e2e.WithArgs("--force", "--sandbox", sbDir, c.env.ImagePath),
+		e2e.ExpectExit(0),
+	)
+}

pkg/image/unpacker/squashfs.go

@@ -16,21 +16,20 @@ import (
 	"path/filepath"
 
 	"github.com/sylabs/singularity/pkg/sylog"
+	"golang.org/x/sys/unix"
 )
 
 const (
 	stdinFile = "/proc/self/fd/0"
 )
 
-var cmdFunc func(unsquashfs string, dest string, filename string, rootless bool) (*exec.Cmd, error)
+var cmdFunc func(unsquashfs string, dest string, filename string, opts ...string) (*exec.Cmd, error)
 
 // unsquashfsCmd is the command instance for executing unsquashfs command
 // in a non sandboxed environment when this package is used for unit tests.
-func unsquashfsCmd(unsquashfs string, dest string, filename string, rootless bool) (*exec.Cmd, error) {
-	args := make([]string, 0)
-	if rootless {
-		args = append(args, "-user-xattrs")
-	}
+func unsquashfsCmd(unsquashfs string, dest string, filename string, opts ...string) (*exec.Cmd, error) {
+	args := []string{}
+	args = append(args, opts...)
 	// remove the destination directory if any, if the directory is
 	// not empty (typically during image build), the unsafe option -f is
 	// set, this is unfortunately required by image build
@@ -42,6 +41,7 @@ func unsquashfsCmd(unsquashfs string, dest string, filename string, rootless boo
 		args = append(args, "-f")
 	}
 	args = append(args, "-d", dest, filename)
+	sylog.Debugf("Calling %s %v", unsquashfs, args)
 	return exec.Command(unsquashfs, args...), nil
 }
 
@@ -94,37 +94,57 @@ func (s *Squashfs) extract(files []string, reader io.Reader, dest string) error
 		}
 	}
 
-	// If we are running as non-root, first try with `-user-xattrs` so we won't fail trying
-	// to set system xatts. This isn't supported on unsquashfs 4.0 in RHEL6 so we
-	//  have to fall back to not using that option on failure.
-	if os.Geteuid() != 0 {
-		sylog.Debugf("Rootless extraction. Trying -user-xattrs for unsquashfs")
-
-		cmd, err := cmdFunc(s.UnsquashfsPath, dest, filename, true)
-		if err != nil {
-			return fmt.Errorf("command error: %s", err)
-		}
-		cmd.Args = append(cmd.Args, files...)
-		if stdin {
-			cmd.Stdin = reader
-		}
+	// First we try unsquashfs with appropriate xattr options.
+	// If we are in rootless mode we need "-user-xattrs" so we don't try to set system xattrs that require root.
+	// However...
+	//  1. This isn't supported on unsquashfs 4.0 in RHEL6 so we have to fall back to not using that option on failure.
+	//  2. Must check (user) xattrs are supported on the FS as unsquashfs >=4.4 will give a non-zero error code if
+	//	   it cannot set them, e.g. on tmpfs (#5668)
+	opts := []string{}
+	rootless := os.Geteuid() != 0
+
+	// Do we support user xattrs?
+	ok, err := TestUserXattr(filepath.Dir(dest))
+	if err != nil {
+		return err
+	}
+	// If we are in rootless mode & we support user xattrs, set -user-xattrs so that user xattrs are extracted, but
+	// system xattrs are ignored (needs root).
+	if ok && rootless {
+		opts = append(opts, "-user-xattrs")
+	}
+	// If user-xattrs aren't supported we need to disable setting of all xattrs.
+	if !ok {
+		opts = append(opts, "-no-xattrs")
+	}
+	// Now run unsquashfs with our 'best' options
+	sylog.Debugf("Trying unsquashfs options: %v", opts)
+	cmd, err := cmdFunc(s.UnsquashfsPath, dest, filename, opts...)
+	if err != nil {
+		return fmt.Errorf("command error: %s", err)
+	}
+	cmd.Args = append(cmd.Args, files...)
+	if stdin {
+		cmd.Stdin = reader
+	}
 
-		o, err := cmd.CombinedOutput()
-		if err == nil {
-			return nil
-		}
+	o, err := cmd.CombinedOutput()
+	if err == nil {
+		return nil
+	}
 
-		// Invalid options give output...
-		// SYNTAX: unsquashfs [options] filesystem [directories or files to extract]
-		if bytes.Contains(o, []byte("SYNTAX")) {
-			sylog.Warningf("unsquashfs does not support -user-xattrs. Images with system xattrs may fail to extract")
-		} else {
-			// A different error is fatal
-			return fmt.Errorf("extract command failed: %s: %s", string(o), err)
-		}
+	// Invalid options give output...
+	// SYNTAX: unsquashfs [options] filesystem [directories or files to extract]
+	if bytes.Contains(o, []byte("SYNTAX")) {
+		sylog.Warningf("unsquashfs does not support %v. Images with xattrs may fail to extract", opts)
+	} else {
+		// A different error is fatal
+		return fmt.Errorf("extract command failed: %s: %s", string(o), err)
 	}
 
-	cmd, err := cmdFunc(s.UnsquashfsPath, dest, filename, false)
+	// Now we fall back to running without additional xattr options - to do the best we can on old 4.0 squashfs that
+	// does not support them.
+	cmd, err = cmdFunc(s.UnsquashfsPath, dest, filename)
 	if err != nil {
 		return fmt.Errorf("command error: %s", err)
 	}
@@ -152,3 +172,17 @@ func (s *Squashfs) ExtractFiles(files []string, reader io.Reader, dest string) e
 	}
 	return s.extract(files, reader, dest)
 }
+
+// TestUserXattr tries to set a user xattr on PATH to ensure they are supported on this fs
+func TestUserXattr(path string) (ok bool, err error) {
+	tmp, err := ioutil.TempFile(path, "uxattr-")
+	defer os.Remove(tmp.Name())
+	tmp.Close()
+	err = unix.Setxattr(tmp.Name(), "user.singularity", []byte{}, 0)
+	if err == unix.ENOTSUP || err == unix.EOPNOTSUPP {
+		return false, nil
+	} else if err != nil {
+		return false, fmt.Errorf("while testing user xattr support at %s: %v", tmp.Name(), err)
+	}
+	return true, nil
+}

pkg/image/unpacker/squashfs_singularity.go

@@ -20,6 +20,7 @@ import (
 	"strings"
 
 	"github.com/sylabs/singularity/internal/pkg/buildcfg"
+	"github.com/sylabs/singularity/pkg/sylog"
 )
 
 func init() {
@@ -108,7 +109,7 @@ func getLibraries(binary string) ([]string, error) {
 
 // unsquashfsSandboxCmd is the command instance for executing unsquashfs command
 // in a sandboxed environment with singularity.
-func unsquashfsSandboxCmd(unsquashfs string, dest string, filename string, rootless bool) (*exec.Cmd, error) {
+func unsquashfsSandboxCmd(unsquashfs string, dest string, filename string, opts ...string) (*exec.Cmd, error) {
 	const (
 		// will contain both dest and filename inside the sandbox
 		rootfsImageDir = "/image"
@@ -216,14 +217,14 @@ func unsquashfsSandboxCmd(unsquashfs string, dest string, filename string, rootl
 
 	// unsquashfs execution arguments
 	args = append(args, unsquashfs)
-	if rootless {
-		args = append(args, "-user-xattrs")
-	}
+	args = append(args, opts...)
+
 	if overwrite {
 		args = append(args, "-f")
 	}
 	args = append(args, "-d", rootfsDest, filename)
 
+	sylog.Debugf("Calling wrapped unsquashfs: singularity %v", args)
 	cmd := exec.Command(filepath.Join(buildcfg.BINDIR, "singularity"), args...)
 	cmd.Dir = "/"
 	cmd.Env = []string{

pkg/image/unpacker/squashfs_test.go

@@ -37,13 +37,27 @@ func isExist(path string) bool {
 }
 
 func TestSquashfs(t *testing.T) {
+	// Run on default TMPDIR which is unlikely to be a tmpfs but may be.
+	t.Run("default", func(t *testing.T) {
+		testSquashfs(t, "")
+	})
+	// Run on /dev/shm which should be a tmpfs - catches #5668
+	t.Run("dev_shm", func(t *testing.T) {
+		if _, err := os.Stat("/dev/shm"); err != nil {
+			t.Skipf("Could not access /dev/shm")
+		}
+		testSquashfs(t, "/dev/shm")
+	})
+}
+
+func testSquashfs(t *testing.T, tmpParent string) {
 	s := NewSquashfs()
 
 	if !s.HasUnsquashfs() {
-		t.SkipNow()
+		t.Skip("unsquashfs not found")
 	}
 
-	dir, err := ioutil.TempDir("", "unpacker-")
+	dir, err := ioutil.TempDir(tmpParent, "unpacker-")
 	if err != nil {
 		t.Fatal(err)
 	}