Skip to content

Commit d7b743b

Browse files
thaJeztahthaJeztah
authored
Merge pull request #49741 from thaJeztah/atomicwriter_stricter_validate
pkg/atomicwriter: disallow symlinks for now, add more tests and touch-up GoDoc
2 parents 43b7c78 + c6cdfbf commit d7b743b

2 files changed

Lines changed: 70 additions & 30 deletions

File tree

‎pkg/atomicwriter/atomicwriter.go‎

Lines changed: 27 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,5 @@
1+
// Package atomicwriter provides utilities to perform atomic writes to a
2+
// file or set of files.
13
package atomicwriter
24

35
import (
@@ -6,6 +8,7 @@ import (
68
"io"
79
"os"
810
"path/filepath"
11+
"syscall"
912

1013
"github.com/moby/sys/sequential"
1114
)
@@ -14,35 +17,33 @@ func validateDestination(fileName string) error {
1417
if fileName == "" {
1518
return errors.New("file name is empty")
1619
}
20+
if dir := filepath.Dir(fileName); dir != "" && dir != "." && dir != ".." {
21+
di, err := os.Stat(dir)
22+
if err != nil {
23+
return fmt.Errorf("invalid output path: %w", err)
24+
}
25+
if !di.IsDir() {
26+
return fmt.Errorf("invalid output path: %w", &os.PathError{Op: "stat", Path: dir, Err: syscall.ENOTDIR})
27+
}
28+
}
1729

1830
// Deliberately using Lstat here to match the behavior of [os.Rename],
1931
// which is used when completing the write and does not resolve symlinks.
20-
//
21-
// TODO(thaJeztah): decide whether we want to disallow symlinks or to follow them.
22-
if fi, err := os.Lstat(fileName); err != nil {
23-
if !os.IsNotExist(err) {
24-
return fmt.Errorf("failed to stat output path: %w", err)
25-
}
26-
} else if err := validateFileMode(fi.Mode()); err != nil {
27-
return err
28-
}
29-
if dir := filepath.Dir(fileName); dir != "" && dir != "." {
30-
if _, err := os.Stat(dir); errors.Is(err, os.ErrNotExist) {
31-
return fmt.Errorf("invalid file path: %w", err)
32+
fi, err := os.Lstat(fileName)
33+
if err != nil {
34+
if os.IsNotExist(err) {
35+
return nil
3236
}
37+
return fmt.Errorf("failed to stat output path: %w", err)
3338
}
34-
return nil
35-
}
3639

37-
func validateFileMode(mode os.FileMode) error {
38-
switch {
40+
switch mode := fi.Mode(); {
3941
case mode.IsRegular():
4042
return nil // Regular file
4143
case mode&os.ModeDir != 0:
4244
return errors.New("cannot write to a directory")
43-
// TODO(thaJeztah): decide whether we want to disallow symlinks or to follow them.
44-
// case mode&os.ModeSymlink != 0:
45-
// return errors.New("cannot write to a symbolic link directly")
45+
case mode&os.ModeSymlink != 0:
46+
return errors.New("cannot write to a symbolic link directly")
4647
case mode&os.ModeNamedPipe != 0:
4748
return errors.New("cannot write to a named pipe (FIFO)")
4849
case mode&os.ModeSocket != 0:
@@ -59,8 +60,7 @@ func validateFileMode(mode os.FileMode) error {
5960
case mode&os.ModeSticky != 0:
6061
return errors.New("cannot write to a sticky bit file")
6162
default:
62-
// Unknown file mode; let's assume it works
63-
return nil
63+
return fmt.Errorf("unknown file mode: %[1]s (%#[1]o)", mode)
6464
}
6565
}
6666

@@ -95,7 +95,12 @@ func New(filename string, perm os.FileMode) (io.WriteCloser, error) {
9595
}, nil
9696
}
9797

98-
// WriteFile atomically writes data to a file named by filename and with the specified permission bits.
98+
// WriteFile atomically writes data to a file named by filename and with the
99+
// specified permission bits. The given filename is created if it does not exist,
100+
// but the destination directory must exist. It can be used as a drop-in replacement
101+
// for [os.WriteFile], but currently does not allow the destination path to be
102+
// a symlink. WriteFile is implemented using [New] for its implementation.
103+
//
99104
// NOTE: umask is not considered for the file's permissions.
100105
func WriteFile(filename string, data []byte, perm os.FileMode) error {
101106
f, err := New(filename, perm)

‎pkg/atomicwriter/atomicwriter_test.go‎

Lines changed: 43 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import (
77
"path/filepath"
88
"runtime"
99
"strings"
10+
"syscall"
1011
"testing"
1112
)
1213

@@ -120,6 +121,23 @@ func TestNewInvalid(t *testing.T) {
120121
t.Errorf("Should produce a 'not found' error, but got %[1]T (%[1]v)", err)
121122
}
122123
})
124+
t.Run("target dir is not a directory", func(t *testing.T) {
125+
tmpDir := t.TempDir()
126+
parentPath := filepath.Join(tmpDir, "not-a-dir")
127+
err := os.WriteFile(parentPath, nil, testMode())
128+
if err != nil {
129+
t.Fatalf("Error writing file: %v", err)
130+
}
131+
fileName := filepath.Join(parentPath, "new-file.txt")
132+
writer, err := New(fileName, testMode())
133+
if writer != nil {
134+
t.Errorf("Should not have created writer")
135+
}
136+
// This should match the behavior of os.WriteFile, which returns a [os.PathError] with [syscall.ENOTDIR].
137+
if !errors.Is(err, syscall.ENOTDIR) {
138+
t.Errorf("Should produce a 'not a directory' error, but got %[1]T (%[1]v)", err)
139+
}
140+
})
123141
t.Run("empty filename", func(t *testing.T) {
124142
writer, err := New("", testMode())
125143
if writer != nil {
@@ -139,6 +157,24 @@ func TestNewInvalid(t *testing.T) {
139157
t.Errorf("Should produce a 'cannot write to a directory' error, but got %[1]T (%[1]v)", err)
140158
}
141159
})
160+
t.Run("symlinked file", func(t *testing.T) {
161+
tmpDir := t.TempDir()
162+
linkTarget := filepath.Join(tmpDir, "symlink-target")
163+
if err := os.WriteFile(linkTarget, []byte("orig content"), testMode()); err != nil {
164+
t.Fatal(err)
165+
}
166+
fileName := filepath.Join(tmpDir, "symlinked-file")
167+
if err := os.Symlink(linkTarget, fileName); err != nil {
168+
t.Fatal(err)
169+
}
170+
writer, err := New(fileName, testMode())
171+
if writer != nil {
172+
t.Errorf("Should not have created writer")
173+
}
174+
if err == nil || err.Error() != "cannot write to a symbolic link directly" {
175+
t.Errorf("Should produce a 'cannot write to a symbolic link directly' error, but got %[1]T (%[1]v)", err)
176+
}
177+
})
142178
}
143179

144180
func TestWriteFile(t *testing.T) {
@@ -178,7 +214,9 @@ func TestWriteFile(t *testing.T) {
178214
t.Run("symlinked file", func(t *testing.T) {
179215
tmpDir := t.TempDir()
180216
linkTarget := filepath.Join(tmpDir, "symlink-target")
181-
if err := os.WriteFile(linkTarget, []byte("orig content"), testMode()); err != nil {
217+
originalContent := []byte("original content")
218+
fileMode := testMode()
219+
if err := os.WriteFile(linkTarget, originalContent, fileMode); err != nil {
182220
t.Fatal(err)
183221
}
184222
if err := os.Symlink(linkTarget, filepath.Join(tmpDir, "symlinked-file")); err != nil {
@@ -188,15 +226,12 @@ func TestWriteFile(t *testing.T) {
188226
assertFileCount(t, tmpDir, origFileCount)
189227

190228
fileName := filepath.Join(tmpDir, "symlinked-file")
191-
fileContent := []byte("new content")
192-
fileMode := testMode()
193-
if err := WriteFile(fileName, fileContent, fileMode); err != nil {
194-
t.Fatalf("Error writing to file: %v", err)
229+
err := WriteFile(fileName, []byte("new content"), testMode())
230+
if err == nil || err.Error() != "cannot write to a symbolic link directly" {
231+
t.Errorf("Should produce a 'cannot write to a symbolic link directly' error, but got %[1]T (%[1]v)", err)
195232
}
196-
assertFile(t, fileName, fileContent, fileMode)
233+
assertFile(t, linkTarget, originalContent, fileMode)
197234
assertFileCount(t, tmpDir, origFileCount)
198-
// FIXME(thaJeztah): [os.Rename] does not resolve symlinks, so writing to a symlinked location replaces the link with a file.
199-
// assertFile(t, linkTarget, fileContent, fileMode)
200235
})
201236
t.Run("symlinked directory", func(t *testing.T) {
202237
tmpDir := t.TempDir()

0 commit comments

Comments
 (0)