merge branch 'pr-141'

LGTMs: @cyphar
Closes #141

CHANGELOG.md

@@ -44,6 +44,8 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 - `umoci` now uses [`SecureJoin`][securejoin] rather than a patched version of
   `FollowSymlinkInScope`. The two implementations are roughly equivalent, but
   `SecureJoin` has a nicer API and is maintained as a separate project.
+- Switched to using `golang.org/x/sys/unix` over `syscall` where possible,
+  which makes the codebase significantly cleaner. openSUSE/umoci#141
 
 [cii]: https://bestpractices.coreinfrastructure.org/projects/1084
 [rspec-v1.0.0]: https://github.com/opencontainers/runtime-spec/releases/tag/v1.0.0

cmd/umoci/repack.go

@@ -25,12 +25,12 @@ import (
 	"time"
 
 	"github.com/apex/log"
-	"github.com/openSUSE/umoci"
 	"github.com/openSUSE/umoci/mutate"
 	"github.com/openSUSE/umoci/oci/cas"
 	"github.com/openSUSE/umoci/oci/casext"
 	igen "github.com/openSUSE/umoci/oci/config/generate"
 	"github.com/openSUSE/umoci/oci/layer"
+	"github.com/openSUSE/umoci/pkg/fseval"
 	"github.com/openSUSE/umoci/pkg/mtreefilter"
 	ispec "github.com/opencontainers/image-spec/specs-go/v1"
 	"github.com/pkg/errors"
@@ -152,9 +152,9 @@ func repack(ctx *cli.Context) error {
 		"keywords": MtreeKeywords,
 	}).Debugf("umoci: parsed mtree spec")
 
-	fsEval := umoci.DefaultFsEval
+	fsEval := fseval.DefaultFsEval
 	if meta.MapOptions.Rootless {
-		fsEval = umoci.RootlessFsEval
+		fsEval = fseval.RootlessFsEval
 	}
 
 	log.Info("computing filesystem diff ...")

cmd/umoci/unpack.go

@@ -24,10 +24,10 @@ import (
 	"strings"
 
 	"github.com/apex/log"
-	"github.com/openSUSE/umoci"
 	"github.com/openSUSE/umoci/oci/cas"
 	"github.com/openSUSE/umoci/oci/casext"
 	"github.com/openSUSE/umoci/oci/layer"
+	"github.com/openSUSE/umoci/pkg/fseval"
 	"github.com/openSUSE/umoci/pkg/idtools"
 	ispec "github.com/opencontainers/image-spec/specs-go/v1"
 	"github.com/pkg/errors"
@@ -187,9 +187,9 @@ func unpack(ctx *cli.Context) error {
 		"mtree":    mtreePath,
 	}).Debugf("umoci: generating mtree manifest")
 
-	fsEval := umoci.DefaultFsEval
+	fsEval := fseval.DefaultFsEval
 	if meta.MapOptions.Rootless {
-		fsEval = umoci.RootlessFsEval
+		fsEval = fseval.RootlessFsEval
 	}
 
 	log.Info("computing filesystem manifest ...")

hack/vendor.sh

