bugfix: parent directory permissions

This commit fixes three bugs regarding parent directory creation.

The first one is that implicit folders were overwriting the permissions
of explicit ones in slice definitions (e.g. slice A declares /foo/bar
and slice B /foo, if slice A was installed after B then the implicit
permissions of /foo from A would prevail.

The second one is that we were creating parent directories of optional
files which did not exist.

The third bug is that we were not using the permissions from the tarball
for parent directories if the path was a glob.

internal/deb/extract.go

@@ -6,8 +6,10 @@ import (
 	"compress/gzip"
 	"fmt"
 	"io"
+	"io/fs"
 	"os"
 	"path/filepath"
+	"slices"
 	"sort"
 	"strings"
 	"syscall"
@@ -158,6 +160,13 @@ func extractData(dataReader io.Reader, options *ExtractOptions) error {
 		}
 	}
 
+	// When creating a file we will iterate through its parent directories and
+	// create them with the permissions defined in the tar.
+	//
+	// The assumption is that the tar entries of the parent directories appear
+	// before the entry for the file itself. This is the case for .deb files but
+	// not for all tar archives.
+	tarDirPermissions := make(map[string]fs.FileMode)
 	tarReader := tar.NewReader(dataReader)
 	for {
 		tarHeader, err := tarReader.Next()
@@ -179,6 +188,9 @@ func extractData(dataReader io.Reader, options *ExtractOptions) error {
 		}
 
 		sourceIsDir := sourcePath[len(sourcePath)-1] == '/'
+		if sourceIsDir {
+			tarDirPermissions[sourcePath] = tarHeader.FileInfo().Mode()
+		}
 
 		//debugf("Extracting header: %#v", tarHeader)
 
@@ -190,24 +202,12 @@ func extractData(dataReader io.Reader, options *ExtractOptions) error {
 			extractInfos, ok = options.Extract[sourcePath]
 			if ok {
 				delete(pendingPaths, sourcePath)
-			} else {
-				// Base directory for extracted content. Relevant mainly to preserve
-				// the metadata, since the extracted content itself will also create
-				// any missing directories unaccounted for in the options.
-				createOptions := &fsutil.CreateOptions{
-					Path:        filepath.Join(options.TargetDir, sourcePath),
-					Mode:        tarHeader.FileInfo().Mode(),
-					MakeParents: true,
-				}
-				err := options.Create(nil, createOptions)
-				if err != nil {
-					return err
-				}
-				continue
 			}
 		}
 
 		var contentCache []byte
+		// contentIsCached is used when extracting the same file to multiple
+		// targets to avoid reading it from disk several times.
 		var contentIsCached = len(extractInfos) > 1 && !sourceIsDir && globPath == ""
 		if contentIsCached {
 			// Read and cache the content so it may be reused.
@@ -227,17 +227,44 @@ func extractData(dataReader io.Reader, options *ExtractOptions) error {
 			if contentIsCached {
 				pathReader = bytes.NewReader(contentCache)
 			}
-			var targetPath string
+			var relPath string
 			if globPath == "" {
-				targetPath = filepath.Join(options.TargetDir, extractInfo.Path)
+				relPath = extractInfo.Path
 			} else {
-				targetPath = filepath.Join(options.TargetDir, sourcePath)
+				relPath = sourcePath
 			}
 			if extractInfo.Mode != 0 {
 				tarHeader.Mode = int64(extractInfo.Mode)
 			}
+			// Create the parent directories using the permissions from the tarball.
+			parents := orderedParentDirs(relPath)
+			for _, path := range parents {
+				if path == "/" {
+					continue
+				}
+				perm, ok := tarDirPermissions[path]
+				if !ok {
+					continue
+				}
+				absPath := filepath.Join(options.TargetDir, path)
+				if _, err := os.Stat(absPath); !os.IsNotExist(err) {
+					// We dont want to this implicit parent directory to overwrite
+					// an existing directory.
+					continue
+				}
+				fsOptions := &fsutil.CreateOptions{
+					Path:        absPath,
+					Mode:        perm,
+					MakeParents: true,
+				}
+				err := options.Create(nil, fsOptions)
+				if err != nil {
+					return err
+				}
+			}
+			// Create the entry itself.
 			createOptions := &fsutil.CreateOptions{
-				Path:        targetPath,
+				Path:        filepath.Join(options.TargetDir, relPath),
 				Mode:        tarHeader.FileInfo().Mode(),
 				Data:        pathReader,
 				Link:        tarHeader.Linkname,
@@ -268,3 +295,18 @@ func extractData(dataReader io.Reader, options *ExtractOptions) error {
 
 	return nil
 }
+
+func orderedParentDirs(path string) []string {
+	var parents []string
+	path = filepath.Clean(path)
+	for {
+		path = filepath.Dir(path)
+		if path == "/" {
+			break
+		}
+		parents = append(parents, path+"/")
+	}
+
+	slices.Reverse(parents)
+	return parents
+}

internal/deb/extract_test.go

@@ -300,8 +300,7 @@ var extractTests = []extractTest{{
 		},
 	},
 	result: map[string]string{
-		"/dir/":       "dir 0755",
-		"/other-dir/": "dir 0755",
+		"/dir/": "dir 0755",
 	},
 	notCreated: []string{},
 }, {

internal/slicer/slicer_test.go

@@ -33,7 +33,6 @@ type slicerTest struct {
 	// TODO:
 	// The results of the report do not conform to the planned implementation
 	// yet. Namely:
-	// * Parent directories of {text} files are reported even though they should not.
 	// * We do not track removed directories or changes done in Starlark.
 	report map[string]string
 	error  string
@@ -206,6 +205,32 @@ var slicerTests = []slicerTest{{
 	report: map[string]string{
 		"/parent/new/": "dir 0755 {test-package_myslice}",
 	},
+}, {
+	summary: "Create new file using glob and preserve parent directory permissions",
+	slices:  []setup.SliceKey{{"test-package", "myslice"}},
+	pkgs: map[string][]byte{
+		"test-package": testutil.PackageData["test-package"],
+	},
+	release: map[string]string{
+		"slices/mydir/test-package.yaml": `
+			package: test-package
+			slices:
+				myslice:
+					contents:
+						# Note the missing /parent/ and /parent/permissions/ here.
+						/parent/**:
+		`,
+	},
+	filesystem: map[string]string{
+		"/parent/":                 "dir 01777", // This is the magic.
+		"/parent/permissions/":     "dir 0764",  // This is the magic.
+		"/parent/permissions/file": "file 0755 722c14b3",
+	},
+	report: map[string]string{
+		"/parent/":                 "dir 01777 {test-package_myslice}",
+		"/parent/permissions/":     "dir 0764 {test-package_myslice}",
+		"/parent/permissions/file": "file 0755 722c14b3 {test-package_myslice}",
+	},
 }, {
 	summary: "Conditional architecture",
 	arch:    "amd64",
@@ -341,8 +366,7 @@ var slicerTests = []slicerTest{{
 		`,
 	},
 	filesystem: map[string]string{
-		// TODO this is the wrong mode for the directory, it should be 01777.
-		"/dir/":     "dir 0755",
+		"/dir/":     "dir 01777",
 		"/dir/file": "file 0644 a441b15f",
 	},
 	report: map[string]string{
@@ -380,8 +404,7 @@ var slicerTests = []slicerTest{{
 		`,
 	},
 	filesystem: map[string]string{
-		// TODO this is the wrong mode for the directory, it should be 01777.
-		"/dir/":     "dir 0755",
+		"/dir/":     "dir 01777",
 		"/dir/file": "file 0644 a441b15f",
 	},
 	report: map[string]string{