From b1d5f26aec7c3dc16ec6805ebe860c7ad2cf02b2 Mon Sep 17 00:00:00 2001 From: Alberto Carretero Date: Thu, 17 Oct 2024 13:09:22 +0200 Subject: [PATCH 1/4] bugfix: explicit parents override implicit --- internal/deb/extract.go | 11 +++--- internal/fsutil/create.go | 14 ++++++- internal/fsutil/create_test.go | 69 ++++++++++++++++++++++++++++++++++ internal/slicer/slicer_test.go | 64 +++++++++++++++++++++++++------ 4 files changed, 141 insertions(+), 17 deletions(-) diff --git a/internal/deb/extract.go b/internal/deb/extract.go index 07f219bd..d9e84875 100644 --- a/internal/deb/extract.go +++ b/internal/deb/extract.go @@ -247,11 +247,12 @@ func extractData(dataReader io.Reader, options *ExtractOptions) error { } // Create the entry itself. createOptions := &fsutil.CreateOptions{ - Path: filepath.Join(options.TargetDir, targetPath), - Mode: tarHeader.FileInfo().Mode(), - Data: pathReader, - Link: tarHeader.Linkname, - MakeParents: true, + Path: filepath.Join(options.TargetDir, targetPath), + Mode: tarHeader.FileInfo().Mode(), + Data: pathReader, + Link: tarHeader.Linkname, + MakeParents: true, + OverrideMode: true, } err := options.Create(extractInfos, createOptions) if err != nil { diff --git a/internal/fsutil/create.go b/internal/fsutil/create.go index f76271f1..89a3d636 100644 --- a/internal/fsutil/create.go +++ b/internal/fsutil/create.go @@ -19,6 +19,9 @@ type CreateOptions struct { // If MakeParents is true, missing parent directories of Path are // created with permissions 0755. MakeParents bool + // For the mode to be updated if the entry already exists, OverrideMode has + // to be set to true. It does not affect symlinks. + OverrideMode bool } type Entry struct { @@ -65,9 +68,18 @@ func Create(options *CreateOptions) (*Entry, error) { if err != nil { return nil, err } + mode := s.Mode() + if o.OverrideMode && mode != o.Mode && o.Mode&fs.ModeSymlink == 0 { + err := os.Chmod(o.Path, o.Mode) + if err != nil { + return nil, err + } + mode = o.Mode + } + entry := &Entry{ Path: o.Path, - Mode: s.Mode(), + Mode: mode, SHA256: hash, Size: rp.size, Link: o.Link, diff --git a/internal/fsutil/create_test.go b/internal/fsutil/create_test.go index d41bbf5a..dd3daa78 100644 --- a/internal/fsutil/create_test.go +++ b/internal/fsutil/create_test.go @@ -93,6 +93,75 @@ var createTests = []createTest{{ // mode is not updated. "/foo": "file 0666 d67e2e94", }, +}, { + options: fsutil.CreateOptions{ + Path: "foo", + Mode: fs.ModeDir | 0775, + OverrideMode: true, + }, + hackdir: func(c *C, dir string) { + c.Assert(os.Mkdir(filepath.Join(dir, "foo/"), fs.ModeDir|0765), IsNil) + }, + result: map[string]string{ + // mode is updated. + "/foo/": "dir 0775", + }, +}, { + options: fsutil.CreateOptions{ + Path: "foo", + Mode: 0775, + Data: bytes.NewBufferString("whatever"), + OverrideMode: true, + }, + hackdir: func(c *C, dir string) { + err := os.WriteFile(filepath.Join(dir, "foo"), []byte("data"), 0666) + c.Assert(err, IsNil) + }, + result: map[string]string{ + // mode is updated. + "/foo": "file 0775 85738f8f", + }, +}, { + options: fsutil.CreateOptions{ + Path: "foo", + Link: "./bar", + Mode: 0666 | fs.ModeSymlink, + }, + hackdir: func(c *C, dir string) { + err := os.WriteFile(filepath.Join(dir, "foo"), []byte("data"), 0666) + c.Assert(err, IsNil) + }, + result: map[string]string{ + "/foo": "symlink ./bar", + }, +}, { + options: fsutil.CreateOptions{ + Path: "bar", + // Existing link with different target. + Link: "other", + Mode: 0666 | fs.ModeSymlink, + }, + hackdir: func(c *C, dir string) { + err := os.Symlink("foo", filepath.Join(dir, "bar")) + c.Assert(err, IsNil) + }, + result: map[string]string{ + "/bar": "symlink other", + }, +}, { + options: fsutil.CreateOptions{ + Path: "bar", + // Existing link with same target. + Link: "foo", + Mode: 0666 | fs.ModeSymlink, + }, + hackdir: func(c *C, dir string) { + err := os.Symlink("foo", filepath.Join(dir, "bar")) + c.Assert(err, IsNil) + }, + result: map[string]string{ + "/bar": "symlink foo", + }, }} func (s *S) TestCreate(c *C) { diff --git a/internal/slicer/slicer_test.go b/internal/slicer/slicer_test.go index 0f3bdfd8..fdd26dfa 100644 --- a/internal/slicer/slicer_test.go +++ b/internal/slicer/slicer_test.go @@ -320,33 +320,33 @@ var slicerTests = []slicerTest{{ "/file": "file 0644 fc02ca0e {other-package_myslice}", }, }, { - summary: "Install two packages, explicit path has preference over implicit parent", + summary: "Install two packages, explicit path has preference over implicit parent (alphabetical order)", slices: []setup.SliceKey{ - {"implicit-parent", "myslice"}, - {"explicit-dir", "myslice"}}, + {"a-implicit-parent", "myslice"}, + {"b-explicit-dir", "myslice"}}, pkgs: map[string]testutil.TestPackage{ - "implicit-parent": { + "a-implicit-parent": { Data: testutil.MustMakeDeb([]testutil.TarEntry{ testutil.Dir(0755, "./dir/"), testutil.Reg(0644, "./dir/file", "random"), }), }, - "explicit-dir": { + "b-explicit-dir": { Data: testutil.MustMakeDeb([]testutil.TarEntry{ testutil.Dir(01777, "./dir/"), }), }, }, release: map[string]string{ - "slices/mydir/implicit-parent.yaml": ` - package: implicit-parent + "slices/mydir/a-implicit-parent.yaml": ` + package: a-implicit-parent slices: myslice: contents: /dir/file: `, - "slices/mydir/explicit-dir.yaml": ` - package: explicit-dir + "slices/mydir/b-explicit-dir.yaml": ` + package: b-explicit-dir slices: myslice: contents: @@ -358,8 +358,50 @@ var slicerTests = []slicerTest{{ "/dir/file": "file 0644 a441b15f", }, manifestPaths: map[string]string{ - "/dir/": "dir 01777 {explicit-dir_myslice}", - "/dir/file": "file 0644 a441b15f {implicit-parent_myslice}", + "/dir/": "dir 01777 {b-explicit-dir_myslice}", + "/dir/file": "file 0644 a441b15f {a-implicit-parent_myslice}", + }, +}, { + summary: "Install two packages, explicit path has preference over implicit parent (reverse order)", + slices: []setup.SliceKey{ + {"b-implicit-parent", "myslice"}, + {"a-explicit-dir", "myslice"}}, + pkgs: map[string]testutil.TestPackage{ + "b-implicit-parent": { + Data: testutil.MustMakeDeb([]testutil.TarEntry{ + testutil.Dir(0755, "./dir/"), + testutil.Reg(0644, "./dir/file", "random"), + }), + }, + "a-explicit-dir": { + Data: testutil.MustMakeDeb([]testutil.TarEntry{ + testutil.Dir(01777, "./dir/"), + }), + }, + }, + release: map[string]string{ + "slices/mydir/b-implicit-parent.yaml": ` + package: b-implicit-parent + slices: + myslice: + contents: + /dir/file: + `, + "slices/mydir/a-explicit-dir.yaml": ` + package: a-explicit-dir + slices: + myslice: + contents: + /dir/: + `, + }, + filesystem: map[string]string{ + "/dir/": "dir 01777", + "/dir/file": "file 0644 a441b15f", + }, + manifestPaths: map[string]string{ + "/dir/": "dir 01777 {a-explicit-dir_myslice}", + "/dir/file": "file 0644 a441b15f {b-implicit-parent_myslice}", }, }, { summary: "Valid same file in two slices in different packages", From 7d34dc0b993709c18f3f3d342b6afa07ccd0f9ec Mon Sep 17 00:00:00 2001 From: Alberto Carretero Date: Thu, 24 Oct 2024 10:05:38 +0200 Subject: [PATCH 2/4] test: OverrideMode does not affect symlinks --- internal/fsutil/create_test.go | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/internal/fsutil/create_test.go b/internal/fsutil/create_test.go index dd3daa78..be086ea5 100644 --- a/internal/fsutil/create_test.go +++ b/internal/fsutil/create_test.go @@ -134,6 +134,24 @@ var createTests = []createTest{{ result: map[string]string{ "/foo": "symlink ./bar", }, +}, { + options: fsutil.CreateOptions{ + Path: "foo", + Link: "./bar", + Mode: 0776 | fs.ModeSymlink, + OverrideMode: true, + }, + hackdir: func(c *C, dir string) { + err := os.WriteFile(filepath.Join(dir, "bar"), []byte("data"), 0666) + c.Assert(err, IsNil) + err = os.WriteFile(filepath.Join(dir, "foo"), []byte("data"), 0666) + c.Assert(err, IsNil) + }, + result: map[string]string{ + "/foo": "symlink ./bar", + // mode is not updated. + "/bar": "file 0666 3a6eb079", + }, }, { options: fsutil.CreateOptions{ Path: "bar", From 8d712cd7af1e1753f87ed02f9fdf308852bef7dd Mon Sep 17 00:00:00 2001 From: Alberto Carretero Date: Mon, 11 Nov 2024 17:13:03 +0100 Subject: [PATCH 3/4] update comment --- internal/fsutil/create.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/fsutil/create.go b/internal/fsutil/create.go index 89a3d636..76561b77 100644 --- a/internal/fsutil/create.go +++ b/internal/fsutil/create.go @@ -19,8 +19,8 @@ type CreateOptions struct { // If MakeParents is true, missing parent directories of Path are // created with permissions 0755. MakeParents bool - // For the mode to be updated if the entry already exists, OverrideMode has - // to be set to true. It does not affect symlinks. + // If OverrideMode is true and entry already exists, update the mode. Does + // not affect symlinks. OverrideMode bool } From c94fdb1ffa66964fbb34a91ae9bad756e41e001f Mon Sep 17 00:00:00 2001 From: Alberto Carretero Date: Tue, 12 Nov 2024 11:10:15 +0100 Subject: [PATCH 4/4] add test in extractor --- internal/deb/extract_test.go | 27 ++++++++++++++++++++++++--- 1 file changed, 24 insertions(+), 3 deletions(-) diff --git a/internal/deb/extract_test.go b/internal/deb/extract_test.go index 22a1fd18..db73df86 100644 --- a/internal/deb/extract_test.go +++ b/internal/deb/extract_test.go @@ -2,6 +2,8 @@ package deb_test import ( "bytes" + "os" + "path" "path/filepath" "sort" "strings" @@ -17,7 +19,7 @@ type extractTest struct { summary string pkgdata []byte options deb.ExtractOptions - hackopt func(o *deb.ExtractOptions) + hackopt func(c *C, o *deb.ExtractOptions) result map[string]string // paths which the extractor did not create explicitly. notCreated []string @@ -98,7 +100,7 @@ var extractTests = []extractTest{{ "/dir/several/levels/deep/file": "file 0644 6bc26dff", "/other-dir/": "dir 0755", }, - hackopt: func(o *deb.ExtractOptions) { + hackopt: func(c *C, o *deb.ExtractOptions) { o.Create = nil }, }, { @@ -352,6 +354,25 @@ var extractTests = []extractTest{{ }, }, error: `cannot extract from package "test-package": path /dir/ requested twice with diverging mode: 0777 != 0000`, +}, { + summary: "Explicit extraction overrides existing file", + pkgdata: testutil.PackageData["test-package"], + options: deb.ExtractOptions{ + Extract: map[string][]deb.ExtractInfo{ + "/dir/": []deb.ExtractInfo{{ + Path: "/dir/", + Mode: 0777, + }}, + }, + }, + hackopt: func(c *C, o *deb.ExtractOptions) { + err := os.Mkdir(path.Join(o.TargetDir, "/dir"), 0666) + c.Assert(err, IsNil) + }, + result: map[string]string{ + "/dir/": "dir 0777", + }, + notCreated: []string{}, }} func (s *S) TestExtract(c *C) { @@ -374,7 +395,7 @@ func (s *S) TestExtract(c *C) { } if test.hackopt != nil { - test.hackopt(&options) + test.hackopt(c, &options) } err := deb.Extract(bytes.NewBuffer(test.pkgdata), &options)