From 6a0c1f117ce1c1703952a15651b72622fc82b424 Mon Sep 17 00:00:00 2001 From: Dmitry Verkhoturov Date: Wed, 19 Aug 2026 06:46:50 +0100 Subject: [PATCH] fix IsFile and IsDir panicking on a stat error other than not-exist exists guarded only os.IsNotExist, so any other stat failure fell through to info.IsDir() with info nil. A path under a directory the process cannot read, or a path the OS rejects outright, panicked with a nil pointer dereference instead of reporting that the file is not there. Every stat failure now reports false. Nothing that previously returned a value changes, only the cases that panicked. MoveFile gains an unexported moveFile taking the rename call as an argument. The existing fallback test never reached the copy+delete branch: its first rename failed only because the destination directory was missing, and the retry after MkdirAll succeeded. It is renamed to say what it actually covers, and a new case forces rename to fail so the fallback carries the move, asserting the two rename attempts and the moved content. TouchFile tests no longer depend on wall clock. The 100ms sleep is replaced by backdating the file with os.Chtimes, which also makes the assertion independent of filesystem timestamp resolution, and the creation case bounds the modification time from below rather than measuring how fast the runner is. --- fileutils.go | 11 ++++++-- fileutils_test.go | 71 ++++++++++++++++++++++++++++++++++++++++++++--- 2 files changed, 75 insertions(+), 7 deletions(-) diff --git a/fileutils.go b/fileutils.go index 7e11915..5478fee 100644 --- a/fileutils.go +++ b/fileutils.go @@ -54,7 +54,7 @@ func IsDir(dirname string) bool { func exists(name string, dir bool) bool { info, err := os.Stat(name) - if os.IsNotExist(err) { + if err != nil { // any stat failure leaves info nil, not just a missing file return false } if dir { @@ -235,6 +235,11 @@ func SanitizePath(s string) string { // If rename fails (e.g., cross-device move), it will fall back to copy+delete. // It will create destination directories if they don't exist. func MoveFile(src, dst string) error { + return moveFile(src, dst, os.Rename) +} + +// moveFile is MoveFile with the rename call injected, so the copy+delete fallback can be tested. +func moveFile(src, dst string, rename func(oldpath, newpath string) error) error { if src == "" { return errors.New("empty source path") } @@ -257,7 +262,7 @@ func MoveFile(src, dst string) error { } // try atomic rename first - if err = os.Rename(src, dst); err == nil { + if err = rename(src, dst); err == nil { return nil } @@ -267,7 +272,7 @@ func MoveFile(src, dst string) error { } // try rename again after creating directory - if err = os.Rename(src, dst); err == nil { + if err = rename(src, dst); err == nil { return nil } diff --git a/fileutils_test.go b/fileutils_test.go index 518a01b..6f1f480 100644 --- a/fileutils_test.go +++ b/fileutils_test.go @@ -1,6 +1,7 @@ package fileutils import ( + "errors" "os" "path/filepath" "strconv" @@ -31,6 +32,39 @@ func TestExistsDir(t *testing.T) { assert.False(t, IsDir("testfiles-nop")) } +func TestExistsStatError(t *testing.T) { + t.Run("invalid path", func(t *testing.T) { + invalidPath := "invalid\x00path" + _, err := os.Stat(invalidPath) + require.Error(t, err) + require.False(t, os.IsNotExist(err), "this path must produce a stat error other than not-exist") + + assert.False(t, IsFile(invalidPath)) + assert.False(t, IsDir(invalidPath)) + }) + + t.Run("unreadable parent directory", func(t *testing.T) { + if os.Geteuid() == 0 { + t.Skip("root bypasses directory permissions") + } + + locked := filepath.Join(t.TempDir(), "locked") + require.NoError(t, os.Mkdir(locked, 0o700)) + target := filepath.Join(locked, "file.txt") + require.NoError(t, os.WriteFile(target, []byte("test content"), 0o600)) + + require.NoError(t, os.Chmod(locked, 0o000)) + t.Cleanup(func() { _ = os.Chmod(locked, 0o700) }) + + _, err := os.Stat(target) + require.Error(t, err) + require.False(t, os.IsNotExist(err), "this path must produce a permission error, not not-exist") + + assert.False(t, IsFile(target)) + assert.False(t, IsDir(target)) + }) +} + func TestCopyFile(t *testing.T) { tmpDir := t.TempDir() @@ -271,7 +305,7 @@ func TestMoveFile(t *testing.T) { assert.Equal(t, "test content", string(content)) }) - t.Run("move with copy fallback", func(t *testing.T) { + t.Run("move into a missing directory", func(t *testing.T) { // create source dir and file srcDir := t.TempDir() srcFile := filepath.Join(srcDir, "move_test_src2.txt") @@ -295,6 +329,29 @@ func TestMoveFile(t *testing.T) { assert.Equal(t, "test content", string(content)) }) + t.Run("copy fallback when rename fails", func(t *testing.T) { + srcFile := filepath.Join(t.TempDir(), "move_test_src3.txt") + require.NoError(t, os.WriteFile(srcFile, []byte("test content"), 0o600)) + + dstFile := filepath.Join(t.TempDir(), "subdir", "move_test_dst.txt") + + // rename always fails, so the copy+delete path has to carry the move + renameAttempts := 0 + err := moveFile(srcFile, dstFile, func(_, _ string) error { + renameAttempts++ + return errors.New("forced rename failure") + }) + require.NoError(t, err) + assert.Equal(t, 2, renameAttempts, "rename is retried once after the destination directory is created") + + _, err = os.Stat(srcFile) + assert.True(t, os.IsNotExist(err), "source file should not exist") + + content, err := os.ReadFile(dstFile) //nolint:gosec + require.NoError(t, err) + assert.Equal(t, "test content", string(content)) + }) + t.Run("errors", func(t *testing.T) { tests := []struct { name string @@ -337,13 +394,16 @@ func TestTouchFile(t *testing.T) { tmpDir := t.TempDir() newFile := filepath.Join(tmpDir, "new.txt") + // bound the modification time from below only, an upper bound would just measure the runner + before := time.Now().Add(-time.Second) + err := TouchFile(newFile) require.NoError(t, err) info, err := os.Stat(newFile) require.NoError(t, err) assert.Equal(t, int64(0), info.Size()) - assert.True(t, time.Since(info.ModTime()) < time.Second) + assert.True(t, info.ModTime().After(before), "modification time should be set on creation") }) t.Run("update existing", func(t *testing.T) { @@ -353,10 +413,13 @@ func TestTouchFile(t *testing.T) { err := os.WriteFile(existingFile, []byte("test"), 0600) require.NoError(t, err) - // get original time and wait a bit + // backdate the file rather than sleeping, so the timestamp has to move regardless of + // how coarse the filesystem's timestamp resolution is + backdated := time.Now().Add(-time.Hour) + require.NoError(t, os.Chtimes(existingFile, backdated, backdated)) + origInfo, err := os.Stat(existingFile) require.NoError(t, err) - time.Sleep(time.Millisecond * 100) err = TouchFile(existingFile) require.NoError(t, err)