@@ -86,7 +86,7 @@ clean() {
 	# Remove any extra files that we know we don't care about.
 	echo -n "pruning unused files, "
 	find vendor -type f -name '*_test.go' -exec rm -vf '{}' ';'
-	find vendor -regextype posix-extended -type f -not '(' -regex '.*\.(c|h|go)' -or '(' "${importantfiles[@]}" ')' ')' -exec rm -vf '{}' ';'
+	find vendor -regextype posix-extended -type f -not '(' -regex '.*\.(s|c|h|go)' -or '(' "${importantfiles[@]}" ')' ')' -exec rm -vf '{}' ';'
 
 	# Remove self from vendor.
 	echo -n "pruning self from vendor, "
@@ -125,7 +125,7 @@ clone github.com/opencontainers/runtime-spec v1.0.0
 clone github.com/opencontainers/runtime-tools 2d270b8764c02228eeb13e36f076f5ce6f2e3591
 clone github.com/syndtr/gocapability 2c00daeb6c3b45114c80ac44119e7b8801fdd852
 clone golang.org/x/crypto b8a2a83acfe6e6770b75de42d5ff4c67596675c0 https://github.com/golang/crypto
-clone golang.org/x/sys d75a52659825e75fff6158388dddc6a5b04f9ba5 https://github.com/golang/sys
+clone golang.org/x/sys fb4cac33e3196ff7f507ab9b2d2a44b0142f5b5a https://github.com/golang/sys
 clone github.com/docker/go-units v0.3.1
 clone github.com/pkg/errors v0.8.0
 clone github.com/apex/log afb2e76037a5f36542c77e88ef8aef9f469b09f8

oci/cas/drivers/dir/dir.go

@@ -25,12 +25,12 @@ import (
 	"path/filepath"
 
 	"github.com/openSUSE/umoci/oci/cas"
-	"github.com/openSUSE/umoci/pkg/system"
 	"github.com/opencontainers/go-digest"
 	imeta "github.com/opencontainers/image-spec/specs-go"
 	ispec "github.com/opencontainers/image-spec/specs-go/v1"
 	"github.com/pkg/errors"
 	"golang.org/x/net/context"
+	"golang.org/x/sys/unix"
 )
 
 const (
@@ -90,7 +90,7 @@ func (e *dirEngine) ensureTempDir() error {
 		if err != nil {
 			return errors.Wrap(err, "open tempdir for lock")
 		}
-		if err := system.Flock(e.tempFile.Fd(), true); err != nil {
+		if err := unix.Flock(int(e.tempFile.Fd()), unix.LOCK_EX|unix.LOCK_NB); err != nil {
 			return errors.Wrap(err, "lock tempdir")
 		}
 
@@ -326,12 +326,12 @@ func (e *dirEngine) Clean(ctx context.Context) error {
 		}
 		defer cfh.Close()
 
-		if err := system.Flock(cfh.Fd(), true); err != nil {
+		if err := unix.Flock(int(cfh.Fd()), unix.LOCK_EX|unix.LOCK_NB); err != nil {
 			// If we fail to get a flock(2) then it's probably already locked,
 			// so we shouldn't touch it.
 			continue
 		}
-		defer system.Unflock(cfh.Fd())
+		defer unix.Flock(int(cfh.Fd()), unix.LOCK_UN)
 
 		if err := os.RemoveAll(path); err != nil {
 			return errors.Wrap(err, "remove garbage path")
@@ -345,7 +345,7 @@ func (e *dirEngine) Clean(ctx context.Context) error {
 // fail.
 func (e *dirEngine) Close() error {
 	if e.temp != "" {
-		if err := system.Unflock(e.tempFile.Fd()); err != nil {
+		if err := unix.Flock(int(e.tempFile.Fd()), unix.LOCK_UN); err != nil {
 			return errors.Wrap(err, "unlock tempdir")
 		}
 		if err := e.tempFile.Close(); err != nil {

oci/cas/drivers/dir/dir_test.go

@@ -23,12 +23,12 @@ import (
 	"io/ioutil"
 	"os"
 	"path/filepath"
-	"syscall"
 	"testing"
 
 	"github.com/openSUSE/umoci/oci/cas"
 	"github.com/pkg/errors"
 	"golang.org/x/net/context"
+	"golang.org/x/sys/unix"
 )
 
 // NOTE: These tests aren't really testing OCI-style manifests. It's all just
@@ -47,10 +47,10 @@ func readonly(t *testing.T, path string) {
 
 	t.Logf("mounting %s as readonly", path)
 
-	if err := syscall.Mount(path, path, "", syscall.MS_BIND|syscall.MS_RDONLY, ""); err != nil {
+	if err := unix.Mount(path, path, "", unix.MS_BIND|unix.MS_RDONLY, ""); err != nil {
 		t.Fatalf("mount %s as ro: %s", path, err)
 	}
-	if err := syscall.Mount("none", path, "", syscall.MS_BIND|syscall.MS_REMOUNT|syscall.MS_RDONLY, ""); err != nil {
+	if err := unix.Mount("none", path, "", unix.MS_BIND|unix.MS_REMOUNT|unix.MS_RDONLY, ""); err != nil {
 		t.Fatalf("mount %s as ro: %s", path, err)
 	}
 }
@@ -62,7 +62,7 @@ func readwrite(t *testing.T, path string) {
 		t.Skip()
 	}
 
-	if err := syscall.Unmount(path, syscall.MNT_DETACH); err != nil {
+	if err := unix.Unmount(path, unix.MNT_DETACH); err != nil {
 		t.Fatalf("unmount %s: %s", path, err)
 	}
 }

oci/layer/tar_extract.go

@@ -28,7 +28,7 @@ import (
 
 	"github.com/apex/log"
 	"github.com/cyphar/filepath-securejoin"
-	"github.com/openSUSE/umoci"
+	"github.com/openSUSE/umoci/pkg/fseval"
 	"github.com/openSUSE/umoci/pkg/system"
 	"github.com/pkg/errors"
 )
@@ -37,15 +37,15 @@ type tarExtractor struct {
 	// mapOptions is the set of mapping options to use when extracting filesystem layers.
 	mapOptions MapOptions
 
-	// fsEval is an umoci.FsEval used for extraction.
-	fsEval umoci.FsEval
+	// fsEval is an fseval.FsEval used for extraction.
+	fsEval fseval.FsEval
 }
 
 // newTarExtractor creates a new tarExtractor.
 func newTarExtractor(opt MapOptions) *tarExtractor {
-	var fsEval umoci.FsEval = umoci.DefaultFsEval
+	fsEval := fseval.DefaultFsEval
 	if opt.Rootless {
-		fsEval = umoci.RootlessFsEval
+		fsEval = fseval.RootlessFsEval
 	}
 
 	return &tarExtractor{

oci/layer/tar_extract_test.go

@@ -23,11 +23,11 @@ import (
 	"io/ioutil"
 	"os"
 	"path/filepath"
-	"syscall"
 	"testing"
 	"time"
 
 	rspec "github.com/opencontainers/runtime-spec/specs-go"
+	"golang.org/x/sys/unix"
 )
 
 // TODO: Test the parent directory metadata is kept the same when unpacking.
@@ -393,40 +393,34 @@ func TestUnpackHardlink(t *testing.T) {
 	}
 
 	// Now we have to check the inode numbers.
-	regFi, err := os.Lstat(filepath.Join(dir, regFile))
-	if err != nil {
+	var regFi, symFi, hardAFi, hardBFi unix.Stat_t
+
+	if err := unix.Lstat(filepath.Join(dir, regFile), &regFi); err != nil {
 		t.Fatalf("could not stat regular file: %s", err)
 	}
-	symFi, err := os.Lstat(filepath.Join(dir, symFile))
-	if err != nil {
+	if err := unix.Lstat(filepath.Join(dir, symFile), &symFi); err != nil {
 		t.Fatalf("could not stat symlink: %s", err)
 	}
-	hardAFi, err := os.Lstat(filepath.Join(dir, hardFileA))
-	if err != nil {
+	if err := unix.Lstat(filepath.Join(dir, hardFileA), &hardAFi); err != nil {
 		t.Fatalf("could not stat hardlinkA: %s", err)
 	}
-	hardBFi, err := os.Lstat(filepath.Join(dir, hardFileB))
-	if err != nil {
+	if err := unix.Lstat(filepath.Join(dir, hardFileB), &hardBFi); err != nil {
 		t.Fatalf("could not stat hardlinkB: %s", err)
 	}
 
 	// This test only runs on Linux anyway.
-	regIno := regFi.Sys().(*syscall.Stat_t).Ino
-	symIno := symFi.Sys().(*syscall.Stat_t).Ino
-	hardAIno := hardAFi.Sys().(*syscall.Stat_t).Ino
-	hardBIno := hardBFi.Sys().(*syscall.Stat_t).Ino
 
-	if regIno == symIno {
-		t.Errorf("regular and symlink have the same inode! ino=%d", regIno)
+	if regFi.Ino == symFi.Ino {
+		t.Errorf("regular and symlink have the same inode! ino=%d", regFi.Ino)
 	}
-	if hardAIno == hardBIno {
-		t.Errorf("both hardlinks have the same inode! ino=%d", hardAIno)
+	if hardAFi.Ino == hardBFi.Ino {
+		t.Errorf("both hardlinks have the same inode! ino=%d", hardAFi.Ino)
 	}
-	if hardAIno != regIno {
-		t.Errorf("hardlink to regular has a different inode: reg=%d hard=%d", regIno, hardAIno)
+	if hardAFi.Ino != regFi.Ino {
+		t.Errorf("hardlink to regular has a different inode: reg=%d hard=%d", regFi.Ino, hardAFi.Ino)
 	}
-	if hardBIno != symIno {
-		t.Errorf("hardlink to symlink has a different inode: sym=%d hard=%d", symIno, hardBIno)
+	if hardBFi.Ino != symFi.Ino {
+		t.Errorf("hardlink to symlink has a different inode: sym=%d hard=%d", symFi.Ino, hardBFi.Ino)
 	}
 
 	// Double-check readlink.
@@ -443,21 +437,17 @@ func TestUnpackHardlink(t *testing.T) {
 	}
 
 	// Make sure that uid and gid don't apply to hardlinks.
-	regUID := int(regFi.Sys().(*syscall.Stat_t).Uid)
-	if regUID != os.Getuid() {
-		t.Errorf("regular file: uid was changed by hardlink unpack: expected=%d got=%d", os.Getuid(), regUID)
+	if int(regFi.Uid) != os.Getuid() {
+		t.Errorf("regular file: uid was changed by hardlink unpack: expected=%d got=%d", os.Getuid(), regFi.Uid)
 	}
-	regGID := int(regFi.Sys().(*syscall.Stat_t).Gid)
-	if regGID != os.Getgid() {
-		t.Errorf("regular file: gid was changed by hardlink unpack: expected=%d got=%d", os.Getgid(), regGID)
+	if int(regFi.Gid) != os.Getgid() {
+		t.Errorf("regular file: gid was changed by hardlink unpack: expected=%d got=%d", os.Getgid(), regFi.Gid)
 	}
-	symUID := int(symFi.Sys().(*syscall.Stat_t).Uid)
-	if symUID != os.Getuid() {
-		t.Errorf("symlink: uid was changed by hardlink unpack: expected=%d got=%d", os.Getuid(), symUID)
+	if int(symFi.Uid) != os.Getuid() {
+		t.Errorf("symlink: uid was changed by hardlink unpack: expected=%d got=%d", os.Getuid(), symFi.Uid)
 	}
-	symGID := int(symFi.Sys().(*syscall.Stat_t).Gid)
-	if symGID != os.Getgid() {
-		t.Errorf("symlink: gid was changed by hardlink unpack: expected=%d got=%d", os.Getgid(), symGID)
+	if int(symFi.Gid) != os.Getgid() {
+		t.Errorf("symlink: gid was changed by hardlink unpack: expected=%d got=%d", os.Getgid(), symFi.Gid)
 	}
 }
 
@@ -488,8 +478,9 @@ func TestUnpackEntryMap(t *testing.T) {
 			t.Logf("running with uid=%#v gid=%#v", test.uidMap, test.gidMap)
 
 			var (
-				hdr            *tar.Header
-				hdrUID, hdrGID int
+				hdrUID, hdrGID, uUID, uGID int
+				hdr                        *tar.Header
+				fi                         unix.Stat_t
 
 				ctrValue = []byte("some content we won't check")
 				regFile  = "regular"
@@ -519,16 +510,17 @@ func TestUnpackEntryMap(t *testing.T) {
 			if err := te.unpackEntry(dir, hdr, bytes.NewBuffer(ctrValue)); err != nil {
 				t.Fatalf("regfile: unexpected unpackEntry error: %s", err)
 			}
-			if fi, err := os.Lstat(filepath.Join(dir, hdr.Name)); err != nil {
+
+			if err := unix.Lstat(filepath.Join(dir, hdr.Name), &fi); err != nil {
 				t.Errorf("failed to lstat %s: %s", hdr.Name, err)
 			} else {
-				theUID := int(fi.Sys().(*syscall.Stat_t).Uid)
-				theGID := int(fi.Sys().(*syscall.Stat_t).Gid)
-				if theUID != int(test.uidMap.HostID)+hdrUID {
-					t.Errorf("file %s has the wrong uid mapping: got=%d expected=%d", hdr.Name, theUID, int(test.uidMap.HostID)+hdrUID)
+				uUID = int(fi.Uid)
+				uGID = int(fi.Gid)
+				if uUID != int(test.uidMap.HostID)+hdrUID {
+					t.Errorf("file %s has the wrong uid mapping: got=%d expected=%d", hdr.Name, uUID, int(test.uidMap.HostID)+hdrUID)
 				}
-				if theGID != int(test.gidMap.HostID)+hdrGID {
-					t.Errorf("file %s has the wrong gid mapping: got=%d expected=%d", hdr.Name, theGID, int(test.gidMap.HostID)+hdrGID)
+				if uGID != int(test.gidMap.HostID)+hdrGID {
+					t.Errorf("file %s has the wrong gid mapping: got=%d expected=%d", hdr.Name, uGID, int(test.gidMap.HostID)+hdrGID)
 				}
 			}
 
@@ -547,16 +539,17 @@ func TestUnpackEntryMap(t *testing.T) {
 			if err := te.unpackEntry(dir, hdr, bytes.NewBuffer(ctrValue)); err != nil {
 				t.Fatalf("regdir: unexpected unpackEntry error: %s", err)
 			}
-			if fi, err := os.Lstat(filepath.Join(dir, hdr.Name)); err != nil {
+
+			if err := unix.Lstat(filepath.Join(dir, hdr.Name), &fi); err != nil {
 				t.Errorf("failed to lstat %s: %s", hdr.Name, err)
 			} else {
-				theUID := int(fi.Sys().(*syscall.Stat_t).Uid)
-				theGID := int(fi.Sys().(*syscall.Stat_t).Gid)
-				if theUID != int(test.uidMap.HostID)+hdrUID {
-					t.Errorf("file %s has the wrong uid mapping: got=%d expected=%d", hdr.Name, theUID, int(test.uidMap.HostID)+hdrUID)
+				uUID = int(fi.Uid)
+				uGID = int(fi.Gid)
+				if uUID != int(test.uidMap.HostID)+hdrUID {
+					t.Errorf("file %s has the wrong uid mapping: got=%d expected=%d", hdr.Name, uUID, int(test.uidMap.HostID)+hdrUID)
 				}
-				if theGID != int(test.gidMap.HostID)+hdrGID {
-					t.Errorf("file %s has the wrong gid mapping: got=%d expected=%d", hdr.Name, theGID, int(test.gidMap.HostID)+hdrGID)
+				if uGID != int(test.gidMap.HostID)+hdrGID {
+					t.Errorf("file %s has the wrong gid mapping: got=%d expected=%d", hdr.Name, uGID, int(test.gidMap.HostID)+hdrGID)
 				}
 			}
 
@@ -575,16 +568,17 @@ func TestUnpackEntryMap(t *testing.T) {
 			if err := te.unpackEntry(dir, hdr, bytes.NewBuffer(ctrValue)); err != nil {
 				t.Fatalf("regdir: unexpected unpackEntry error: %s", err)
 			}
-			if fi, err := os.Lstat(filepath.Join(dir, hdr.Name)); err != nil {
+
+			if err := unix.Lstat(filepath.Join(dir, hdr.Name), &fi); err != nil {
 				t.Errorf("failed to lstat %s: %s", hdr.Name, err)
 			} else {
-				theUID := int(fi.Sys().(*syscall.Stat_t).Uid)
-				theGID := int(fi.Sys().(*syscall.Stat_t).Gid)
-				if theUID != int(test.uidMap.HostID)+hdrUID {
-					t.Errorf("file %s has the wrong uid mapping: got=%d expected=%d", hdr.Name, theUID, int(test.uidMap.HostID)+hdrUID)
+				uUID = int(fi.Uid)
+				uGID = int(fi.Gid)
+				if uUID != int(test.uidMap.HostID)+hdrUID {
+					t.Errorf("file %s has the wrong uid mapping: got=%d expected=%d", hdr.Name, uUID, int(test.uidMap.HostID)+hdrUID)
 				}
-				if theGID != int(test.gidMap.HostID)+hdrGID {
-					t.Errorf("file %s has the wrong gid mapping: got=%d expected=%d", hdr.Name, theGID, int(test.gidMap.HostID)+hdrGID)
+				if uGID != int(test.gidMap.HostID)+hdrGID {
+					t.Errorf("file %s has the wrong gid mapping: got=%d expected=%d", hdr.Name, uGID, int(test.gidMap.HostID)+hdrGID)
 				}
 			}
 
@@ -603,16 +597,17 @@ func TestUnpackEntryMap(t *testing.T) {
 			if err := te.unpackEntry(dir, hdr, bytes.NewBuffer(ctrValue)); err != nil {
 				t.Fatalf("regdir: unexpected unpackEntry error: %s", err)
 			}
-			if fi, err := os.Lstat(filepath.Join(dir, hdr.Name)); err != nil {
+
+			if err := unix.Lstat(filepath.Join(dir, hdr.Name), &fi); err != nil {
 				t.Errorf("failed to lstat %s: %s", hdr.Name, err)
 			} else {
-				theUID := int(fi.Sys().(*syscall.Stat_t).Uid)
-				theGID := int(fi.Sys().(*syscall.Stat_t).Gid)
-				if theUID != int(test.uidMap.HostID)+hdrUID {
-					t.Errorf("file %s has the wrong uid mapping: got=%d expected=%d", hdr.Name, theUID, int(test.uidMap.HostID)+hdrUID)
+				uUID = int(fi.Uid)
+				uGID = int(fi.Gid)
+				if uUID != int(test.uidMap.HostID)+hdrUID {
+					t.Errorf("file %s has the wrong uid mapping: got=%d expected=%d", hdr.Name, uUID, int(test.uidMap.HostID)+hdrUID)
 				}
-				if theGID != int(test.gidMap.HostID)+hdrGID {
-					t.Errorf("file %s has the wrong gid mapping: got=%d expected=%d", hdr.Name, theGID, int(test.gidMap.HostID)+hdrGID)
+				if uGID != int(test.gidMap.HostID)+hdrGID {
+					t.Errorf("file %s has the wrong gid mapping: got=%d expected=%d", hdr.Name, uGID, int(test.gidMap.HostID)+hdrGID)
 				}
 			}
 		}