Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Set Umask as part of archive.Extract #730

Merged
merged 2 commits into from
Oct 1, 2021
Merged

Set Umask as part of archive.Extract #730

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ae2793a
Set Umask as part of archive.Extract
Sep 30, 2021
901a5d6
Move the unlock methods to be under defer
Sep 30, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 30 additions & 4 deletions archive/extract.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"io"
"os"
"path/filepath"
"sync"

"github.com/pkg/errors"
)
Expand All @@ -15,10 +16,35 @@ type PathMode struct {
Mode os.FileMode
}

var (
umaskLock sync.Mutex
extractCounter int
originalUmask int
)
Comment on lines +19 to +23
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can put these three variables inside a private struct if anyone thinks it'll be clearer.
Or we can even move the variables and the related functions (setUmaskIfNeeded and unsetUmaskIfNeeded) to an internal package.

Personally, I don't think it's necessary but it's an option.


func setUmaskIfNeeded() {
umaskLock.Lock()
defer umaskLock.Unlock()
extractCounter++
if extractCounter == 1 {
originalUmask = setUmask(0)
}
}

func unsetUmaskIfNeeded() {
umaskLock.Lock()
defer umaskLock.Unlock()
extractCounter--
if extractCounter == 0 {
_ = setUmask(originalUmask)
}
}

// 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)

Expand Down Expand Up @@ -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
Expand Down
230 changes: 126 additions & 104 deletions archive/extract_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand All @@ -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)

Expand All @@ -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)
}
}
2 changes: 1 addition & 1 deletion archive/tar_unix.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}

Expand Down
2 changes: 1 addition & 1 deletion archive/tar_windows.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down
2 changes: 1 addition & 1 deletion archive/tar_windows_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
10 changes: 2 additions & 8 deletions layers/extract.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
Loading