Skip to content

Commit

Permalink
fix: report same path in multiple slices (#134)
Browse files Browse the repository at this point in the history 
Fix reporting of paths that were selected by multiple slices, explicitly or via globs.
  • Loading branch information
@letFunny
letFunny authored Jun 3, 2024
1 parent 6856584 commit a00ec09
Show file tree
Hide file tree
Showing 7 changed files with 672 additions and 214 deletions.
103 changes: 44 additions & 59 deletions internal/deb/extract.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,30 +26,33 @@ type ExtractOptions struct {
TargetDir string
Extract map[string][]ExtractInfo
// Create can optionally be set to control the creation of extracted entries.
// extractInfo is set to the matching entry in Extract, and is nil in cases where
// extractInfos is set to the matching entries in Extract, and is nil in cases where
// the created entry is implicit and unlisted (for example, parent directories).
Create func(extractInfo *ExtractInfo, options *fsutil.CreateOptions) error
Create func(extractInfos []ExtractInfo, options *fsutil.CreateOptions) error
}

type ExtractInfo struct {
Path string
Mode uint
Optional bool
Context any
}

func getValidOptions(options *ExtractOptions) (*ExtractOptions, error) {
for extractPath, extractInfos := range options.Extract {
isGlob := strings.ContainsAny(extractPath, "*?")
if isGlob {
if len(extractInfos) != 1 || extractInfos[0].Path != extractPath || extractInfos[0].Mode != 0 {
return nil, fmt.Errorf("when using wildcards source and target paths must match: %s", extractPath)
for _, extractInfo := range extractInfos {
if extractInfo.Path != extractPath || extractInfo.Mode != 0 {
return nil, fmt.Errorf("when using wildcards source and target paths must match: %s", extractPath)
}
}
}
}

if options.Create == nil {
validOpts := *options
validOpts.Create = func(_ *ExtractInfo, o *fsutil.CreateOptions) error {
validOpts.Create = func(_ []ExtractInfo, o *fsutil.CreateOptions) error {
_, err := fsutil.Create(o)
return err
}
Expand Down Expand Up @@ -123,33 +126,6 @@ func extractData(dataReader io.Reader, options *ExtractOptions) error {
syscall.Umask(oldUmask)
}()

shouldExtract := func(pkgPath string) (globPath string, ok bool) {
if pkgPath == "" {
return "", false
}
pkgPathIsDir := pkgPath[len(pkgPath)-1] == '/'
for extractPath, extractInfos := range options.Extract {
if extractPath == "" {
continue
}
switch {
case strings.ContainsAny(extractPath, "*?"):
if strdist.GlobPath(extractPath, pkgPath) {
return extractPath, true
}
case extractPath == pkgPath:
return "", true
case pkgPathIsDir:
for _, extractInfo := range extractInfos {
if strings.HasPrefix(extractInfo.Path, pkgPath) {
return "", true
}
}
}
}
return "", false
}

pendingPaths := make(map[string]bool)
for extractPath, extractInfos := range options.Extract {
for _, extractInfo := range extractInfos {
Expand Down Expand Up @@ -182,8 +158,7 @@ func extractData(dataReader io.Reader, options *ExtractOptions) error {
continue
}
sourcePath = sourcePath[1:]
globPath, ok := shouldExtract(sourcePath)
if !ok {
if sourcePath == "" {
continue
}

Expand All @@ -192,22 +167,32 @@ func extractData(dataReader io.Reader, options *ExtractOptions) error {
tarDirMode[sourcePath] = tarHeader.FileInfo().Mode()
}

//debugf("Extracting header: %#v", tarHeader)

var extractInfos []ExtractInfo
if globPath != "" {
extractInfos = options.Extract[globPath]
delete(pendingPaths, globPath)
} else {
extractInfos, ok = options.Extract[sourcePath]
if !ok {
// Find all globs and copies that require this source, and map them by
// their target paths on disk.
targetPaths := map[string][]ExtractInfo{}
for extractPath, extractInfos := range options.Extract {
if extractPath == "" {
continue
}
delete(pendingPaths, sourcePath)
if strings.ContainsAny(extractPath, "*?") {
if strdist.GlobPath(extractPath, sourcePath) {
targetPaths[sourcePath] = append(targetPaths[sourcePath], extractInfos...)
delete(pendingPaths, extractPath)
}
} else if extractPath == sourcePath {
for _, extractInfo := range extractInfos {
targetPaths[extractInfo.Path] = append(targetPaths[extractInfo.Path], extractInfo)
}
delete(pendingPaths, extractPath)
}
}
if len(targetPaths) == 0 {
// Nothing to do.
continue
}

var contentCache []byte
var contentIsCached = len(extractInfos) > 1 && !sourceIsDir && globPath == ""
var contentIsCached = len(targetPaths) > 1 && !sourceIsDir
if contentIsCached {
// Read and cache the content so it may be reused.
// As an alternative, to avoid having an entire file in
Expand All @@ -222,21 +207,24 @@ func extractData(dataReader io.Reader, options *ExtractOptions) error {
}

var pathReader io.Reader = tarReader
for _, extractInfo := range extractInfos {
for targetPath, extractInfos := range targetPaths {
if contentIsCached {
pathReader = bytes.NewReader(contentCache)
}
var relPath string
if globPath == "" {
relPath = extractInfo.Path
} else {
relPath = sourcePath
mode := extractInfos[0].Mode
for _, extractInfo := range extractInfos {
if extractInfo.Mode != mode {
if mode < extractInfo.Mode {
mode, extractInfo.Mode = extractInfo.Mode, mode
}
return fmt.Errorf("path %s requested twice with diverging mode: 0%03o != 0%03o", targetPath, mode, extractInfo.Mode)
}
}
if extractInfo.Mode != 0 {
tarHeader.Mode = int64(extractInfo.Mode)
if mode != 0 {
tarHeader.Mode = int64(mode)
}
// Create the parent directories using the permissions from the tarball.
parents := parentDirs(relPath)
parents := parentDirs(targetPath)
for _, path := range parents {
if path == "/" {
continue
Expand All @@ -259,19 +247,16 @@ func extractData(dataReader io.Reader, options *ExtractOptions) error {
}
// Create the entry itself.
createOptions := &fsutil.CreateOptions{
Path: filepath.Join(options.TargetDir, relPath),
Path: filepath.Join(options.TargetDir, targetPath),
Mode: tarHeader.FileInfo().Mode(),
Data: pathReader,
Link: tarHeader.Linkname,
MakeParents: true,
}
err := options.Create(&extractInfo, createOptions)
err := options.Create(extractInfos, createOptions)
if err != nil {
return err
}
if globPath != "" {
break
}
}
}

Expand Down
122 changes: 115 additions & 7 deletions internal/deb/extract_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,7 @@ var extractTests = []extractTest{{
"/dir/": "dir 0755",
"/dir/several/": "dir 0755",
},
notCreated: []string{"/dir/"},
notCreated: []string{},
}, {
summary: "Globbing for files with multiple levels at once",
pkgdata: testutil.PackageData["test-package"],
Expand All @@ -175,7 +175,7 @@ var extractTests = []extractTest{{
"/dir/several/levels/deep/": "dir 0755",
"/dir/several/levels/deep/file": "file 0644 6bc26dff",
},
notCreated: []string{"/dir/"},
notCreated: []string{},
}, {
summary: "Globbing multiple paths",
pkgdata: testutil.PackageData["test-package"],
Expand All @@ -197,7 +197,7 @@ var extractTests = []extractTest{{
"/dir/several/levels/deep/": "dir 0755",
"/dir/several/levels/deep/file": "file 0644 6bc26dff",
},
notCreated: []string{"/dir/"},
notCreated: []string{},
}, {
summary: "Globbing must have matching source and target",
pkgdata: testutil.PackageData["test-package"],
Expand All @@ -208,7 +208,7 @@ var extractTests = []extractTest{{
}},
},
},
error: `cannot extract .*: when using wildcards source and target paths must match: /foo/b\*\*`,
error: `cannot extract from package "test-package": when using wildcards source and target paths must match: /foo/b\*\*`,
}, {
summary: "Globbing must also have a single target",
pkgdata: testutil.PackageData["test-package"],
Expand All @@ -221,7 +221,7 @@ var extractTests = []extractTest{{
}},
},
},
error: `cannot extract .*: when using wildcards source and target paths must match: /foo/b\*\*`,
error: `cannot extract from package "test-package": when using wildcards source and target paths must match: /foo/b\*\*`,
}, {
summary: "Globbing cannot change modes",
pkgdata: testutil.PackageData["test-package"],
Expand All @@ -233,7 +233,7 @@ var extractTests = []extractTest{{
}},
},
},
error: `cannot extract .*: when using wildcards source and target paths must match: /dir/n\*\*`,
error: `cannot extract from package "test-package": when using wildcards source and target paths must match: /dir/n\*\*`,
}, {
summary: "Missing file",
pkgdata: testutil.PackageData["test-package"],
Expand Down Expand Up @@ -337,6 +337,21 @@ var extractTests = []extractTest{{
"/日本/語": "file 0644 85738f8f",
},
notCreated: []string{},
}, {
summary: "Entries for same destination must have the same mode",
pkgdata: testutil.PackageData["test-package"],
options: deb.ExtractOptions{
Extract: map[string][]deb.ExtractInfo{
"/dir/": []deb.ExtractInfo{{
Path: "/dir/",
Mode: 0777,
}},
"/d**": []deb.ExtractInfo{{
Path: "/d**",
}},
},
},
error: `cannot extract from package "test-package": path /dir/ requested twice with diverging mode: 0777 != 0000`,
}}

func (s *S) TestExtract(c *C) {
Expand All @@ -348,7 +363,7 @@ func (s *S) TestExtract(c *C) {
options.Package = "test-package"
options.TargetDir = dir
createdPaths := make(map[string]bool)
options.Create = func(_ *deb.ExtractInfo, o *fsutil.CreateOptions) error {
options.Create = func(_ []deb.ExtractInfo, o *fsutil.CreateOptions) error {
relPath := filepath.Clean("/" + strings.TrimPrefix(o.Path, dir))
if o.Mode.IsDir() {
relPath = relPath + "/"
Expand Down Expand Up @@ -386,3 +401,96 @@ func (s *S) TestExtract(c *C) {
c.Assert(result, DeepEquals, test.result)
}
}

var extractCreateCallbackTests = []struct {
summary string
pkgdata []byte
options deb.ExtractOptions
calls map[string][]deb.ExtractInfo
}{{
summary: "Create is called with the set of ExtractInfo(s) that match the file",
pkgdata: testutil.MustMakeDeb([]testutil.TarEntry{
testutil.Dir(0755, "./"),
testutil.Dir(0766, "./dir/"),
testutil.Reg(0644, "./dir/file", "whatever"),
}),
options: deb.ExtractOptions{
Extract: map[string][]deb.ExtractInfo{
"/dir/": []deb.ExtractInfo{{
Path: "/dir/",
}},
"/d**": []deb.ExtractInfo{{
Path: "/d**",
}},
"/d?r/": []deb.ExtractInfo{{
Path: "/d?r/",
}},
"/dir/file": []deb.ExtractInfo{{
Path: "/dir/file",
}, {
Path: "/dir/file-cpy",
}},
"/foo/": []deb.ExtractInfo{{
Path: "/foo/",
Optional: true,
}},
},
},
calls: map[string][]deb.ExtractInfo{
"/dir/": []deb.ExtractInfo{
deb.ExtractInfo{
Path: "/d**",
},
deb.ExtractInfo{
Path: "/d?r/",
},
deb.ExtractInfo{
Path: "/dir/",
},
},
"/dir/file": []deb.ExtractInfo{
deb.ExtractInfo{
Path: "/d**",
},
deb.ExtractInfo{
Path: "/dir/file",
},
},
"/dir/file-cpy": []deb.ExtractInfo{
deb.ExtractInfo{
Path: "/dir/file-cpy",
},
},
},
}}

func (s *S) TestExtractCreateCallback(c *C) {
for _, test := range extractCreateCallbackTests {
c.Logf("Test: %s", test.summary)
dir := c.MkDir()
options := test.options
options.Package = "test-package"
options.TargetDir = dir
createExtractInfos := map[string][]deb.ExtractInfo{}
options.Create = func(extractInfos []deb.ExtractInfo, o *fsutil.CreateOptions) error {
if extractInfos == nil {
// Creating implicit parent directories, we don't care about those.
return nil
}
relPath := filepath.Clean("/" + strings.TrimPrefix(o.Path, dir))
if o.Mode.IsDir() {
relPath = relPath + "/"
}
sort.Slice(extractInfos, func(i, j int) bool {
return strings.Compare(extractInfos[i].Path, extractInfos[j].Path) < 0
})
createExtractInfos[relPath] = extractInfos
return nil
}

err := deb.Extract(bytes.NewBuffer(test.pkgdata), &options)
c.Assert(err, IsNil)

c.Assert(createExtractInfos, DeepEquals, test.calls)
}
}
15 changes: 15 additions & 0 deletions internal/setup/setup_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1295,6 +1295,21 @@ var setupTests = []setupTest{{
`,
},
relerror: `package "mypkg" has invalid essential slice reference: "mypkg-slice"`,
}, {
summary: "Glob clashes within same package",
input: map[string]string{
"slices/mydir/test-package.yaml": `
package: test-package
slices:
myslice1:
contents:
/dir/**:
myslice2:
contents:
/dir/file: {text: "foo"}
`,
},
// TODO this should be an error because the content does not match.
}}

var defaultChiselYaml = `
Expand Down
Loading

0 comments on commit a00ec09

Please sign in to comment.