Set Umask as part of archive.Extract

Signed-off-by: Natalie Arellano <[email protected]>

archive/extract.go

@@ -6,6 +6,7 @@ import (
 	"io"
 	"os"
 	"path/filepath"
+	"sync"
 
 	"github.com/pkg/errors"
 )
@@ -15,10 +16,35 @@ type PathMode struct {
 	Mode os.FileMode
 }
 
+var (
+	umaskLock      sync.Mutex
+	extractCounter int
+	originalUmask  int
+)
+
+func setUmaskIfNeeded() {
+	umaskLock.Lock()
+	extractCounter++
+	if extractCounter == 1 {
+		originalUmask = setUmask(0)
+	}
+	umaskLock.Unlock()
+}
+
+func unsetUmaskIfNeeded() {
+	umaskLock.Lock()
+	extractCounter--
+	if extractCounter == 0 {
+		_ = setUmask(originalUmask)
+	}
+	umaskLock.Unlock()
+}
+
 // Extract reads all entries from TarReader and extracts them to the filesystem.
-// The umask must be unset before calling this function on unix, to ensure that files have the correct file mode. SetUmask can be used to set and unset the umask.
-// The provided umask will be applied to new directories that are created as parent directories of files in the tar, that do not themselves have headers in the tar.
-func Extract(tr TarReader, procUmask int) error {
+func Extract(tr TarReader) error {
+	setUmaskIfNeeded()
+	defer unsetUmaskIfNeeded()
+
 	buf := make([]byte, 32*32*1024)
 	dirsFound := make(map[string]bool)
 
@@ -52,7 +78,7 @@ func Extract(tr TarReader, procUmask int) error {
 			dirPath := filepath.Dir(hdr.Name)
 			if !dirsFound[dirPath] {
 				if _, err := os.Stat(dirPath); os.IsNotExist(err) {
-					if err := os.MkdirAll(dirPath, applyUmask(os.ModePerm, procUmask)); err != nil { // if there is no header for the parent directory in the tar, apply the provided umask
+					if err := os.MkdirAll(dirPath, applyUmask(os.ModePerm, originalUmask)); err != nil { // if there is no header for the parent directory in the tar, apply the provided umask
 						return errors.Wrapf(err, "failed to create parent dir %q for file %q", dirPath, hdr.Name)
 					}
 					dirsFound[dirPath] = true

archive/extract_test.go

@@ -12,49 +12,29 @@ import (
 
 	"github.com/sclevine/spec"
 	"github.com/sclevine/spec/report"
+	"golang.org/x/sync/errgroup"
 
 	"github.com/buildpacks/lifecycle/archive"
 	h "github.com/buildpacks/lifecycle/testhelpers"
 )
 
+var originalUmask int
+
 func TestArchiveExtract(t *testing.T) {
 	rand.Seed(time.Now().UTC().UnixNano())
+	originalUmask = h.GetUmask(t)
 	spec.Run(t, "extract", testExtract, spec.Report(report.Terminal{}))
 }
 
 func testExtract(t *testing.T, when spec.G, it spec.S) {
 	var (
-		tmpDir string
-		tr     *archive.NormalizingTarReader
-		ftr    *fakeTarReader
+		tmpDir    string
+		tr        *archive.NormalizingTarReader
+		pathModes []archive.PathMode
 	)
 
 	it.Before(func() {
-		var err error
-		tmpDir, err = ioutil.TempDir("", "archive-extract-test")
-		h.AssertNil(t, err)
-		ftr = &fakeTarReader{}
-		tr = archive.NewNormalizingTarReader(ftr)
-		tr.PrependDir(tmpDir)
-	})
-
-	it.After(func() {
-		h.AssertNil(t, os.RemoveAll(tmpDir))
-	})
-
-	when("#Extract", func() {
-		var pathModes = []archive.PathMode{
-			{"root", os.ModeDir + 0755},
-			{"root/readonly", os.ModeDir + 0500},
-			{"root/readonly/readonlysub", os.ModeDir + 0500},
-			{"root/readonly/readonlysub/somefile", 0444},
-			{"root/standarddir", os.ModeDir + 0755},
-			{"root/standarddir/somefile", 0644},
-			{"root/nonexistdirnotintar", os.ModeDir + os.FileMode(int(os.ModePerm)&^h.ProvidedUmask)},
-			{"root/symlinkdir", os.ModeSymlink + 0777},  //symlink permissions are not preserved from archive
-			{"root/symlinkfile", os.ModeSymlink + 0777}, //symlink permissions are not preserved from archive
-		}
-
+		tr, tmpDir = newFakeTarReader(t)
 		// Golang for Windows only implements owner permissions
 		if runtime.GOOS == "windows" {
 			pathModes = []archive.PathMode{
@@ -68,104 +48,69 @@ func testExtract(t *testing.T, when spec.G, it spec.S) {
 				{`root\symlinkdir`, os.ModeSymlink + 0666},
 				{`root\symlinkfile`, os.ModeSymlink + 0666},
 			}
+		} else {
+			pathModes = []archive.PathMode{
+				{"root", os.ModeDir + 0755},
+				{"root/readonly", os.ModeDir + 0500},
+				{"root/readonly/readonlysub", os.ModeDir + 0500},
+				{"root/readonly/readonlysub/somefile", 0444},
+				{"root/standarddir", os.ModeDir + 0755},
+				{"root/standarddir/somefile", 0644},
+				{"root/nonexistdirnotintar", os.ModeDir + os.FileMode(int(os.ModePerm)&^originalUmask)},
+				{"root/symlinkdir", os.ModeSymlink + 0777},  //symlink permissions are not preserved from archive
+				{"root/symlinkfile", os.ModeSymlink + 0777}, //symlink permissions are not preserved from archive
+			}
 		}
+	})
 
-		it.Before(func() {
-			ftr.pushHeader(&tar.Header{
-				Name:     "root/symlinkdir",
-				Typeflag: tar.TypeSymlink,
-				Linkname: filepath.Join("..", "not-in-archive-dir"),
-				Mode:     int64(os.ModeSymlink | 0755),
-			})
-			ftr.pushHeader(&tar.Header{
-				Name:     "root/symlinkfile",
-				Typeflag: tar.TypeSymlink,
-				Linkname: filepath.FromSlash("../not-in-archive-file"),
-				Mode:     int64(os.ModeSymlink | 0755),
-			})
-			ftr.pushHeader(&tar.Header{
-				Name:     "root/nonexistdirnotintar/somefile",
-				Typeflag: tar.TypeReg,
-				Mode:     int64(0644),
-			})
-			ftr.pushHeader(&tar.Header{
-				Name:     "root/standarddir/somefile",
-				Typeflag: tar.TypeReg,
-				Mode:     int64(0644),
-			})
-			ftr.pushHeader(&tar.Header{
-				Name:     "root/standarddir",
-				Typeflag: tar.TypeDir,
-				Mode:     int64(os.ModeDir | 0755),
-			})
-			ftr.pushHeader(&tar.Header{
-				Name:     "root/readonly/readonlysub/somefile",
-				Typeflag: tar.TypeReg,
-				Mode:     int64(0444),
-			})
-			ftr.pushHeader(&tar.Header{
-				Name:     "root/readonly/readonlysub",
-				Typeflag: tar.TypeDir,
-				Mode:     int64(os.ModeDir | 0500),
-			})
-			ftr.pushHeader(&tar.Header{
-				Name:     "root/readonly",
-				Typeflag: tar.TypeDir,
-				Mode:     int64(os.ModeDir | 0500),
-			})
-			ftr.pushHeader(&tar.Header{
-				Name:     "root",
-				Typeflag: tar.TypeDir,
-				Mode:     int64(os.ModeDir | 0755),
-			})
-		})
+	it.After(func() {
+		// Make all files os.ModePerm so they can all be cleaned up.
+		cleanupTmpDir(t, tmpDir, pathModes)
+	})
+
+	when("#Extract", func() {
+		it("extracts a tar file", func() {
+			h.AssertNil(t, archive.Extract(tr))
 
-		it.After(func() {
-			// Make all files os.ModePerm so they can all be cleaned up.
 			for _, pathMode := range pathModes {
-				extractedFile := filepath.Join(tmpDir, pathMode.Path)
-				if fi, err := os.Lstat(extractedFile); err == nil {
-					if fi.Mode()&os.ModeSymlink != 0 {
-						continue
-					}
-					if err := os.Chmod(extractedFile, os.ModePerm); err != nil {
-						h.AssertNil(t, err)
-					}
-				}
+				testPathPerms(t, tmpDir, pathMode.Path, pathMode.Mode)
 			}
 		})
 
-		it("extracts a tar file", func() {
-			oldUmask := archive.SetUmask(0)
-			defer archive.SetUmask(oldUmask)
+		it("thread safe", func() {
+			tr2, tmpDir2 := newFakeTarReader(t)
+			tr3, tmpDir3 := newFakeTarReader(t)
+
+			var g errgroup.Group
+			tars := []*archive.NormalizingTarReader{tr, tr2, tr3}
+			for _, tarReader := range tars {
+				tarReader := tarReader
+				g.Go(func() error {
+					return archive.Extract(tarReader)
+				})
+			}
 
-			h.AssertNil(t, archive.Extract(tr, h.ProvidedUmask))
+			h.AssertNil(t, g.Wait())
+			h.AssertEq(t, h.GetUmask(t), originalUmask)
 
-			for _, pathMode := range pathModes {
-				testPathPerms(t, tmpDir, pathMode.Path, pathMode.Mode)
-			}
+			cleanupTmpDir(t, tmpDir2, pathModes)
+			cleanupTmpDir(t, tmpDir3, pathModes)
 		})
 
 		it("fails if file exists where directory needs to be created", func() {
-			oldUmask := archive.SetUmask(0)
-			defer archive.SetUmask(oldUmask)
-
 			file, err := os.Create(filepath.Join(tmpDir, "root"))
 			h.AssertNil(t, err)
 			h.AssertNil(t, file.Close())
 
-			h.AssertError(t, archive.Extract(tr, oldUmask), "failed to create directory")
+			h.AssertError(t, archive.Extract(tr), "failed to create directory")
 		})
 
 		it("doesn't alter permissions of existing folders", func() {
-			oldUmask := archive.SetUmask(0)
-			defer archive.SetUmask(oldUmask)
-
 			h.AssertNil(t, os.Mkdir(filepath.Join(tmpDir, "root"), 0744))
 			// Update permissions in case umask was applied.
 			h.AssertNil(t, os.Chmod(filepath.Join(tmpDir, "root"), 0744))
 
-			h.AssertNil(t, archive.Extract(tr, oldUmask))
+			h.AssertNil(t, archive.Extract(tr))
 			fileInfo, err := os.Stat(filepath.Join(tmpDir, "root"))
 			h.AssertNil(t, err)
 
@@ -179,11 +124,88 @@ func testExtract(t *testing.T, when spec.G, it spec.S) {
 	})
 }
 
+func newFakeTarReader(t *testing.T) (*archive.NormalizingTarReader, string) {
+	tmpDir, err := ioutil.TempDir("", "archive-extract-test")
+	h.AssertNil(t, err)
+	ftr := &fakeTarReader{}
+	tr := archive.NewNormalizingTarReader(ftr)
+	tr.PrependDir(tmpDir)
+	pushHeaders(ftr)
+	return tr, tmpDir
+}
+
+func pushHeaders(ftr *fakeTarReader) {
+	ftr.pushHeader(&tar.Header{
+		Name:     "root/symlinkdir",
+		Typeflag: tar.TypeSymlink,
+		Linkname: filepath.Join("..", "not-in-archive-dir"),
+		Mode:     int64(os.ModeSymlink | 0755),
+	})
+	ftr.pushHeader(&tar.Header{
+		Name:     "root/symlinkfile",
+		Typeflag: tar.TypeSymlink,
+		Linkname: filepath.FromSlash("../not-in-archive-file"),
+		Mode:     int64(os.ModeSymlink | 0755),
+	})
+	ftr.pushHeader(&tar.Header{
+		Name:     "root/nonexistdirnotintar/somefile",
+		Typeflag: tar.TypeReg,
+		Mode:     int64(0644),
+	})
+	ftr.pushHeader(&tar.Header{
+		Name:     "root/standarddir/somefile",
+		Typeflag: tar.TypeReg,
+		Mode:     int64(0644),
+	})
+	ftr.pushHeader(&tar.Header{
+		Name:     "root/standarddir",
+		Typeflag: tar.TypeDir,
+		Mode:     int64(os.ModeDir | 0755),
+	})
+	ftr.pushHeader(&tar.Header{
+		Name:     "root/readonly/readonlysub/somefile",
+		Typeflag: tar.TypeReg,
+		Mode:     int64(0444),
+	})
+	ftr.pushHeader(&tar.Header{
+		Name:     "root/readonly/readonlysub",
+		Typeflag: tar.TypeDir,
+		Mode:     int64(os.ModeDir | 0500),
+	})
+	ftr.pushHeader(&tar.Header{
+		Name:     "root/readonly",
+		Typeflag: tar.TypeDir,
+		Mode:     int64(os.ModeDir | 0500),
+	})
+	ftr.pushHeader(&tar.Header{
+		Name:     "root",
+		Typeflag: tar.TypeDir,
+		Mode:     int64(os.ModeDir | 0755),
+	})
+}
+
+func cleanupTmpDir(t *testing.T, tmpDir string, pathModes []archive.PathMode) {
+	for _, pathMode := range pathModes {
+		extractedFile := filepath.Join(tmpDir, pathMode.Path)
+		if fi, err := os.Lstat(extractedFile); err == nil {
+			if fi.Mode()&os.ModeSymlink != 0 {
+				continue
+			}
+			if err := os.Chmod(extractedFile, os.ModePerm); err != nil {
+				h.AssertNil(t, err)
+			}
+		}
+	}
+	h.AssertNil(t, os.RemoveAll(tmpDir))
+}
+
 func testPathPerms(t *testing.T, parentDir, path string, expectedMode os.FileMode) {
 	extractedFile := filepath.Join(parentDir, path)
 
 	fileInfo, err := os.Lstat(extractedFile)
 	h.AssertNil(t, err)
 
-	h.AssertEq(t, fileInfo.Mode(), expectedMode)
+	if fileInfo.Mode() != expectedMode {
+		t.Fatalf("unexpected permissions for %s; got: %o, want: %o", path, fileInfo.Mode(), expectedMode)
+	}
 }

archive/tar_unix.go

@@ -9,7 +9,7 @@ import (
 	"golang.org/x/sys/unix"
 )
 
-func SetUmask(newMask int) (oldMask int) {
+func setUmask(newMask int) (oldMask int) {
 	return unix.Umask(newMask)
 }
 

archive/tar_windows.go

@@ -17,7 +17,7 @@ const (
 	hdrFileAttributes  = hdrMSWindowsPrefix + "fileattr"
 )
 
-func SetUmask(newMask int) (oldMask int) {
+func setUmask(newMask int) (oldMask int) {
 	// Not implemented on Windows
 	return 0
 }

archive/tar_windows_test.go

@@ -62,7 +62,7 @@ func testTarWindows(t *testing.T, when spec.G, it spec.S) {
 		})
 
 		it("sets dir attribute on windows directory symlinks", func() {
-			h.AssertNil(t, archive.Extract(tr, 0))
+			h.AssertNil(t, archive.Extract(tr))
 
 			extractedFile := filepath.Join(tmpDir, "root", "symlinkdir")
 			t.Log("asserting on", extractedFile)

layers/extract.go

@@ -11,15 +11,9 @@ import (
 // Extract extracts entries from r to the dest directory
 // Contents of r should be an OCI layer.
 // If dest is an empty string files with be extracted to `/` or `c:\` on unix and windows filesystems respectively.
-// The umask must be unset before calling this function on unix, to ensure that files have the correct file mode. SetUmask can be used to set and unset the umask.
-// The provided umask will be applied to new directories that are created as parent directories of files in the tar, that do not themselves have headers in the tar.
-func Extract(r io.Reader, dest string, procUmask int) error {
+func Extract(r io.Reader, dest string) error {
 	tr := tarReader(r, dest)
-	return archive.Extract(tr, procUmask)
-}
-
-func SetUmask(newMask int) (oldMask int) {
-	return archive.SetUmask(newMask)
+	return archive.Extract(tr)
 }
 
 func tarReader(r io.Reader, dest string) archive.TarReader {