From 8d9b05b8658094d4dd30f5d38518819632ac5b16 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1=C5=A1=20Virtus?= Date: Thu, 1 Jun 2023 23:44:33 +0200 Subject: [PATCH 1/2] fsutil: Add Dir() and Clean() functions From the code: These functions exists because we work with slash terminated directory paths that come from deb package tarballs but standard library path functions treat slash terminated paths as unclean. We use ad-hoc code to make filepath.Dir() slash terminated in two places right now. For example this code targetDir := filepath.Dir(strings.TrimRight(targetPath, "/")) + "/" is not strictly correct as "/a/b/.///" will be turned into "/a/b/./" which is still "/a/b". Since we deal with slash terminated directory paths everywhere, create and use dedicated helper functions to process them. --- internal/fsutil/path.go | 47 ++++++++++++++++++++++++++++++++ internal/fsutil/path_test.go | 52 ++++++++++++++++++++++++++++++++++++ internal/slicer/slicer.go | 16 ++++------- 3 files changed, 104 insertions(+), 11 deletions(-) create mode 100644 internal/fsutil/path.go create mode 100644 internal/fsutil/path_test.go diff --git a/internal/fsutil/path.go b/internal/fsutil/path.go new file mode 100644 index 00000000..7d73c523 --- /dev/null +++ b/internal/fsutil/path.go @@ -0,0 +1,47 @@ +package fsutil + +import ( + "path/filepath" + "strings" +) + +// These functions exists because we work with slash terminated directory paths +// that come from deb package tarballs but standard library path functions +// treat slash terminated paths as unclean. + +// Like filepath.Dir() but trailing slash on input is ignored and the return +// value always includes trailing slash. +// +// Comparison of filepath.Dir() with fsutil.Dir(): +// +// filepath.Dir("/foo/bar/") == "/foo/bar" +// filepath.Dir("/foo/bar") == "/foo" +// +// fsutil.Dir("/foo/bar") == "/foo/" +// fsutil.Dir("/foo/bar/") == "/foo/" +func Dir(path string) string { + parent := filepath.Dir(filepath.Clean(path)) + if parent != "/" { + parent += "/" + } + return parent +} + +// Like filepath.Clean() but keeps trailing slash. +// +// Comparison of filepath.Clean() with fsutil.Clean(): +// +// filepath.Clean("/foo/bar") == "/foo/bar" +// filepath.Clean("/foo/bar/") == "/foo/bar" +// filepath.Clean("/foo/bar/.//) == "/foo/bar" +// +// fsutil.Clean("/foo/bar") == "/foo/bar" +// fsutil.Clean("/foo/bar/") == "/foo/bar/" +// fsutil.Clean("/foo/bar/.//") == "/foo/bar/" +func Clean(path string) string { + clean := filepath.Clean(path) + if strings.HasSuffix(path, "/") { + clean += "/" + } + return clean +} diff --git a/internal/fsutil/path_test.go b/internal/fsutil/path_test.go new file mode 100644 index 00000000..9d39230c --- /dev/null +++ b/internal/fsutil/path_test.go @@ -0,0 +1,52 @@ +package fsutil_test + +import ( + . "gopkg.in/check.v1" + + "github.com/canonical/chisel/internal/fsutil" +) + +var dirTests = []struct { + input string + output string +}{ + {"/a/b/c", "/a/b/"}, + {"/a/b/c/", "/a/b/"}, + {"/a/b/c//", "/a/b/"}, + {"/a/b/c/././", "/a/b/"}, + {"/a/b/c/.././", "/a/"}, + {"/a/b//c", "/a/b/"}, + {"/a/b/./c", "/a/b/"}, + {"a/b/./c", "a/b/"}, + {"./a/b/./c", "a/b/"}, + {"/", "/"}, + {"///.///", "/"}, + {".", "./"}, + {"", "./"}, +} + +var cleanTests = []struct { + input string + output string +}{ + {"/a/b/c", "/a/b/c"}, + {"/a/b/c/", "/a/b/c/"}, + {"/a/b/c/.//", "/a/b/c/"}, + {"/a/b//./c", "/a/b/c"}, + {"/a/b//./c/", "/a/b/c/"}, + {"/a/b/.././c/", "/a/c/"}, +} + +func (s *S) TestDir(c *C) { + for _, t := range dirTests { + c.Logf("%s => %s", t.input, t.output) + c.Assert(fsutil.Dir(t.input), Equals, t.output) + } +} + +func (s *S) TestClean(c *C) { + for _, t := range cleanTests { + c.Logf("%s => %s", t.input, t.output) + c.Assert(fsutil.Clean(t.input), Equals, t.output) + } +} diff --git a/internal/slicer/slicer.go b/internal/slicer/slicer.go index 8fca7426..50198a96 100644 --- a/internal/slicer/slicer.go +++ b/internal/slicer/slicer.go @@ -36,21 +36,15 @@ func Run(options *RunOptions) error { if path[0] != '/' { panic("bug: tried to add relative path to known paths") } - cleanPath := filepath.Clean(path) - slashPath := cleanPath - if path[len(path)-1] == '/' && cleanPath != "/" { - slashPath += "/" - } + path = fsutil.Clean(path) for { - if _, ok := knownPaths[slashPath]; ok { + if _, ok := knownPaths[path]; ok { break } - knownPaths[slashPath] = true - cleanPath = filepath.Dir(cleanPath) - if cleanPath == "/" { + knownPaths[path] = true + if path = fsutil.Dir(path); path == "/" { break } - slashPath = cleanPath + "/" } } @@ -113,7 +107,7 @@ func Run(options *RunOptions) error { hasCopyright = true } } else { - targetDir := filepath.Dir(strings.TrimRight(targetPath, "/")) + "/" + targetDir := fsutil.Dir(targetPath) if targetDir == "" || targetDir == "/" { continue } From f85d1ee7a3c4eaa9e708eed89e4409a6181d1502 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1=C5=A1=20Virtus?= Date: Mon, 12 Jun 2023 23:33:39 +0200 Subject: [PATCH 2/2] fsutil: Explicit parent directory creation Currently fsutil.Create() always creates missing parent directories with 0755 mode. While this is handy for some use cases, during extraction we want to create every path with attributes from the tarball and having parent directories created automatically hides the bugs where we fail to create some directories from the tarball. One such bug is when a glob matches. In this case, the directories before wildcards are created by os.MkdirAll() in fsutil.Create(). Don't fix the bugs now but make the creation of the parent directories explicit in CreateOptions with the Dirs attribute. Later, we will drop it from call sites where appropriate. --- internal/deb/extract.go | 2 ++ internal/fsutil/create.go | 20 +++++++------------- internal/fsutil/create_test.go | 9 +++++++++ internal/setup/fetch.go | 1 + internal/slicer/slicer.go | 1 + 5 files changed, 20 insertions(+), 13 deletions(-) diff --git a/internal/deb/extract.go b/internal/deb/extract.go index 1eb8908d..c7f08c7d 100644 --- a/internal/deb/extract.go +++ b/internal/deb/extract.go @@ -189,6 +189,7 @@ func extractData(dataReader io.Reader, options *ExtractOptions) error { err := fsutil.Create(&fsutil.CreateOptions{ Path: filepath.Join(options.TargetDir, sourcePath), Mode: tarHeader.FileInfo().Mode(), + Dirs: true, }) if err != nil { return err @@ -231,6 +232,7 @@ func extractData(dataReader io.Reader, options *ExtractOptions) error { Mode: tarHeader.FileInfo().Mode(), Data: pathReader, Link: tarHeader.Linkname, + Dirs: true, }) if err != nil { return err diff --git a/internal/fsutil/create.go b/internal/fsutil/create.go index 6407f0d9..632f67d9 100644 --- a/internal/fsutil/create.go +++ b/internal/fsutil/create.go @@ -13,10 +13,16 @@ type CreateOptions struct { Mode fs.FileMode Data io.Reader Link string + Dirs bool // create missing parent directories with 0755 mode } func Create(o *CreateOptions) error { var err error + if o.Dirs { + if err := os.MkdirAll(filepath.Dir(o.Path), 0755); err != nil { + return err + } + } switch o.Mode & fs.ModeType { case 0: err = createFile(o) @@ -32,11 +38,7 @@ func Create(o *CreateOptions) error { func createDir(o *CreateOptions) error { debugf("Creating directory: %s (mode %#o)", o.Path, o.Mode) - err := os.MkdirAll(filepath.Dir(o.Path), 0755) - if err != nil { - return err - } - err = os.Mkdir(o.Path, o.Mode) + err := os.Mkdir(o.Path, o.Mode) if os.IsExist(err) { err = os.Chmod(o.Path, o.Mode) } @@ -45,10 +47,6 @@ func createDir(o *CreateOptions) error { func createFile(o *CreateOptions) error { debugf("Writing file: %s (mode %#o)", o.Path, o.Mode) - err := os.MkdirAll(filepath.Dir(o.Path), 0755) - if err != nil && !os.IsExist(err) { - return err - } file, err := os.OpenFile(o.Path, os.O_CREATE|os.O_TRUNC|os.O_WRONLY, o.Mode) if err != nil { return err @@ -63,10 +61,6 @@ func createFile(o *CreateOptions) error { func createSymlink(o *CreateOptions) error { debugf("Creating symlink: %s => %s", o.Path, o.Link) - err := os.MkdirAll(filepath.Dir(o.Path), 0755) - if err != nil && !os.IsExist(err) { - return err - } fileinfo, err := os.Lstat(o.Path) if err == nil { if (fileinfo.Mode() & os.ModeSymlink) != 0 { diff --git a/internal/fsutil/create_test.go b/internal/fsutil/create_test.go index fe9f2a4d..39d42aed 100644 --- a/internal/fsutil/create_test.go +++ b/internal/fsutil/create_test.go @@ -23,6 +23,7 @@ var createTests = []createTest{{ Path: "foo/bar", Data: bytes.NewBufferString("data1"), Mode: 0444, + Dirs: true, }, result: map[string]string{ "/foo/": "dir 0755", @@ -33,6 +34,7 @@ var createTests = []createTest{{ Path: "foo/bar", Link: "../baz", Mode: fs.ModeSymlink, + Dirs: true, }, result: map[string]string{ "/foo/": "dir 0755", @@ -42,6 +44,7 @@ var createTests = []createTest{{ options: fsutil.CreateOptions{ Path: "foo/bar", Mode: fs.ModeDir | 0444, + Dirs: true, }, result: map[string]string{ "/foo/": "dir 0755", @@ -55,6 +58,12 @@ var createTests = []createTest{{ result: map[string]string{ "/tmp/": "dir 01775", }, +}, { + options: fsutil.CreateOptions{ + Path: "foo/bar", + Mode: fs.ModeDir | 0775, + }, + error: `.*: no such file or directory`, }} func (s *S) TestCreate(c *C) { diff --git a/internal/setup/fetch.go b/internal/setup/fetch.go index a18190e6..3137c477 100644 --- a/internal/setup/fetch.go +++ b/internal/setup/fetch.go @@ -143,6 +143,7 @@ func extractTar(dataReader io.Reader, targetDir string) error { Mode: tarHeader.FileInfo().Mode(), Data: tarReader, Link: tarHeader.Linkname, + Dirs: true, }) if err != nil { return err diff --git a/internal/slicer/slicer.go b/internal/slicer/slicer.go index 8fca7426..d07aaef6 100644 --- a/internal/slicer/slicer.go +++ b/internal/slicer/slicer.go @@ -216,6 +216,7 @@ func Run(options *RunOptions) error { Mode: tarHeader.FileInfo().Mode(), Data: fileContent, Link: linkTarget, + Dirs: true, }) if err != nil { return err