From 1daddb6d76b7b994c655212bbdf04b062974d696 Mon Sep 17 00:00:00 2001 From: Tonis Tiigi Date: Wed, 20 Apr 2016 17:08:47 -0700 Subject: [PATCH 01/27] Safer file io for configuration files Signed-off-by: Tonis Tiigi --- pkg/ioutils/fswriters.go | 75 +++++++++++++++++++++++++++++++++++ pkg/ioutils/fswriters_test.go | 31 +++++++++++++++ 2 files changed, 106 insertions(+) create mode 100644 pkg/ioutils/fswriters.go create mode 100644 pkg/ioutils/fswriters_test.go diff --git a/pkg/ioutils/fswriters.go b/pkg/ioutils/fswriters.go new file mode 100644 index 00000000..ca976707 --- /dev/null +++ b/pkg/ioutils/fswriters.go @@ -0,0 +1,75 @@ +package ioutils + +import ( + "io" + "io/ioutil" + "os" + "path/filepath" +) + +// NewAtomicFileWriter returns WriteCloser so that writing to it writes to a +// temporary file and closing it atomically changes the temporary file to +// destination path. Writing and closing concurrently is not allowed. +func NewAtomicFileWriter(filename string, perm os.FileMode) (io.WriteCloser, error) { + f, err := ioutil.TempFile(filepath.Dir(filename), ".tmp-"+filepath.Base(filename)) + if err != nil { + return nil, err + } + abspath, err := filepath.Abs(filename) + if err != nil { + return nil, err + } + return &atomicFileWriter{ + f: f, + fn: abspath, + }, nil +} + +// AtomicWriteFile atomically writes data to a file named by filename. +func AtomicWriteFile(filename string, data []byte, perm os.FileMode) error { + f, err := NewAtomicFileWriter(filename, perm) + if err != nil { + return err + } + n, err := f.Write(data) + if err == nil && n < len(data) { + err = io.ErrShortWrite + } + if err1 := f.Close(); err == nil { + err = err1 + } + return err +} + +type atomicFileWriter struct { + f *os.File + fn string + writeErr error +} + +func (w *atomicFileWriter) Write(dt []byte) (int, error) { + n, err := w.f.Write(dt) + if err != nil { + w.writeErr = err + } + return n, err +} + +func (w *atomicFileWriter) Close() (retErr error) { + defer func() { + if retErr != nil { + os.Remove(w.f.Name()) + } + }() + if err := w.f.Sync(); err != nil { + w.f.Close() + return err + } + if err := w.f.Close(); err != nil { + return err + } + if w.writeErr == nil { + return os.Rename(w.f.Name(), w.fn) + } + return nil +} diff --git a/pkg/ioutils/fswriters_test.go b/pkg/ioutils/fswriters_test.go new file mode 100644 index 00000000..40717a51 --- /dev/null +++ b/pkg/ioutils/fswriters_test.go @@ -0,0 +1,31 @@ +package ioutils + +import ( + "bytes" + "io/ioutil" + "os" + "path/filepath" + "testing" +) + +func TestAtomicWriteToFile(t *testing.T) { + tmpDir, err := ioutil.TempDir("", "atomic-writers-test") + if err != nil { + t.Fatalf("Error when creating temporary directory: %s", err) + } + defer os.RemoveAll(tmpDir) + + expected := []byte("barbaz") + if err := AtomicWriteFile(filepath.Join(tmpDir, "foo"), expected, 0600); err != nil { + t.Fatalf("Error writing to file: %v", err) + } + + actual, err := ioutil.ReadFile(filepath.Join(tmpDir, "foo")) + if err != nil { + t.Fatalf("Error reading from file: %v", err) + } + + if bytes.Compare(actual, expected) != 0 { + t.Fatalf("Data mismatch, expected %q, got %q", expected, actual) + } +} From 9f03097bcc398d3ca4b6117cda2501ce2261fbba Mon Sep 17 00:00:00 2001 From: Derek McGowan Date: Wed, 29 Jun 2016 13:09:13 -0700 Subject: [PATCH 02/27] Set permission on atomic file write Perform chmod before rename with the atomic file writer. Ensure writeErr is set on short write and file is removed on write error. Signed-off-by: Derek McGowan (github: dmcgowan) --- pkg/ioutils/fswriters.go | 13 ++++++++++--- pkg/ioutils/fswriters_test.go | 10 +++++++++- 2 files changed, 19 insertions(+), 4 deletions(-) diff --git a/pkg/ioutils/fswriters.go b/pkg/ioutils/fswriters.go index ca976707..6dc50a03 100644 --- a/pkg/ioutils/fswriters.go +++ b/pkg/ioutils/fswriters.go @@ -15,13 +15,15 @@ func NewAtomicFileWriter(filename string, perm os.FileMode) (io.WriteCloser, err if err != nil { return nil, err } + abspath, err := filepath.Abs(filename) if err != nil { return nil, err } return &atomicFileWriter{ - f: f, - fn: abspath, + f: f, + fn: abspath, + perm: perm, }, nil } @@ -34,6 +36,7 @@ func AtomicWriteFile(filename string, data []byte, perm os.FileMode) error { n, err := f.Write(data) if err == nil && n < len(data) { err = io.ErrShortWrite + f.(*atomicFileWriter).writeErr = err } if err1 := f.Close(); err == nil { err = err1 @@ -45,6 +48,7 @@ type atomicFileWriter struct { f *os.File fn string writeErr error + perm os.FileMode } func (w *atomicFileWriter) Write(dt []byte) (int, error) { @@ -57,7 +61,7 @@ func (w *atomicFileWriter) Write(dt []byte) (int, error) { func (w *atomicFileWriter) Close() (retErr error) { defer func() { - if retErr != nil { + if retErr != nil || w.writeErr != nil { os.Remove(w.f.Name()) } }() @@ -68,6 +72,9 @@ func (w *atomicFileWriter) Close() (retErr error) { if err := w.f.Close(); err != nil { return err } + if err := os.Chmod(w.f.Name(), w.perm); err != nil { + return err + } if w.writeErr == nil { return os.Rename(w.f.Name(), w.fn) } diff --git a/pkg/ioutils/fswriters_test.go b/pkg/ioutils/fswriters_test.go index 40717a51..470ca1a6 100644 --- a/pkg/ioutils/fswriters_test.go +++ b/pkg/ioutils/fswriters_test.go @@ -16,7 +16,7 @@ func TestAtomicWriteToFile(t *testing.T) { defer os.RemoveAll(tmpDir) expected := []byte("barbaz") - if err := AtomicWriteFile(filepath.Join(tmpDir, "foo"), expected, 0600); err != nil { + if err := AtomicWriteFile(filepath.Join(tmpDir, "foo"), expected, 0666); err != nil { t.Fatalf("Error writing to file: %v", err) } @@ -28,4 +28,12 @@ func TestAtomicWriteToFile(t *testing.T) { if bytes.Compare(actual, expected) != 0 { t.Fatalf("Data mismatch, expected %q, got %q", expected, actual) } + + st, err := os.Stat(filepath.Join(tmpDir, "foo")) + if err != nil { + t.Fatalf("Error statting file: %v", err) + } + if expected := os.FileMode(0666); st.Mode() != expected { + t.Fatalf("Mode mismatched, expected %o, got %o", expected, st.Mode()) + } } From 48cfa01244a2bc8614c5ca6915740bb00d5a0053 Mon Sep 17 00:00:00 2001 From: Derek McGowan Date: Tue, 9 Aug 2016 11:55:17 -0700 Subject: [PATCH 03/27] Update layer store to sync transaction files before committing Fixes case where shutdown occurs before content is synced to disked on layer creation. This case can leave the layer store in an bad state and require manual recovery. This change ensures all files are synced to disk before a layer is committed. Any shutdown that occurs will only cause the layer to not show up but will allow it to be repulled or recreated without error. Added generic io logic to ioutils package to abstract it out of the layer store package. Signed-off-by: Derek McGowan --- pkg/ioutils/fswriters.go | 80 +++++++++++++++++++++++++++++ pkg/ioutils/fswriters_test.go | 97 ++++++++++++++++++++++++++++++++++- 2 files changed, 175 insertions(+), 2 deletions(-) diff --git a/pkg/ioutils/fswriters.go b/pkg/ioutils/fswriters.go index 6dc50a03..a56c4626 100644 --- a/pkg/ioutils/fswriters.go +++ b/pkg/ioutils/fswriters.go @@ -80,3 +80,83 @@ func (w *atomicFileWriter) Close() (retErr error) { } return nil } + +// AtomicWriteSet is used to atomically write a set +// of files and ensure they are visible at the same time. +// Must be committed to a new directory. +type AtomicWriteSet struct { + root string +} + +// NewAtomicWriteSet creates a new atomic write set to +// atomically create a set of files. The given directory +// is used as the base directory for storing files before +// commit. If no temporary directory is given the system +// default is used. +func NewAtomicWriteSet(tmpDir string) (*AtomicWriteSet, error) { + td, err := ioutil.TempDir(tmpDir, "write-set-") + if err != nil { + return nil, err + } + + return &AtomicWriteSet{ + root: td, + }, nil +} + +// WriteFile writes a file to the set, guaranteeing the file +// has been synced. +func (ws *AtomicWriteSet) WriteFile(filename string, data []byte, perm os.FileMode) error { + f, err := ws.FileWriter(filename, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, perm) + if err != nil { + return err + } + n, err := f.Write(data) + if err == nil && n < len(data) { + err = io.ErrShortWrite + } + if err1 := f.Close(); err == nil { + err = err1 + } + return err +} + +type syncFileCloser struct { + *os.File +} + +func (w syncFileCloser) Close() error { + err := w.File.Sync() + if err1 := w.File.Close(); err == nil { + err = err1 + } + return err +} + +// FileWriter opens a file writer inside the set. The file +// should be synced and closed before calling commit. +func (ws *AtomicWriteSet) FileWriter(name string, flag int, perm os.FileMode) (io.WriteCloser, error) { + f, err := os.OpenFile(filepath.Join(ws.root, name), flag, perm) + if err != nil { + return nil, err + } + return syncFileCloser{f}, nil +} + +// Cancel cancels the set and removes all temporary data +// created in the set. +func (ws *AtomicWriteSet) Cancel() error { + return os.RemoveAll(ws.root) +} + +// Commit moves all created files to the target directory. The +// target directory must not exist and the parent of the target +// directory must exist. +func (ws *AtomicWriteSet) Commit(target string) error { + return os.Rename(ws.root, target) +} + +// String returns the location the set is writing to. +func (ws *AtomicWriteSet) String() string { + return ws.root +} diff --git a/pkg/ioutils/fswriters_test.go b/pkg/ioutils/fswriters_test.go index 470ca1a6..c4d14193 100644 --- a/pkg/ioutils/fswriters_test.go +++ b/pkg/ioutils/fswriters_test.go @@ -5,9 +5,21 @@ import ( "io/ioutil" "os" "path/filepath" + "runtime" "testing" ) +var ( + testMode os.FileMode = 0640 +) + +func init() { + // Windows does not support full Linux file mode + if runtime.GOOS == "windows" { + testMode = 0666 + } +} + func TestAtomicWriteToFile(t *testing.T) { tmpDir, err := ioutil.TempDir("", "atomic-writers-test") if err != nil { @@ -16,7 +28,7 @@ func TestAtomicWriteToFile(t *testing.T) { defer os.RemoveAll(tmpDir) expected := []byte("barbaz") - if err := AtomicWriteFile(filepath.Join(tmpDir, "foo"), expected, 0666); err != nil { + if err := AtomicWriteFile(filepath.Join(tmpDir, "foo"), expected, testMode); err != nil { t.Fatalf("Error writing to file: %v", err) } @@ -33,7 +45,88 @@ func TestAtomicWriteToFile(t *testing.T) { if err != nil { t.Fatalf("Error statting file: %v", err) } - if expected := os.FileMode(0666); st.Mode() != expected { + if expected := os.FileMode(testMode); st.Mode() != expected { + t.Fatalf("Mode mismatched, expected %o, got %o", expected, st.Mode()) + } +} + +func TestAtomicWriteSetCommit(t *testing.T) { + tmpDir, err := ioutil.TempDir("", "atomic-writerset-test") + if err != nil { + t.Fatalf("Error when creating temporary directory: %s", err) + } + defer os.RemoveAll(tmpDir) + + if err := os.Mkdir(filepath.Join(tmpDir, "tmp"), 0700); err != nil { + t.Fatalf("Error creating tmp directory: %s", err) + } + + targetDir := filepath.Join(tmpDir, "target") + ws, err := NewAtomicWriteSet(filepath.Join(tmpDir, "tmp")) + if err != nil { + t.Fatalf("Error creating atomic write set: %s", err) + } + + expected := []byte("barbaz") + if err := ws.WriteFile("foo", expected, testMode); err != nil { + t.Fatalf("Error writing to file: %v", err) + } + + if _, err := ioutil.ReadFile(filepath.Join(targetDir, "foo")); err == nil { + t.Fatalf("Expected error reading file where should not exist") + } + + if err := ws.Commit(targetDir); err != nil { + t.Fatalf("Error committing file: %s", err) + } + + actual, err := ioutil.ReadFile(filepath.Join(targetDir, "foo")) + if err != nil { + t.Fatalf("Error reading from file: %v", err) + } + + if bytes.Compare(actual, expected) != 0 { + t.Fatalf("Data mismatch, expected %q, got %q", expected, actual) + } + + st, err := os.Stat(filepath.Join(targetDir, "foo")) + if err != nil { + t.Fatalf("Error statting file: %v", err) + } + if expected := os.FileMode(testMode); st.Mode() != expected { t.Fatalf("Mode mismatched, expected %o, got %o", expected, st.Mode()) } + +} + +func TestAtomicWriteSetCancel(t *testing.T) { + tmpDir, err := ioutil.TempDir("", "atomic-writerset-test") + if err != nil { + t.Fatalf("Error when creating temporary directory: %s", err) + } + defer os.RemoveAll(tmpDir) + + if err := os.Mkdir(filepath.Join(tmpDir, "tmp"), 0700); err != nil { + t.Fatalf("Error creating tmp directory: %s", err) + } + + ws, err := NewAtomicWriteSet(filepath.Join(tmpDir, "tmp")) + if err != nil { + t.Fatalf("Error creating atomic write set: %s", err) + } + + expected := []byte("barbaz") + if err := ws.WriteFile("foo", expected, testMode); err != nil { + t.Fatalf("Error writing to file: %v", err) + } + + if err := ws.Cancel(); err != nil { + t.Fatalf("Error committing file: %s", err) + } + + if _, err := ioutil.ReadFile(filepath.Join(tmpDir, "target", "foo")); err == nil { + t.Fatalf("Expected error reading file where should not exist") + } else if !os.IsNotExist(err) { + t.Fatalf("Unexpected error reading file: %s", err) + } } From 68bbe0a2d9a39cf6487c515be2a477f06ff65472 Mon Sep 17 00:00:00 2001 From: unclejack Date: Thu, 30 Mar 2017 12:26:16 +0300 Subject: [PATCH 04/27] pkg/*: clean up a few issues Signed-off-by: Cristian Staretu --- pkg/ioutils/fswriters_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/ioutils/fswriters_test.go b/pkg/ioutils/fswriters_test.go index c4d14193..5d286005 100644 --- a/pkg/ioutils/fswriters_test.go +++ b/pkg/ioutils/fswriters_test.go @@ -37,7 +37,7 @@ func TestAtomicWriteToFile(t *testing.T) { t.Fatalf("Error reading from file: %v", err) } - if bytes.Compare(actual, expected) != 0 { + if !bytes.Equal(actual, expected) { t.Fatalf("Data mismatch, expected %q, got %q", expected, actual) } @@ -85,7 +85,7 @@ func TestAtomicWriteSetCommit(t *testing.T) { t.Fatalf("Error reading from file: %v", err) } - if bytes.Compare(actual, expected) != 0 { + if !bytes.Equal(actual, expected) { t.Fatalf("Data mismatch, expected %q, got %q", expected, actual) } From 600a2ab99aa87a74792521c210eb876cab6bbf16 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Mon, 5 Feb 2018 16:05:59 -0500 Subject: [PATCH 05/27] Add canonical import comment Signed-off-by: Daniel Nephin --- pkg/ioutils/fswriters.go | 2 +- pkg/ioutils/fswriters_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/ioutils/fswriters.go b/pkg/ioutils/fswriters.go index a56c4626..534d66ac 100644 --- a/pkg/ioutils/fswriters.go +++ b/pkg/ioutils/fswriters.go @@ -1,4 +1,4 @@ -package ioutils +package ioutils // import "github.com/docker/docker/pkg/ioutils" import ( "io" diff --git a/pkg/ioutils/fswriters_test.go b/pkg/ioutils/fswriters_test.go index 5d286005..b283045d 100644 --- a/pkg/ioutils/fswriters_test.go +++ b/pkg/ioutils/fswriters_test.go @@ -1,4 +1,4 @@ -package ioutils +package ioutils // import "github.com/docker/docker/pkg/ioutils" import ( "bytes" From 2c26a9eb6d553bad07a1d0735d8f4883e540e1eb Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Wed, 7 Aug 2019 02:38:51 +0200 Subject: [PATCH 06/27] unconvert: remove unnescessary conversions Signed-off-by: Kir Kolyshkin Signed-off-by: Sebastiaan van Stijn --- pkg/ioutils/fswriters_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/ioutils/fswriters_test.go b/pkg/ioutils/fswriters_test.go index b283045d..bce68c2f 100644 --- a/pkg/ioutils/fswriters_test.go +++ b/pkg/ioutils/fswriters_test.go @@ -45,7 +45,7 @@ func TestAtomicWriteToFile(t *testing.T) { if err != nil { t.Fatalf("Error statting file: %v", err) } - if expected := os.FileMode(testMode); st.Mode() != expected { + if expected := testMode; st.Mode() != expected { t.Fatalf("Mode mismatched, expected %o, got %o", expected, st.Mode()) } } @@ -93,7 +93,7 @@ func TestAtomicWriteSetCommit(t *testing.T) { if err != nil { t.Fatalf("Error statting file: %v", err) } - if expected := os.FileMode(testMode); st.Mode() != expected { + if expected := testMode; st.Mode() != expected { t.Fatalf("Mode mismatched, expected %o, got %o", expected, st.Mode()) } From 108c158e037ab7727039555f299b3666794f64ce Mon Sep 17 00:00:00 2001 From: Eng Zer Jun Date: Tue, 24 Aug 2021 18:10:50 +0800 Subject: [PATCH 07/27] refactor: move from io/ioutil to io and os package The io/ioutil package has been deprecated in Go 1.16. This commit replaces the existing io/ioutil functions with their new definitions in io and os packages. Signed-off-by: Eng Zer Jun --- pkg/ioutils/fswriters.go | 5 ++--- pkg/ioutils/fswriters_test.go | 15 +++++++-------- 2 files changed, 9 insertions(+), 11 deletions(-) diff --git a/pkg/ioutils/fswriters.go b/pkg/ioutils/fswriters.go index 534d66ac..82671d8c 100644 --- a/pkg/ioutils/fswriters.go +++ b/pkg/ioutils/fswriters.go @@ -2,7 +2,6 @@ package ioutils // import "github.com/docker/docker/pkg/ioutils" import ( "io" - "io/ioutil" "os" "path/filepath" ) @@ -11,7 +10,7 @@ import ( // temporary file and closing it atomically changes the temporary file to // destination path. Writing and closing concurrently is not allowed. func NewAtomicFileWriter(filename string, perm os.FileMode) (io.WriteCloser, error) { - f, err := ioutil.TempFile(filepath.Dir(filename), ".tmp-"+filepath.Base(filename)) + f, err := os.CreateTemp(filepath.Dir(filename), ".tmp-"+filepath.Base(filename)) if err != nil { return nil, err } @@ -94,7 +93,7 @@ type AtomicWriteSet struct { // commit. If no temporary directory is given the system // default is used. func NewAtomicWriteSet(tmpDir string) (*AtomicWriteSet, error) { - td, err := ioutil.TempDir(tmpDir, "write-set-") + td, err := os.MkdirTemp(tmpDir, "write-set-") if err != nil { return nil, err } diff --git a/pkg/ioutils/fswriters_test.go b/pkg/ioutils/fswriters_test.go index bce68c2f..d6355613 100644 --- a/pkg/ioutils/fswriters_test.go +++ b/pkg/ioutils/fswriters_test.go @@ -2,7 +2,6 @@ package ioutils // import "github.com/docker/docker/pkg/ioutils" import ( "bytes" - "io/ioutil" "os" "path/filepath" "runtime" @@ -21,7 +20,7 @@ func init() { } func TestAtomicWriteToFile(t *testing.T) { - tmpDir, err := ioutil.TempDir("", "atomic-writers-test") + tmpDir, err := os.MkdirTemp("", "atomic-writers-test") if err != nil { t.Fatalf("Error when creating temporary directory: %s", err) } @@ -32,7 +31,7 @@ func TestAtomicWriteToFile(t *testing.T) { t.Fatalf("Error writing to file: %v", err) } - actual, err := ioutil.ReadFile(filepath.Join(tmpDir, "foo")) + actual, err := os.ReadFile(filepath.Join(tmpDir, "foo")) if err != nil { t.Fatalf("Error reading from file: %v", err) } @@ -51,7 +50,7 @@ func TestAtomicWriteToFile(t *testing.T) { } func TestAtomicWriteSetCommit(t *testing.T) { - tmpDir, err := ioutil.TempDir("", "atomic-writerset-test") + tmpDir, err := os.MkdirTemp("", "atomic-writerset-test") if err != nil { t.Fatalf("Error when creating temporary directory: %s", err) } @@ -72,7 +71,7 @@ func TestAtomicWriteSetCommit(t *testing.T) { t.Fatalf("Error writing to file: %v", err) } - if _, err := ioutil.ReadFile(filepath.Join(targetDir, "foo")); err == nil { + if _, err := os.ReadFile(filepath.Join(targetDir, "foo")); err == nil { t.Fatalf("Expected error reading file where should not exist") } @@ -80,7 +79,7 @@ func TestAtomicWriteSetCommit(t *testing.T) { t.Fatalf("Error committing file: %s", err) } - actual, err := ioutil.ReadFile(filepath.Join(targetDir, "foo")) + actual, err := os.ReadFile(filepath.Join(targetDir, "foo")) if err != nil { t.Fatalf("Error reading from file: %v", err) } @@ -100,7 +99,7 @@ func TestAtomicWriteSetCommit(t *testing.T) { } func TestAtomicWriteSetCancel(t *testing.T) { - tmpDir, err := ioutil.TempDir("", "atomic-writerset-test") + tmpDir, err := os.MkdirTemp("", "atomic-writerset-test") if err != nil { t.Fatalf("Error when creating temporary directory: %s", err) } @@ -124,7 +123,7 @@ func TestAtomicWriteSetCancel(t *testing.T) { t.Fatalf("Error committing file: %s", err) } - if _, err := ioutil.ReadFile(filepath.Join(tmpDir, "target", "foo")); err == nil { + if _, err := os.ReadFile(filepath.Join(tmpDir, "target", "foo")); err == nil { t.Fatalf("Expected error reading file where should not exist") } else if !os.IsNotExist(err) { t.Fatalf("Unexpected error reading file: %s", err) From 98088d2594a8a0482ccd251ae06428b9a8499e13 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Fri, 23 Sep 2022 20:16:47 +0200 Subject: [PATCH 08/27] pkg/*: fix "empty-lines" (revive) pkg/directory/directory.go:9:49: empty-lines: extra empty line at the start of a block (revive) pkg/pubsub/publisher.go:8:48: empty-lines: extra empty line at the start of a block (revive) pkg/loopback/attach_loopback.go:96:69: empty-lines: extra empty line at the start of a block (revive) pkg/devicemapper/devmapper_wrapper.go:136:48: empty-lines: extra empty line at the start of a block (revive) pkg/devicemapper/devmapper.go:391:35: empty-lines: extra empty line at the end of a block (revive) pkg/devicemapper/devmapper.go:676:35: empty-lines: extra empty line at the end of a block (revive) pkg/archive/changes_posix_test.go:15:38: empty-lines: extra empty line at the end of a block (revive) pkg/devicemapper/devmapper.go:241:51: empty-lines: extra empty line at the start of a block (revive) pkg/fileutils/fileutils_test.go:17:47: empty-lines: extra empty line at the end of a block (revive) pkg/fileutils/fileutils_test.go:34:48: empty-lines: extra empty line at the end of a block (revive) pkg/fileutils/fileutils_test.go:318:32: empty-lines: extra empty line at the end of a block (revive) pkg/tailfile/tailfile.go:171:6: empty-lines: extra empty line at the end of a block (revive) pkg/tarsum/fileinfosums_test.go:16:41: empty-lines: extra empty line at the end of a block (revive) pkg/tarsum/tarsum_test.go:198:42: empty-lines: extra empty line at the start of a block (revive) pkg/tarsum/tarsum_test.go:294:25: empty-lines: extra empty line at the start of a block (revive) pkg/tarsum/tarsum_test.go:407:34: empty-lines: extra empty line at the end of a block (revive) pkg/ioutils/fswriters_test.go:52:45: empty-lines: extra empty line at the end of a block (revive) pkg/ioutils/writers_test.go:24:39: empty-lines: extra empty line at the end of a block (revive) pkg/ioutils/bytespipe_test.go:78:26: empty-lines: extra empty line at the end of a block (revive) pkg/sysinfo/sysinfo_linux_test.go:13:37: empty-lines: extra empty line at the end of a block (revive) pkg/archive/archive_linux_test.go:57:64: empty-lines: extra empty line at the end of a block (revive) pkg/archive/changes.go:248:72: empty-lines: extra empty line at the start of a block (revive) pkg/archive/changes_posix_test.go:15:38: empty-lines: extra empty line at the end of a block (revive) pkg/archive/copy.go:248:124: empty-lines: extra empty line at the end of a block (revive) pkg/archive/diff_test.go:198:44: empty-lines: extra empty line at the end of a block (revive) pkg/archive/archive.go:304:12: empty-lines: extra empty line at the end of a block (revive) pkg/archive/archive.go:749:37: empty-lines: extra empty line at the end of a block (revive) pkg/archive/archive.go:812:81: empty-lines: extra empty line at the start of a block (revive) pkg/archive/copy_unix_test.go:347:34: empty-lines: extra empty line at the end of a block (revive) pkg/system/path.go:11:39: empty-lines: extra empty line at the end of a block (revive) pkg/system/meminfo_linux.go:29:21: empty-lines: extra empty line at the end of a block (revive) pkg/plugins/plugins.go:135:32: empty-lines: extra empty line at the end of a block (revive) pkg/authorization/response.go:71:48: empty-lines: extra empty line at the start of a block (revive) pkg/authorization/api_test.go:18:51: empty-lines: extra empty line at the end of a block (revive) pkg/authorization/middleware_test.go:23:44: empty-lines: extra empty line at the end of a block (revive) pkg/authorization/middleware_unix_test.go:17:46: empty-lines: extra empty line at the end of a block (revive) pkg/authorization/api_test.go:57:45: empty-lines: extra empty line at the end of a block (revive) pkg/authorization/response.go:83:50: empty-lines: extra empty line at the start of a block (revive) pkg/authorization/api_test.go:66:47: empty-lines: extra empty line at the end of a block (revive) pkg/authorization/middleware_unix_test.go:45:48: empty-lines: extra empty line at the end of a block (revive) pkg/authorization/response.go:145:75: empty-lines: extra empty line at the start of a block (revive) pkg/authorization/middleware_unix_test.go:56:51: empty-lines: extra empty line at the end of a block (revive) Signed-off-by: Sebastiaan van Stijn --- pkg/ioutils/fswriters_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/pkg/ioutils/fswriters_test.go b/pkg/ioutils/fswriters_test.go index d6355613..3cc7bbfc 100644 --- a/pkg/ioutils/fswriters_test.go +++ b/pkg/ioutils/fswriters_test.go @@ -95,7 +95,6 @@ func TestAtomicWriteSetCommit(t *testing.T) { if expected := testMode; st.Mode() != expected { t.Fatalf("Mode mismatched, expected %o, got %o", expected, st.Mode()) } - } func TestAtomicWriteSetCancel(t *testing.T) { From f6bd355f4b630549e5ac0109edff03c633ebe34a Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Thu, 20 Jan 2022 14:15:45 +0100 Subject: [PATCH 09/27] pkg/ioutils: format code with gofumpt Formatting the code with https://github.com/mvdan/gofumpt Signed-off-by: Sebastiaan van Stijn --- pkg/ioutils/fswriters_test.go | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/pkg/ioutils/fswriters_test.go b/pkg/ioutils/fswriters_test.go index 3cc7bbfc..ec1a72c9 100644 --- a/pkg/ioutils/fswriters_test.go +++ b/pkg/ioutils/fswriters_test.go @@ -8,14 +8,12 @@ import ( "testing" ) -var ( - testMode os.FileMode = 0640 -) +var testMode os.FileMode = 0o640 func init() { // Windows does not support full Linux file mode if runtime.GOOS == "windows" { - testMode = 0666 + testMode = 0o666 } } @@ -56,7 +54,7 @@ func TestAtomicWriteSetCommit(t *testing.T) { } defer os.RemoveAll(tmpDir) - if err := os.Mkdir(filepath.Join(tmpDir, "tmp"), 0700); err != nil { + if err := os.Mkdir(filepath.Join(tmpDir, "tmp"), 0o700); err != nil { t.Fatalf("Error creating tmp directory: %s", err) } @@ -104,7 +102,7 @@ func TestAtomicWriteSetCancel(t *testing.T) { } defer os.RemoveAll(tmpDir) - if err := os.Mkdir(filepath.Join(tmpDir, "tmp"), 0700); err != nil { + if err := os.Mkdir(filepath.Join(tmpDir, "tmp"), 0o700); err != nil { t.Fatalf("Error creating tmp directory: %s", err) } From 5aa2f9dea04325baf9c7a0ebebbc9c9313c52dc5 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Thu, 13 Jul 2023 00:26:11 +0200 Subject: [PATCH 10/27] pkg/ioutils: some cleanups in tests - remove gotest.tools dependency as it was only used in one test, and only for a trivial check - use t.TempDir() - rename vars that collided with package types - don't use un-keyed structs - explicitly ignore some errors to please linters - use iotest.ErrReader - TestReadCloserWrapperClose: verify reading works before closing :) Signed-off-by: Sebastiaan van Stijn --- pkg/ioutils/fswriters_test.go | 18 +++--------------- 1 file changed, 3 insertions(+), 15 deletions(-) diff --git a/pkg/ioutils/fswriters_test.go b/pkg/ioutils/fswriters_test.go index ec1a72c9..31232f0e 100644 --- a/pkg/ioutils/fswriters_test.go +++ b/pkg/ioutils/fswriters_test.go @@ -18,11 +18,7 @@ func init() { } func TestAtomicWriteToFile(t *testing.T) { - tmpDir, err := os.MkdirTemp("", "atomic-writers-test") - if err != nil { - t.Fatalf("Error when creating temporary directory: %s", err) - } - defer os.RemoveAll(tmpDir) + tmpDir := t.TempDir() expected := []byte("barbaz") if err := AtomicWriteFile(filepath.Join(tmpDir, "foo"), expected, testMode); err != nil { @@ -48,11 +44,7 @@ func TestAtomicWriteToFile(t *testing.T) { } func TestAtomicWriteSetCommit(t *testing.T) { - tmpDir, err := os.MkdirTemp("", "atomic-writerset-test") - if err != nil { - t.Fatalf("Error when creating temporary directory: %s", err) - } - defer os.RemoveAll(tmpDir) + tmpDir := t.TempDir() if err := os.Mkdir(filepath.Join(tmpDir, "tmp"), 0o700); err != nil { t.Fatalf("Error creating tmp directory: %s", err) @@ -96,11 +88,7 @@ func TestAtomicWriteSetCommit(t *testing.T) { } func TestAtomicWriteSetCancel(t *testing.T) { - tmpDir, err := os.MkdirTemp("", "atomic-writerset-test") - if err != nil { - t.Fatalf("Error when creating temporary directory: %s", err) - } - defer os.RemoveAll(tmpDir) + tmpDir := t.TempDir() if err := os.Mkdir(filepath.Join(tmpDir, "tmp"), 0o700); err != nil { t.Fatalf("Error creating tmp directory: %s", err) From d4f48bc9460da3f2133249f3291ea5ad27542b95 Mon Sep 17 00:00:00 2001 From: Antonio Aguilar Date: Tue, 2 Apr 2024 23:07:29 -0600 Subject: [PATCH 11/27] Update GoDoc for ioutils on atomic writers Unlike its stdlib counterparts, AtomicFileWriter does not take into consideration umask due to its use of chmod. Failure to recognize this may cause subtle problems like the one described in #47498. Therefore the documentation has been updated to let users know that umask is not taken into consideration when using AtomicFileWriter. Closes #47516. Signed-off-by: Antonio Aguilar --- pkg/ioutils/fswriters.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/pkg/ioutils/fswriters.go b/pkg/ioutils/fswriters.go index 82671d8c..05da97b0 100644 --- a/pkg/ioutils/fswriters.go +++ b/pkg/ioutils/fswriters.go @@ -9,6 +9,7 @@ import ( // NewAtomicFileWriter returns WriteCloser so that writing to it writes to a // temporary file and closing it atomically changes the temporary file to // destination path. Writing and closing concurrently is not allowed. +// NOTE: umask is not considered for the file's permissions. func NewAtomicFileWriter(filename string, perm os.FileMode) (io.WriteCloser, error) { f, err := os.CreateTemp(filepath.Dir(filename), ".tmp-"+filepath.Base(filename)) if err != nil { @@ -26,7 +27,8 @@ func NewAtomicFileWriter(filename string, perm os.FileMode) (io.WriteCloser, err }, nil } -// AtomicWriteFile atomically writes data to a file named by filename. +// AtomicWriteFile atomically writes data to a file named by filename and with the specified permission bits. +// NOTE: umask is not considered for the file's permissions. func AtomicWriteFile(filename string, data []byte, perm os.FileMode) error { f, err := NewAtomicFileWriter(filename, perm) if err != nil { From 26657e1ce77520193c3ea59af7e8d14272ba75f9 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sat, 28 Dec 2024 17:06:23 +0100 Subject: [PATCH 12/27] pkg/ioutils: move atomic file-writers to a separate (pkg/atomicwriter) package Signed-off-by: Sebastiaan van Stijn --- .../atomicwriter.go | 32 +++++++++---------- .../atomicwriter_test.go | 8 ++--- 2 files changed, 20 insertions(+), 20 deletions(-) rename pkg/ioutils/fswriters.go => atomicwriter/atomicwriter.go (74%) rename pkg/ioutils/fswriters_test.go => atomicwriter/atomicwriter_test.go (90%) diff --git a/pkg/ioutils/fswriters.go b/atomicwriter/atomicwriter.go similarity index 74% rename from pkg/ioutils/fswriters.go rename to atomicwriter/atomicwriter.go index 05da97b0..cbbe835b 100644 --- a/pkg/ioutils/fswriters.go +++ b/atomicwriter/atomicwriter.go @@ -1,4 +1,4 @@ -package ioutils // import "github.com/docker/docker/pkg/ioutils" +package atomicwriter import ( "io" @@ -6,11 +6,11 @@ import ( "path/filepath" ) -// NewAtomicFileWriter returns WriteCloser so that writing to it writes to a +// New returns a WriteCloser so that writing to it writes to a // temporary file and closing it atomically changes the temporary file to // destination path. Writing and closing concurrently is not allowed. // NOTE: umask is not considered for the file's permissions. -func NewAtomicFileWriter(filename string, perm os.FileMode) (io.WriteCloser, error) { +func New(filename string, perm os.FileMode) (io.WriteCloser, error) { f, err := os.CreateTemp(filepath.Dir(filename), ".tmp-"+filepath.Base(filename)) if err != nil { return nil, err @@ -27,10 +27,10 @@ func NewAtomicFileWriter(filename string, perm os.FileMode) (io.WriteCloser, err }, nil } -// AtomicWriteFile atomically writes data to a file named by filename and with the specified permission bits. +// WriteFile atomically writes data to a file named by filename and with the specified permission bits. // NOTE: umask is not considered for the file's permissions. -func AtomicWriteFile(filename string, data []byte, perm os.FileMode) error { - f, err := NewAtomicFileWriter(filename, perm) +func WriteFile(filename string, data []byte, perm os.FileMode) error { + f, err := New(filename, perm) if err != nil { return err } @@ -82,32 +82,32 @@ func (w *atomicFileWriter) Close() (retErr error) { return nil } -// AtomicWriteSet is used to atomically write a set +// WriteSet is used to atomically write a set // of files and ensure they are visible at the same time. // Must be committed to a new directory. -type AtomicWriteSet struct { +type WriteSet struct { root string } -// NewAtomicWriteSet creates a new atomic write set to +// NewWriteSet creates a new atomic write set to // atomically create a set of files. The given directory // is used as the base directory for storing files before // commit. If no temporary directory is given the system // default is used. -func NewAtomicWriteSet(tmpDir string) (*AtomicWriteSet, error) { +func NewWriteSet(tmpDir string) (*WriteSet, error) { td, err := os.MkdirTemp(tmpDir, "write-set-") if err != nil { return nil, err } - return &AtomicWriteSet{ + return &WriteSet{ root: td, }, nil } // WriteFile writes a file to the set, guaranteeing the file // has been synced. -func (ws *AtomicWriteSet) WriteFile(filename string, data []byte, perm os.FileMode) error { +func (ws *WriteSet) WriteFile(filename string, data []byte, perm os.FileMode) error { f, err := ws.FileWriter(filename, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, perm) if err != nil { return err @@ -136,7 +136,7 @@ func (w syncFileCloser) Close() error { // FileWriter opens a file writer inside the set. The file // should be synced and closed before calling commit. -func (ws *AtomicWriteSet) FileWriter(name string, flag int, perm os.FileMode) (io.WriteCloser, error) { +func (ws *WriteSet) FileWriter(name string, flag int, perm os.FileMode) (io.WriteCloser, error) { f, err := os.OpenFile(filepath.Join(ws.root, name), flag, perm) if err != nil { return nil, err @@ -146,18 +146,18 @@ func (ws *AtomicWriteSet) FileWriter(name string, flag int, perm os.FileMode) (i // Cancel cancels the set and removes all temporary data // created in the set. -func (ws *AtomicWriteSet) Cancel() error { +func (ws *WriteSet) Cancel() error { return os.RemoveAll(ws.root) } // Commit moves all created files to the target directory. The // target directory must not exist and the parent of the target // directory must exist. -func (ws *AtomicWriteSet) Commit(target string) error { +func (ws *WriteSet) Commit(target string) error { return os.Rename(ws.root, target) } // String returns the location the set is writing to. -func (ws *AtomicWriteSet) String() string { +func (ws *WriteSet) String() string { return ws.root } diff --git a/pkg/ioutils/fswriters_test.go b/atomicwriter/atomicwriter_test.go similarity index 90% rename from pkg/ioutils/fswriters_test.go rename to atomicwriter/atomicwriter_test.go index 31232f0e..3a2f48f9 100644 --- a/pkg/ioutils/fswriters_test.go +++ b/atomicwriter/atomicwriter_test.go @@ -1,4 +1,4 @@ -package ioutils // import "github.com/docker/docker/pkg/ioutils" +package atomicwriter import ( "bytes" @@ -21,7 +21,7 @@ func TestAtomicWriteToFile(t *testing.T) { tmpDir := t.TempDir() expected := []byte("barbaz") - if err := AtomicWriteFile(filepath.Join(tmpDir, "foo"), expected, testMode); err != nil { + if err := WriteFile(filepath.Join(tmpDir, "foo"), expected, testMode); err != nil { t.Fatalf("Error writing to file: %v", err) } @@ -51,7 +51,7 @@ func TestAtomicWriteSetCommit(t *testing.T) { } targetDir := filepath.Join(tmpDir, "target") - ws, err := NewAtomicWriteSet(filepath.Join(tmpDir, "tmp")) + ws, err := NewWriteSet(filepath.Join(tmpDir, "tmp")) if err != nil { t.Fatalf("Error creating atomic write set: %s", err) } @@ -94,7 +94,7 @@ func TestAtomicWriteSetCancel(t *testing.T) { t.Fatalf("Error creating tmp directory: %s", err) } - ws, err := NewAtomicWriteSet(filepath.Join(tmpDir, "tmp")) + ws, err := NewWriteSet(filepath.Join(tmpDir, "tmp")) if err != nil { t.Fatalf("Error creating atomic write set: %s", err) } From 37e39917bf8e760b85fc843c5756003ec494b130 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sun, 9 Mar 2025 14:55:11 +0100 Subject: [PATCH 13/27] pkg/atomicwriter: New(): prevent creating temp-file on errors The temp-file was created before trying to make the given filename an absolute path. Reverse the order of code so that we don't create a temp-file if an error happens. Signed-off-by: Sebastiaan van Stijn --- atomicwriter/atomicwriter.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/atomicwriter/atomicwriter.go b/atomicwriter/atomicwriter.go index cbbe835b..d15ce3f0 100644 --- a/atomicwriter/atomicwriter.go +++ b/atomicwriter/atomicwriter.go @@ -11,12 +11,12 @@ import ( // destination path. Writing and closing concurrently is not allowed. // NOTE: umask is not considered for the file's permissions. func New(filename string, perm os.FileMode) (io.WriteCloser, error) { - f, err := os.CreateTemp(filepath.Dir(filename), ".tmp-"+filepath.Base(filename)) + abspath, err := filepath.Abs(filename) if err != nil { return nil, err } - abspath, err := filepath.Abs(filename) + f, err := os.CreateTemp(filepath.Dir(filename), ".tmp-"+filepath.Base(filename)) if err != nil { return nil, err } From adaf3d20d59dc88f7fd7724a2aebf244e0004984 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sun, 9 Mar 2025 15:00:00 +0100 Subject: [PATCH 14/27] pkg/atomicwriter: New(): use absolute path for temp-file Use an absolute path for both the temp-file and the destination-file. Signed-off-by: Sebastiaan van Stijn --- atomicwriter/atomicwriter.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/atomicwriter/atomicwriter.go b/atomicwriter/atomicwriter.go index d15ce3f0..abf46275 100644 --- a/atomicwriter/atomicwriter.go +++ b/atomicwriter/atomicwriter.go @@ -16,7 +16,7 @@ func New(filename string, perm os.FileMode) (io.WriteCloser, error) { return nil, err } - f, err := os.CreateTemp(filepath.Dir(filename), ".tmp-"+filepath.Base(filename)) + f, err := os.CreateTemp(filepath.Dir(abspath), ".tmp-"+filepath.Base(filename)) if err != nil { return nil, err } From d09d225de534c9b5495df994741bb95a8af6b92a Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Mon, 10 Mar 2025 08:44:57 +0100 Subject: [PATCH 15/27] pkg/atomicwriter: refactor tests - rename tests to match the function tested - remove init func in favor or a test-helper - rename some vars to prevent shadowing - update example values to be more descriptive - add a utility for asserting file content and mode Signed-off-by: Sebastiaan van Stijn --- atomicwriter/atomicwriter_test.go | 81 +++++++++++++++---------------- 1 file changed, 39 insertions(+), 42 deletions(-) diff --git a/atomicwriter/atomicwriter_test.go b/atomicwriter/atomicwriter_test.go index 3a2f48f9..a4702ba5 100644 --- a/atomicwriter/atomicwriter_test.go +++ b/atomicwriter/atomicwriter_test.go @@ -2,48 +2,57 @@ package atomicwriter import ( "bytes" + "errors" "os" "path/filepath" "runtime" "testing" ) -var testMode os.FileMode = 0o640 - -func init() { - // Windows does not support full Linux file mode +// testMode returns the file-mode to use in tests, accounting for Windows +// not supporting full Linux file mode. +func testMode() os.FileMode { if runtime.GOOS == "windows" { - testMode = 0o666 + return 0o666 } + return 0o640 } -func TestAtomicWriteToFile(t *testing.T) { - tmpDir := t.TempDir() - - expected := []byte("barbaz") - if err := WriteFile(filepath.Join(tmpDir, "foo"), expected, testMode); err != nil { - t.Fatalf("Error writing to file: %v", err) - } - - actual, err := os.ReadFile(filepath.Join(tmpDir, "foo")) +// assertFile asserts the given fileName to exist, and to have the expected +// content and mode. +func assertFile(t *testing.T, fileName string, fileContent []byte, expectedMode os.FileMode) { + t.Helper() + actual, err := os.ReadFile(fileName) if err != nil { t.Fatalf("Error reading from file: %v", err) } - if !bytes.Equal(actual, expected) { - t.Fatalf("Data mismatch, expected %q, got %q", expected, actual) + if !bytes.Equal(actual, fileContent) { + t.Errorf("Data mismatch, expected %q, got %q", fileContent, actual) } - st, err := os.Stat(filepath.Join(tmpDir, "foo")) + st, err := os.Stat(fileName) if err != nil { t.Fatalf("Error statting file: %v", err) } - if expected := testMode; st.Mode() != expected { - t.Fatalf("Mode mismatched, expected %o, got %o", expected, st.Mode()) + if st.Mode() != expectedMode { + t.Errorf("Mode mismatched, expected %o, got %o", expectedMode, st.Mode()) + } +} + +func TestWriteFile(t *testing.T) { + tmpDir := t.TempDir() + + fileName := filepath.Join(tmpDir, "test.txt") + fileContent := []byte("file content") + fileMode := testMode() + if err := WriteFile(fileName, fileContent, fileMode); err != nil { + t.Fatalf("Error writing to file: %v", err) } + assertFile(t, fileName, fileContent, fileMode) } -func TestAtomicWriteSetCommit(t *testing.T) { +func TestWriteSetCommit(t *testing.T) { tmpDir := t.TempDir() if err := os.Mkdir(filepath.Join(tmpDir, "tmp"), 0o700); err != nil { @@ -56,8 +65,10 @@ func TestAtomicWriteSetCommit(t *testing.T) { t.Fatalf("Error creating atomic write set: %s", err) } - expected := []byte("barbaz") - if err := ws.WriteFile("foo", expected, testMode); err != nil { + fileContent := []byte("file content") + fileMode := testMode() + + if err := ws.WriteFile("foo", fileContent, fileMode); err != nil { t.Fatalf("Error writing to file: %v", err) } @@ -69,25 +80,10 @@ func TestAtomicWriteSetCommit(t *testing.T) { t.Fatalf("Error committing file: %s", err) } - actual, err := os.ReadFile(filepath.Join(targetDir, "foo")) - if err != nil { - t.Fatalf("Error reading from file: %v", err) - } - - if !bytes.Equal(actual, expected) { - t.Fatalf("Data mismatch, expected %q, got %q", expected, actual) - } - - st, err := os.Stat(filepath.Join(targetDir, "foo")) - if err != nil { - t.Fatalf("Error statting file: %v", err) - } - if expected := testMode; st.Mode() != expected { - t.Fatalf("Mode mismatched, expected %o, got %o", expected, st.Mode()) - } + assertFile(t, filepath.Join(targetDir, "foo"), fileContent, fileMode) } -func TestAtomicWriteSetCancel(t *testing.T) { +func TestWriteSetCancel(t *testing.T) { tmpDir := t.TempDir() if err := os.Mkdir(filepath.Join(tmpDir, "tmp"), 0o700); err != nil { @@ -99,8 +95,9 @@ func TestAtomicWriteSetCancel(t *testing.T) { t.Fatalf("Error creating atomic write set: %s", err) } - expected := []byte("barbaz") - if err := ws.WriteFile("foo", expected, testMode); err != nil { + fileContent := []byte("file content") + fileMode := testMode() + if err := ws.WriteFile("foo", fileContent, fileMode); err != nil { t.Fatalf("Error writing to file: %v", err) } @@ -110,7 +107,7 @@ func TestAtomicWriteSetCancel(t *testing.T) { if _, err := os.ReadFile(filepath.Join(tmpDir, "target", "foo")); err == nil { t.Fatalf("Expected error reading file where should not exist") - } else if !os.IsNotExist(err) { + } else if !errors.Is(err, os.ErrNotExist) { t.Fatalf("Unexpected error reading file: %s", err) } } From d27bf51ab6e20d7e47c59a0eb165aeb835cf5bd2 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Mon, 10 Mar 2025 10:37:17 +0100 Subject: [PATCH 16/27] pkg/atomicwriter: add separate tests for New() We were testing this function implicitly through `TestWriteFile`, but not verifying the behavior of `New` in isolation. Add separate tests for this function. Signed-off-by: Sebastiaan van Stijn --- atomicwriter/atomicwriter_test.go | 76 +++++++++++++++++++++++++++++++ 1 file changed, 76 insertions(+) diff --git a/atomicwriter/atomicwriter_test.go b/atomicwriter/atomicwriter_test.go index a4702ba5..8cc165b9 100644 --- a/atomicwriter/atomicwriter_test.go +++ b/atomicwriter/atomicwriter_test.go @@ -6,6 +6,7 @@ import ( "os" "path/filepath" "runtime" + "strings" "testing" ) @@ -40,6 +41,81 @@ func assertFile(t *testing.T, fileName string, fileContent []byte, expectedMode } } +// assertFileCount asserts the given directory has the expected number +// of files, and returns the list of files found. +func assertFileCount(t *testing.T, directory string, expected int) []os.DirEntry { + t.Helper() + files, err := os.ReadDir(directory) + if err != nil { + t.Fatalf("Error reading dir: %v", err) + } + if len(files) != expected { + t.Errorf("Expected %d files, got %d: %v", expected, len(files), files) + } + return files +} + +func TestNew(t *testing.T) { + for _, tc := range []string{"normal", "symlinked"} { + tmpDir := t.TempDir() + parentDir := tmpDir + actualParentDir := parentDir + if tc == "symlinked" { + actualParentDir = filepath.Join(tmpDir, "parent-dir") + if err := os.Mkdir(actualParentDir, 0o700); err != nil { + t.Fatal(err) + } + parentDir = filepath.Join(tmpDir, "parent-dir-symlink") + if err := os.Symlink(actualParentDir, parentDir); err != nil { + t.Fatal(err) + } + } + t.Run(tc, func(t *testing.T) { + for _, tc := range []string{"new-file", "existing-file"} { + t.Run(tc, func(t *testing.T) { + fileName := filepath.Join(parentDir, "test.txt") + var origFileCount int + if tc == "existing-file" { + if err := os.WriteFile(fileName, []byte("original content"), testMode()); err != nil { + t.Fatalf("Error writing file: %v", err) + } + origFileCount = 1 + } + writer, err := New(fileName, testMode()) + if writer == nil { + t.Errorf("Writer is nil") + } + if err != nil { + t.Fatalf("Error creating new atomicwriter: %v", err) + } + files := assertFileCount(t, actualParentDir, origFileCount+1) + if tmpFileName := files[0].Name(); !strings.HasPrefix(tmpFileName, ".tmp-test.txt") { + t.Errorf("Unexpected file name for temp-file: %s", tmpFileName) + } + + if err = writer.Close(); err != nil { + t.Errorf("Error closing writer: %v", err) + } + }) + } + }) + } +} + +func TestNewInvalid(t *testing.T) { + t.Run("missing target dir", func(t *testing.T) { + tmpDir := t.TempDir() + fileName := filepath.Join(tmpDir, "missing-dir", "test.txt") + writer, err := New(fileName, testMode()) + if writer != nil { + t.Errorf("Should not have created writer") + } + if !errors.Is(err, os.ErrNotExist) { + t.Errorf("Should produce a 'not found' error, but got %[1]T (%[1]v)", err) + } + }) +} + func TestWriteFile(t *testing.T) { tmpDir := t.TempDir() From d0c17c2b37cf174467782cffe7e868817859aa1d Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Mon, 10 Mar 2025 10:44:39 +0100 Subject: [PATCH 17/27] pkg/atomicwriter: don't overwrite destination on close without write Creating a writer (`atomicwriter.New()`) and closing it without a write ever happening, would replace the destination file with an empty file. This patch adds a check whether a write was performed (either successful or unsuccessful); if no write happened, we cleanup the tempfile without replacing the destination file. Signed-off-by: Sebastiaan van Stijn --- atomicwriter/atomicwriter.go | 11 +++++++---- atomicwriter/atomicwriter_test.go | 6 ++++++ 2 files changed, 13 insertions(+), 4 deletions(-) diff --git a/atomicwriter/atomicwriter.go b/atomicwriter/atomicwriter.go index abf46275..c2d95d28 100644 --- a/atomicwriter/atomicwriter.go +++ b/atomicwriter/atomicwriter.go @@ -1,6 +1,7 @@ package atomicwriter import ( + "errors" "io" "os" "path/filepath" @@ -49,10 +50,12 @@ type atomicFileWriter struct { f *os.File fn string writeErr error + written bool perm os.FileMode } func (w *atomicFileWriter) Write(dt []byte) (int, error) { + w.written = true n, err := w.f.Write(dt) if err != nil { w.writeErr = err @@ -62,12 +65,12 @@ func (w *atomicFileWriter) Write(dt []byte) (int, error) { func (w *atomicFileWriter) Close() (retErr error) { defer func() { - if retErr != nil || w.writeErr != nil { - os.Remove(w.f.Name()) + if err := os.Remove(w.f.Name()); !errors.Is(err, os.ErrNotExist) && retErr == nil { + retErr = err } }() if err := w.f.Sync(); err != nil { - w.f.Close() + _ = w.f.Close() return err } if err := w.f.Close(); err != nil { @@ -76,7 +79,7 @@ func (w *atomicFileWriter) Close() (retErr error) { if err := os.Chmod(w.f.Name(), w.perm); err != nil { return err } - if w.writeErr == nil { + if w.writeErr == nil && w.written { return os.Rename(w.f.Name(), w.fn) } return nil diff --git a/atomicwriter/atomicwriter_test.go b/atomicwriter/atomicwriter_test.go index 8cc165b9..de314b2e 100644 --- a/atomicwriter/atomicwriter_test.go +++ b/atomicwriter/atomicwriter_test.go @@ -93,9 +93,15 @@ func TestNew(t *testing.T) { t.Errorf("Unexpected file name for temp-file: %s", tmpFileName) } + // Closing the writer without writing should clean up the temp-file, + // and should not replace the destination file. if err = writer.Close(); err != nil { t.Errorf("Error closing writer: %v", err) } + assertFileCount(t, actualParentDir, origFileCount) + if tc == "existing-file" { + assertFile(t, fileName, []byte("original content"), testMode()) + } }) } }) From 2da7e4fc83993cd7556b49fa1c46ad98f0cefd50 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Mon, 10 Mar 2025 12:49:31 +0100 Subject: [PATCH 18/27] pkg/atomicwriter: add additional test-cases - test errors returned for non-existing destination - test that files are cleaned up after - test writing to a symlinked file (to be fixed) Signed-off-by: Sebastiaan van Stijn --- atomicwriter/atomicwriter_test.go | 80 +++++++++++++++++++++++++++---- 1 file changed, 72 insertions(+), 8 deletions(-) diff --git a/atomicwriter/atomicwriter_test.go b/atomicwriter/atomicwriter_test.go index de314b2e..3a841d9a 100644 --- a/atomicwriter/atomicwriter_test.go +++ b/atomicwriter/atomicwriter_test.go @@ -123,15 +123,77 @@ func TestNewInvalid(t *testing.T) { } func TestWriteFile(t *testing.T) { - tmpDir := t.TempDir() + t.Run("write to file", func(t *testing.T) { + tmpDir := t.TempDir() + fileName := filepath.Join(tmpDir, "test.txt") + fileContent := []byte("file content") + fileMode := testMode() + if err := WriteFile(fileName, fileContent, fileMode); err != nil { + t.Fatalf("Error writing to file: %v", err) + } + assertFile(t, fileName, fileContent, fileMode) + assertFileCount(t, tmpDir, 1) + }) + t.Run("missing parent directory", func(t *testing.T) { + tmpDir := t.TempDir() + fileName := filepath.Join(tmpDir, "missing-dir", "test.txt") + fileContent := []byte("file content") + fileMode := testMode() + if err := WriteFile(fileName, fileContent, fileMode); !errors.Is(err, os.ErrNotExist) { + t.Errorf("Should produce a 'not found' error, but got %[1]T (%[1]v)", err) + } + assertFileCount(t, tmpDir, 0) + }) + t.Run("symlinked file", func(t *testing.T) { + tmpDir := t.TempDir() + linkTarget := filepath.Join(tmpDir, "symlink-target") + if err := os.WriteFile(linkTarget, []byte("orig content"), testMode()); err != nil { + t.Fatal(err) + } + if err := os.Symlink(linkTarget, filepath.Join(tmpDir, "symlinked-file")); err != nil { + t.Fatal(err) + } + origFileCount := 2 + assertFileCount(t, tmpDir, origFileCount) - fileName := filepath.Join(tmpDir, "test.txt") - fileContent := []byte("file content") - fileMode := testMode() - if err := WriteFile(fileName, fileContent, fileMode); err != nil { - t.Fatalf("Error writing to file: %v", err) - } - assertFile(t, fileName, fileContent, fileMode) + fileName := filepath.Join(tmpDir, "symlinked-file") + fileContent := []byte("new content") + fileMode := testMode() + if err := WriteFile(fileName, fileContent, fileMode); err != nil { + t.Fatalf("Error writing to file: %v", err) + } + assertFile(t, fileName, fileContent, fileMode) + assertFileCount(t, tmpDir, origFileCount) + // FIXME(thaJeztah): [os.Rename] does not resolve symlinks, so writing to a symlinked location replaces the link with a file. + // assertFile(t, linkTarget, fileContent, fileMode) + }) + t.Run("symlinked directory", func(t *testing.T) { + tmpDir := t.TempDir() + actualParentDir := filepath.Join(tmpDir, "parent-dir") + if err := os.Mkdir(actualParentDir, 0o700); err != nil { + t.Fatal(err) + } + actualTargetFile := filepath.Join(actualParentDir, "target-file") + if err := os.WriteFile(actualTargetFile, []byte("orig content"), testMode()); err != nil { + t.Fatal(err) + } + parentDir := filepath.Join(tmpDir, "parent-dir-symlink") + if err := os.Symlink(actualParentDir, parentDir); err != nil { + t.Fatal(err) + } + origFileCount := 1 + assertFileCount(t, actualParentDir, origFileCount) + + fileName := filepath.Join(parentDir, "target-file") + fileContent := []byte("new content") + fileMode := testMode() + if err := WriteFile(fileName, fileContent, fileMode); err != nil { + t.Fatalf("Error writing to file: %v", err) + } + assertFile(t, fileName, fileContent, fileMode) + assertFile(t, actualTargetFile, fileContent, fileMode) + assertFileCount(t, actualParentDir, origFileCount) + }) } func TestWriteSetCommit(t *testing.T) { @@ -163,6 +225,7 @@ func TestWriteSetCommit(t *testing.T) { } assertFile(t, filepath.Join(targetDir, "foo"), fileContent, fileMode) + assertFileCount(t, targetDir, 1) } func TestWriteSetCancel(t *testing.T) { @@ -192,4 +255,5 @@ func TestWriteSetCancel(t *testing.T) { } else if !errors.Is(err, os.ErrNotExist) { t.Fatalf("Unexpected error reading file: %s", err) } + assertFileCount(t, filepath.Join(tmpDir, "tmp"), 0) } From 306cc18fb2fa3900425619f41b4eccb51bfd87a2 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Mon, 10 Mar 2025 15:05:33 +0100 Subject: [PATCH 19/27] pkg/atomicwriter: validate destination path - Disallow empty filenames - Don't allow writing to a directory - Return early if parent dir doesn't exist - TBD: do we want to allow symlinks to be followed, or disallow? Signed-off-by: Sebastiaan van Stijn --- atomicwriter/atomicwriter.go | 58 +++++++++++++++++++++++++++++++ atomicwriter/atomicwriter_test.go | 31 +++++++++++++++++ 2 files changed, 89 insertions(+) diff --git a/atomicwriter/atomicwriter.go b/atomicwriter/atomicwriter.go index c2d95d28..e8aa7807 100644 --- a/atomicwriter/atomicwriter.go +++ b/atomicwriter/atomicwriter.go @@ -2,16 +2,74 @@ package atomicwriter import ( "errors" + "fmt" "io" "os" "path/filepath" ) +func validateDestination(fileName string) error { + if fileName == "" { + return errors.New("file name is empty") + } + + // Deliberately using Lstat here to match the behavior of [os.Rename], + // which is used when completing the write and does not resolve symlinks. + // + // TODO(thaJeztah): decide whether we want to disallow symlinks or to follow them. + if fi, err := os.Lstat(fileName); err != nil { + if !os.IsNotExist(err) { + return fmt.Errorf("failed to stat output path: %w", err) + } + } else if err := validateFileMode(fi.Mode()); err != nil { + return err + } + if dir := filepath.Dir(fileName); dir != "" && dir != "." { + if _, err := os.Stat(dir); errors.Is(err, os.ErrNotExist) { + return fmt.Errorf("invalid file path: %w", err) + } + } + return nil +} + +func validateFileMode(mode os.FileMode) error { + switch { + case mode.IsRegular(): + return nil // Regular file + case mode&os.ModeDir != 0: + return errors.New("cannot write to a directory") + // TODO(thaJeztah): decide whether we want to disallow symlinks or to follow them. + // case mode&os.ModeSymlink != 0: + // return errors.New("cannot write to a symbolic link directly") + case mode&os.ModeNamedPipe != 0: + return errors.New("cannot write to a named pipe (FIFO)") + case mode&os.ModeSocket != 0: + return errors.New("cannot write to a socket") + case mode&os.ModeDevice != 0: + if mode&os.ModeCharDevice != 0 { + return errors.New("cannot write to a character device file") + } + return errors.New("cannot write to a block device file") + case mode&os.ModeSetuid != 0: + return errors.New("cannot write to a setuid file") + case mode&os.ModeSetgid != 0: + return errors.New("cannot write to a setgid file") + case mode&os.ModeSticky != 0: + return errors.New("cannot write to a sticky bit file") + default: + // Unknown file mode; let's assume it works + return nil + } +} + // New returns a WriteCloser so that writing to it writes to a // temporary file and closing it atomically changes the temporary file to // destination path. Writing and closing concurrently is not allowed. // NOTE: umask is not considered for the file's permissions. func New(filename string, perm os.FileMode) (io.WriteCloser, error) { + if err := validateDestination(filename); err != nil { + return nil, err + } abspath, err := filepath.Abs(filename) if err != nil { return nil, err diff --git a/atomicwriter/atomicwriter_test.go b/atomicwriter/atomicwriter_test.go index 3a841d9a..9892489f 100644 --- a/atomicwriter/atomicwriter_test.go +++ b/atomicwriter/atomicwriter_test.go @@ -120,9 +120,40 @@ func TestNewInvalid(t *testing.T) { t.Errorf("Should produce a 'not found' error, but got %[1]T (%[1]v)", err) } }) + t.Run("empty filename", func(t *testing.T) { + writer, err := New("", testMode()) + if writer != nil { + t.Errorf("Should not have created writer") + } + if err == nil || err.Error() != "file name is empty" { + t.Errorf("Should produce a 'file name is empty' error, but got %[1]T (%[1]v)", err) + } + }) + t.Run("directory", func(t *testing.T) { + tmpDir := t.TempDir() + writer, err := New(tmpDir, testMode()) + if writer != nil { + t.Errorf("Should not have created writer") + } + if err == nil || err.Error() != "cannot write to a directory" { + t.Errorf("Should produce a 'cannot write to a directory' error, but got %[1]T (%[1]v)", err) + } + }) } func TestWriteFile(t *testing.T) { + t.Run("empty filename", func(t *testing.T) { + err := WriteFile("", nil, testMode()) + if err == nil || err.Error() != "file name is empty" { + t.Errorf("Should produce a 'file name is empty' error, but got %[1]T (%[1]v)", err) + } + }) + t.Run("write to directory", func(t *testing.T) { + err := WriteFile(t.TempDir(), nil, testMode()) + if err == nil || err.Error() != "cannot write to a directory" { + t.Errorf("Should produce a 'cannot write to a directory' error, but got %[1]T (%[1]v)", err) + } + }) t.Run("write to file", func(t *testing.T) { tmpDir := t.TempDir() fileName := filepath.Join(tmpDir, "test.txt") From 0cd5512baeab1b941bbb7436138a524a91447ac1 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Sun, 9 Mar 2025 15:30:04 +0100 Subject: [PATCH 20/27] pkg/atomicwriter: use sequential file access on Windows Using sequential file access ([FILE_FLAG_SEQUENTIAL_SCAN]) prevents Windows from aggressively keeping files in the cache, freeing up system memory for other tasks. On Linux, these changes have no effect, as the sequential package use the standard (os.CreateTemp, os.OpenFile) on non-Windows platforms. Refer to the [Win32 API documentation] for details on sequential file access. [Win32 API documentation]: https://learn.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-createfilea#FILE_FLAG_SEQUENTIAL_SCAN [FILE_FLAG_SEQUENTIAL_SCAN]: https://learn.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-createfilea#FILE_FLAG_SEQUENTIAL_SCAN Signed-off-by: Sebastiaan van Stijn --- atomicwriter/atomicwriter.go | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/atomicwriter/atomicwriter.go b/atomicwriter/atomicwriter.go index e8aa7807..d798aab2 100644 --- a/atomicwriter/atomicwriter.go +++ b/atomicwriter/atomicwriter.go @@ -6,6 +6,8 @@ import ( "io" "os" "path/filepath" + + "github.com/moby/sys/sequential" ) func validateDestination(fileName string) error { @@ -66,6 +68,13 @@ func validateFileMode(mode os.FileMode) error { // temporary file and closing it atomically changes the temporary file to // destination path. Writing and closing concurrently is not allowed. // NOTE: umask is not considered for the file's permissions. +// +// New uses [sequential.CreateTemp] to use sequential file access on Windows, +// avoiding depleting the standby list un-necessarily. On Linux, this equates to +// a regular [os.CreateTemp]. Refer to the [Win32 API documentation] for details +// on sequential file access. +// +// [Win32 API documentation]: https://learn.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-createfilea#FILE_FLAG_SEQUENTIAL_SCAN func New(filename string, perm os.FileMode) (io.WriteCloser, error) { if err := validateDestination(filename); err != nil { return nil, err @@ -75,7 +84,7 @@ func New(filename string, perm os.FileMode) (io.WriteCloser, error) { return nil, err } - f, err := os.CreateTemp(filepath.Dir(abspath), ".tmp-"+filepath.Base(filename)) + f, err := sequential.CreateTemp(filepath.Dir(abspath), ".tmp-"+filepath.Base(filename)) if err != nil { return nil, err } @@ -197,8 +206,15 @@ func (w syncFileCloser) Close() error { // FileWriter opens a file writer inside the set. The file // should be synced and closed before calling commit. +// +// FileWriter uses [sequential.OpenFile] to use sequential file access on Windows, +// avoiding depleting the standby list un-necessarily. On Linux, this equates to +// a regular [os.OpenFile]. Refer to the [Win32 API documentation] for details +// on sequential file access. +// +// [Win32 API documentation]: https://learn.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-createfilea#FILE_FLAG_SEQUENTIAL_SCAN func (ws *WriteSet) FileWriter(name string, flag int, perm os.FileMode) (io.WriteCloser, error) { - f, err := os.OpenFile(filepath.Join(ws.root, name), flag, perm) + f, err := sequential.OpenFile(filepath.Join(ws.root, name), flag, perm) if err != nil { return nil, err } From da51f406b45848e114f8c3f354e5004d1cfc8c63 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Thu, 3 Apr 2025 21:39:21 +0200 Subject: [PATCH 21/27] pkg/atomicwriter: add basic godoc for package Signed-off-by: Sebastiaan van Stijn --- atomicwriter/atomicwriter.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/atomicwriter/atomicwriter.go b/atomicwriter/atomicwriter.go index d798aab2..d91aab80 100644 --- a/atomicwriter/atomicwriter.go +++ b/atomicwriter/atomicwriter.go @@ -1,3 +1,5 @@ +// Package atomicwriter provides utilities to perform atomic writes to a +// file or set of files. package atomicwriter import ( From f977a5e7b397e6327b8d59ebe791fb8553f1f28e Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Thu, 3 Apr 2025 18:04:40 +0200 Subject: [PATCH 22/27] pkg/atomicwriter: disallow symlinked files for now The implementation uses "os.Rename" to move the temporary file to the destination, which does not follow symlinks, and because of this would replace a symlink with a file. We can consider adding support for symlinked files in future, so that WriteFile can be used as a drop-in replacement for `os.WriteFile()` but in the meantime, let's produce an error so that nobody can depend on this. Signed-off-by: Sebastiaan van Stijn --- atomicwriter/atomicwriter.go | 14 +++++++------ atomicwriter/atomicwriter_test.go | 33 +++++++++++++++++++++++-------- 2 files changed, 33 insertions(+), 14 deletions(-) diff --git a/atomicwriter/atomicwriter.go b/atomicwriter/atomicwriter.go index d91aab80..9509ce6e 100644 --- a/atomicwriter/atomicwriter.go +++ b/atomicwriter/atomicwriter.go @@ -19,8 +19,6 @@ func validateDestination(fileName string) error { // Deliberately using Lstat here to match the behavior of [os.Rename], // which is used when completing the write and does not resolve symlinks. - // - // TODO(thaJeztah): decide whether we want to disallow symlinks or to follow them. if fi, err := os.Lstat(fileName); err != nil { if !os.IsNotExist(err) { return fmt.Errorf("failed to stat output path: %w", err) @@ -42,9 +40,8 @@ func validateFileMode(mode os.FileMode) error { return nil // Regular file case mode&os.ModeDir != 0: return errors.New("cannot write to a directory") - // TODO(thaJeztah): decide whether we want to disallow symlinks or to follow them. - // case mode&os.ModeSymlink != 0: - // return errors.New("cannot write to a symbolic link directly") + case mode&os.ModeSymlink != 0: + return errors.New("cannot write to a symbolic link directly") case mode&os.ModeNamedPipe != 0: return errors.New("cannot write to a named pipe (FIFO)") case mode&os.ModeSocket != 0: @@ -97,7 +94,12 @@ func New(filename string, perm os.FileMode) (io.WriteCloser, error) { }, nil } -// WriteFile atomically writes data to a file named by filename and with the specified permission bits. +// WriteFile atomically writes data to a file named by filename and with the +// specified permission bits. The given filename is created if it does not exist, +// but the destination directory must exist. It can be used as a drop-in replacement +// for [os.WriteFile], but currently does not allow the destination path to be +// a symlink. WriteFile is implemented using [New] for its implementation. +// // NOTE: umask is not considered for the file's permissions. func WriteFile(filename string, data []byte, perm os.FileMode) error { f, err := New(filename, perm) diff --git a/atomicwriter/atomicwriter_test.go b/atomicwriter/atomicwriter_test.go index 9892489f..37328026 100644 --- a/atomicwriter/atomicwriter_test.go +++ b/atomicwriter/atomicwriter_test.go @@ -139,6 +139,24 @@ func TestNewInvalid(t *testing.T) { t.Errorf("Should produce a 'cannot write to a directory' error, but got %[1]T (%[1]v)", err) } }) + t.Run("symlinked file", func(t *testing.T) { + tmpDir := t.TempDir() + linkTarget := filepath.Join(tmpDir, "symlink-target") + if err := os.WriteFile(linkTarget, []byte("orig content"), testMode()); err != nil { + t.Fatal(err) + } + fileName := filepath.Join(tmpDir, "symlinked-file") + if err := os.Symlink(linkTarget, fileName); err != nil { + t.Fatal(err) + } + writer, err := New(fileName, testMode()) + if writer != nil { + t.Errorf("Should not have created writer") + } + if err == nil || err.Error() != "cannot write to a symbolic link directly" { + t.Errorf("Should produce a 'cannot write to a symbolic link directly' error, but got %[1]T (%[1]v)", err) + } + }) } func TestWriteFile(t *testing.T) { @@ -178,7 +196,9 @@ func TestWriteFile(t *testing.T) { t.Run("symlinked file", func(t *testing.T) { tmpDir := t.TempDir() linkTarget := filepath.Join(tmpDir, "symlink-target") - if err := os.WriteFile(linkTarget, []byte("orig content"), testMode()); err != nil { + originalContent := []byte("original content") + fileMode := testMode() + if err := os.WriteFile(linkTarget, originalContent, fileMode); err != nil { t.Fatal(err) } if err := os.Symlink(linkTarget, filepath.Join(tmpDir, "symlinked-file")); err != nil { @@ -188,15 +208,12 @@ func TestWriteFile(t *testing.T) { assertFileCount(t, tmpDir, origFileCount) fileName := filepath.Join(tmpDir, "symlinked-file") - fileContent := []byte("new content") - fileMode := testMode() - if err := WriteFile(fileName, fileContent, fileMode); err != nil { - t.Fatalf("Error writing to file: %v", err) + err := WriteFile(fileName, []byte("new content"), testMode()) + if err == nil || err.Error() != "cannot write to a symbolic link directly" { + t.Errorf("Should produce a 'cannot write to a symbolic link directly' error, but got %[1]T (%[1]v)", err) } - assertFile(t, fileName, fileContent, fileMode) + assertFile(t, linkTarget, originalContent, fileMode) assertFileCount(t, tmpDir, origFileCount) - // FIXME(thaJeztah): [os.Rename] does not resolve symlinks, so writing to a symlinked location replaces the link with a file. - // assertFile(t, linkTarget, fileContent, fileMode) }) t.Run("symlinked directory", func(t *testing.T) { tmpDir := t.TempDir() From 66f215a52d8a1a11a0ce002c10f89edca73e49eb Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Thu, 3 Apr 2025 19:55:25 +0200 Subject: [PATCH 23/27] pkg/atomicwriter: error on unknown file-modes Previously, we were silently discarding this situation and hoping that it would work; let's produce an error instead (we can add additional filemodes when they arrive and if we need them) Signed-off-by: Sebastiaan van Stijn --- atomicwriter/atomicwriter.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/atomicwriter/atomicwriter.go b/atomicwriter/atomicwriter.go index 9509ce6e..af3e6348 100644 --- a/atomicwriter/atomicwriter.go +++ b/atomicwriter/atomicwriter.go @@ -58,8 +58,7 @@ func validateFileMode(mode os.FileMode) error { case mode&os.ModeSticky != 0: return errors.New("cannot write to a sticky bit file") default: - // Unknown file mode; let's assume it works - return nil + return fmt.Errorf("unknown file mode: %[1]s (%#[1]o)", mode) } } From ab8938c4409a879162e118dd4066696ab5fe8545 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Thu, 3 Apr 2025 21:30:40 +0200 Subject: [PATCH 24/27] pkg/atomicwriter: add test for parent dir not being a directory While the target-file does not have to exist, its parent must, and must be a directory. This adds a test-case to verify the behavior if the parent is not a directory. Signed-off-by: Sebastiaan van Stijn --- atomicwriter/atomicwriter_test.go | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/atomicwriter/atomicwriter_test.go b/atomicwriter/atomicwriter_test.go index 37328026..e98f7f33 100644 --- a/atomicwriter/atomicwriter_test.go +++ b/atomicwriter/atomicwriter_test.go @@ -7,6 +7,7 @@ import ( "path/filepath" "runtime" "strings" + "syscall" "testing" ) @@ -120,6 +121,23 @@ func TestNewInvalid(t *testing.T) { t.Errorf("Should produce a 'not found' error, but got %[1]T (%[1]v)", err) } }) + t.Run("target dir is not a directory", func(t *testing.T) { + tmpDir := t.TempDir() + parentPath := filepath.Join(tmpDir, "not-a-dir") + err := os.WriteFile(parentPath, nil, testMode()) + if err != nil { + t.Fatalf("Error writing file: %v", err) + } + fileName := filepath.Join(parentPath, "new-file.txt") + writer, err := New(fileName, testMode()) + if writer != nil { + t.Errorf("Should not have created writer") + } + // This should match the behavior of os.WriteFile, which returns a [os.PathError] with [syscall.ENOTDIR]. + if !errors.Is(err, syscall.ENOTDIR) { + t.Errorf("Should produce a 'not a directory' error, but got %[1]T (%[1]v)", err) + } + }) t.Run("empty filename", func(t *testing.T) { writer, err := New("", testMode()) if writer != nil { From 145deb84df3f2558a66257df1d7e52ebdfb9fa2f Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Thu, 3 Apr 2025 21:57:13 +0200 Subject: [PATCH 25/27] pkg/atomicwriter: return early if parent directory is invalid Rewrite `validateDestination` to first check if the destination path exists. This slightly simplifies the logic (allowing returning early in each step of the validation) and slightly improves the error produced. Before this, the error confusingly would mention the full path not being a directory. While this _does_ match what `os.Writefile` would return, it's .. confusing: failed to stat output path: lstat ./not-a-dir/new-file.txt: not a directory After this, the error would mention the directory that doesn't exist: invalid output path: stat ./not-a-dir: not a directory A slight optimization is made as well, now checking for _both_ "." and ".." as special case, as either path should exist given any current working directory (unless the working directory has been deleted, but we'd fail further down the line). With this change in order, we can also merge `validateFileMode` into `validateDestination`. Signed-off-by: Sebastiaan van Stijn --- atomicwriter/atomicwriter.go | 30 ++++++++++++++++-------------- 1 file changed, 16 insertions(+), 14 deletions(-) diff --git a/atomicwriter/atomicwriter.go b/atomicwriter/atomicwriter.go index af3e6348..d0d3be88 100644 --- a/atomicwriter/atomicwriter.go +++ b/atomicwriter/atomicwriter.go @@ -8,6 +8,7 @@ import ( "io" "os" "path/filepath" + "syscall" "github.com/moby/sys/sequential" ) @@ -16,26 +17,27 @@ func validateDestination(fileName string) error { if fileName == "" { return errors.New("file name is empty") } + if dir := filepath.Dir(fileName); dir != "" && dir != "." && dir != ".." { + di, err := os.Stat(dir) + if err != nil { + return fmt.Errorf("invalid output path: %w", err) + } + if !di.IsDir() { + return fmt.Errorf("invalid output path: %w", &os.PathError{Op: "stat", Path: dir, Err: syscall.ENOTDIR}) + } + } // Deliberately using Lstat here to match the behavior of [os.Rename], // which is used when completing the write and does not resolve symlinks. - if fi, err := os.Lstat(fileName); err != nil { - if !os.IsNotExist(err) { - return fmt.Errorf("failed to stat output path: %w", err) - } - } else if err := validateFileMode(fi.Mode()); err != nil { - return err - } - if dir := filepath.Dir(fileName); dir != "" && dir != "." { - if _, err := os.Stat(dir); errors.Is(err, os.ErrNotExist) { - return fmt.Errorf("invalid file path: %w", err) + fi, err := os.Lstat(fileName) + if err != nil { + if os.IsNotExist(err) { + return nil } + return fmt.Errorf("failed to stat output path: %w", err) } - return nil -} -func validateFileMode(mode os.FileMode) error { - switch { + switch mode := fi.Mode(); { case mode.IsRegular(): return nil // Regular file case mode&os.ModeDir != 0: From 62a361a617ae173e5405073d62875bba07bd2317 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Wed, 2 Apr 2025 01:40:08 +0200 Subject: [PATCH 26/27] atomicwriter: add go.mod Signed-off-by: Sebastiaan van Stijn --- atomicwriter/go.mod | 7 +++++++ atomicwriter/go.sum | 4 ++++ 2 files changed, 11 insertions(+) create mode 100644 atomicwriter/go.mod create mode 100644 atomicwriter/go.sum diff --git a/atomicwriter/go.mod b/atomicwriter/go.mod new file mode 100644 index 00000000..cb5908ea --- /dev/null +++ b/atomicwriter/go.mod @@ -0,0 +1,7 @@ +module github.com/moby/sys/atomicwriter + +go 1.18 + +require github.com/moby/sys/sequential v0.6.0 + +require golang.org/x/sys v0.1.0 // indirect diff --git a/atomicwriter/go.sum b/atomicwriter/go.sum new file mode 100644 index 00000000..b9a5d4aa --- /dev/null +++ b/atomicwriter/go.sum @@ -0,0 +1,4 @@ +github.com/moby/sys/sequential v0.6.0 h1:qrx7XFUd/5DxtqcoH1h438hF5TmOvzC/lspjy7zgvCU= +github.com/moby/sys/sequential v0.6.0/go.mod h1:uyv8EUTrca5PnDsdMGXhZe6CCe8U/UiTWd+lL+7b/Ko= +golang.org/x/sys v0.1.0 h1:kunALQeHf1/185U1i0GOB/fy1IPRDDpuoOOqRReG57U= +golang.org/x/sys v0.1.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= From 17fe011b1d5b50f6758a1af3b585e49d5e487762 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Wed, 2 Apr 2025 01:46:58 +0200 Subject: [PATCH 27/27] atomicwriter: add to Makefile and GitHub actions Signed-off-by: Sebastiaan van Stijn --- .github/workflows/test.yml | 2 +- Makefile | 15 +++++++++++---- 2 files changed, 12 insertions(+), 5 deletions(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 3fa92833..d3a64e2a 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -26,7 +26,7 @@ jobs: run: | # This corresponds with the list in Makefile:1, but omits the "userns" # and "capability" modules, which require go1.21 as minimum. - echo 'PACKAGES=mountinfo mount reexec sequential signal symlink user' >> $GITHUB_ENV + echo 'PACKAGES=atomicwriter mountinfo mount reexec sequential signal symlink user' >> $GITHUB_ENV - name: go mod tidy run: | make foreach CMD="go mod tidy" diff --git a/Makefile b/Makefile index bc0c1246..7f68e84d 100644 --- a/Makefile +++ b/Makefile @@ -1,4 +1,4 @@ -PACKAGES ?= capability mountinfo mount reexec sequential signal symlink user userns # IMPORTANT: when updating this list, also update the conditional one in .github/workflows/test.yml +PACKAGES ?= atomicwriter capability mountinfo mount reexec sequential signal symlink user userns # IMPORTANT: when updating this list, also update the conditional one in .github/workflows/test.yml BINDIR ?= _build/bin CROSS ?= linux/arm linux/arm64 linux/ppc64le linux/s390x \ freebsd/amd64 openbsd/amd64 darwin/amd64 darwin/arm64 windows/amd64 @@ -29,9 +29,12 @@ test: test-local test: CMD=go test $(RUN_VIA_SUDO) -v -coverprofile=coverage.txt -covermode=atomic . test: foreach -# Test the mount module against the local mountinfo source code instead of the -# release specified in its go.mod. This allows catching regressions / breaking -# changes in mountinfo. +# Some modules in this repo have interdependencies: +# - mount depends on mountinfo +# - atomicwrite depends on sequential +# +# The code below tests these modules against their local dependencies +# to catch regressions / breaking changes early. .PHONY: test-local test-local: MOD = -modfile=go-local.mod test-local: @@ -39,6 +42,10 @@ test-local: # Run go mod tidy to make sure mountinfo dependency versions are met. cd mount && go mod tidy $(MOD) && go test $(MOD) $(RUN_VIA_SUDO) -v . $(RM) mount/go-local.* + echo 'replace github.com/moby/sys/sequential => ../sequential' | cat atomicwriter/go.mod - > atomicwriter/go-local.mod + # Run go mod tidy to make sure sequential dependency versions are met. + cd atomicwriter && go mod tidy $(MOD) && go test $(MOD) $(RUN_VIA_SUDO) -v . + $(RM) atomicwriter/go-local.* .PHONY: lint lint: $(BINDIR)/golangci-lint