diff --git a/TODO.md b/TODO.md index ef00742..423e692 100644 --- a/TODO.md +++ b/TODO.md @@ -29,6 +29,11 @@ # Completed Steps +- the hash and content phases open each file without blocking and check that it + is still a regular file, so one replaced by a FIFO or device node after it was + statted is warned about and skipped instead of hanging the scan (2026-10-14, + https://git.eeqj.de/sneak/sfdupes/issues/100) + - the scan lock file is named from the database's path with symlinks resolved, after `scan` creates the database file if it is missing, so a scan through a symlink to the database fails while another scan holds the lock (2026-10-14, diff --git a/scan.go b/scan.go index 5514ab1..1b8eb68 100644 --- a/scan.go +++ b/scan.go @@ -1309,6 +1309,38 @@ func hashWorker(ctx context.Context, jobs <-chan []fileRec, } } +// errNotRegular reports a path that is no longer a regular file when it +// is opened to be read. +var errNotRegular = errors.New("not a regular file") + +// openRegularFile opens path for reading and returns it only if it is +// still a regular file, since the path may have been replaced after it +// was statted. O_NONBLOCK keeps the open of a FIFO or a device node +// from waiting forever, and fstat then rejects it. Reads of a regular +// file ignore O_NONBLOCK. +func openRegularFile(path string) (*os.File, error) { + //nolint:gosec // hashing operator-supplied paths is the tool's purpose + f, err := os.OpenFile(path, os.O_RDONLY|syscall.O_NONBLOCK, 0) + if err != nil { + return nil, err + } + + fi, err := f.Stat() + if err != nil { + _ = f.Close() + + return nil, err + } + + if !fi.Mode().IsRegular() { + _ = f.Close() + + return nil, errNotRegular + } + + return f, nil +} + // emptyHash is the lowercase-hex SHA-256 of the empty input: the head, // tail, and content hash of every zero-length file. const emptyHash = "e3b0c44298fc1c149afbf4c8996fb924" + @@ -1331,8 +1363,7 @@ func hashSignature(path string, size int64) (string, string, string, error) { return emptyHash, emptyHash, emptyHash, nil } - //nolint:gosec // hashing operator-supplied paths is the tool's purpose - f, err := os.Open(path) + f, err := openRegularFile(path) if err != nil { return "", "", "", err } @@ -1363,8 +1394,7 @@ func hashSignature(path string, size int64) (string, string, string, error) { // tail are returned empty, because the content phase keeps the ones its // records already hold. func hashContentOnly(path string, size int64) (string, string, string, error) { - //nolint:gosec // hashing operator-supplied paths is the tool's purpose - f, err := os.Open(path) + f, err := openRegularFile(path) if err != nil { return "", "", "", err } diff --git a/scan_test.go b/scan_test.go index 079f7b9..7d3c705 100644 --- a/scan_test.go +++ b/scan_test.go @@ -1797,6 +1797,54 @@ func TestScanHardlinkRunFailsTogether(t *testing.T) { } } +// TestHashPhaseSkipsFileReplacedByFIFO replaces a walked regular file +// with a FIFO before the hash phase reads it. Opening a FIFO that has +// no writer waits for one, so a blocking open would hang the phase. The +// path must be warned about once and skipped, with no record written. +// +//nolint:paralleltest // captureStderr replaces the process-wide os.Stderr +func TestHashPhaseSkipsFileReplacedByFIFO(t *testing.T) { + stderr := captureStderr(t) + dir := t.TempDir() + p := writeFile(t, dir, "a.bin", pattern(1, 300)) + + walked, _ := collectWalk(t, []string{dir}, false, 1) + + err := os.Remove(p) + if err != nil { + t.Fatal(err) + } + + err = unix.Mkfifo(p, 0o600) + if err != nil { + t.Fatal(err) + } + + s := &scanState{db: openTestDB(t), toHash: walked} + + err = s.hashPhase(t.Context(), 1) + if err != nil { + t.Fatalf("hashPhase = %v, want nil", err) + } + + if s.st.skipped != 1 || len(s.batch) != 0 { + t.Errorf("skipped = %d, records = %d, want 1 skipped and none", + s.st.skipped, len(s.batch)) + } + + warning := "hash " + p + ": not a regular file\n" + if got := stderr(); strings.Count(got, warning) != 1 { + t.Errorf("stderr = %q, want %q once", got, warning) + } + + // The content phase opens files the same way. + _, _, content, err := hashContentOnly(p, headTailMin) + if content != "" || !errors.Is(err, errNotRegular) { + t.Errorf("hashContentOnly on a FIFO = %q, %v; want no hash and %v", + content, err, errNotRegular) + } +} + // injectedWriteFailure is the message the injected database trigger // aborts with, so the test can recognize its own failure in the error // the scan reports.