Skip to content

fix IsFile and IsDir panicking on a stat error other than not-exist - #11

Merged
umputun merged 1 commit into
masterfrom
fix/exists-stat-error-and-test-determinism
Aug 19, 2026
Merged

fix IsFile and IsDir panicking on a stat error other than not-exist#11
umputun merged 1 commit into
masterfrom
fix/exists-stat-error-and-test-determinism

Conversation

@paskal

@paskal paskal commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

Three related bits of hardening, all behind existing API.

IsFile and IsDir panicked on a stat error other than not-exist

info, err := os.Stat(name)
if os.IsNotExist(err) {
    return false
}
if dir {
    return info.IsDir()   // info is nil for any other error
}

os.Stat returns a nil FileInfo on every failure, not only on a missing file, so anything else reached info.IsDir() and dereferenced nil. A file under a directory the process cannot read is the ordinary way to hit it:

panic: runtime error: invalid memory address or nil pointer dereference
    fileutils.exists(...) fileutils.go:63
    fileutils.IsFile(...) fileutils.go:47

Every stat failure now reports false. No call that previously returned a value behaves differently, only the ones that panicked.

The copy-and-delete fallback in MoveFile was never tested

The subtest named "move with copy fallback" did not reach the fallback. Its destination directory was missing, so the first os.Rename failed with ENOENT, MkdirAll created the directory, and the retry succeeded. CopyFile was never called.

MoveFile is now a one-line wrapper over an unexported moveFile that takes the rename call as an argument. The old subtest keeps its coverage under a name that says what it does, and a new one forces rename to fail so the fallback has to carry the move, asserting both rename attempts happen and the content lands with the source removed. Nothing exported changes.

TouchFile tests no longer depend on wall clock

The update case slept 100ms so the modification time would differ, which is both slow and dependent on the filesystem's timestamp resolution. It now backdates the file an hour with os.Chtimes, so the timestamp has to move regardless of resolution and nothing sleeps.

The creation case asserted time.Since(info.ModTime()) < time.Second, an upper bound that measures how loaded the runner is rather than anything about TouchFile. It now bounds the time from below against a moment captured before the call.

Verification

The new TestExistsStatError panics against the old exists and passes against the new one, so it is not a vacuous test. Full suite green under -race, golangci-lint clean. The permission-based case skips under uid 0, since root bypasses directory permissions.

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.
@paskal
paskal requested a review from umputun as a code owner August 19, 2026 05:48
@umputun
umputun merged commit d2cef51 into master Aug 19, 2026
7 checks passed
@umputun
umputun deleted the fix/exists-stat-error-and-test-determinism branch August 19, 2026 05:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants