Read only regular files in gen, freshen and check (closes #189)

Opening a named pipe that has no writer blocks, so gen and freshen hung
on a tree holding one, and check hung on a listed path that is a pipe on
disk. gen and freshen now list regular files only (with
--follow-symlinks, what a symlink points to must be one) and name what
they skip with -v. check reports a listed path that is not a regular
file as an error without opening it, and its search for extra files
leaves out pipes, sockets and devices.

Judgement call: the pipe tests build on unix only, where syscall.Mkfifo
exists.
Unverified: sockets and devices are not tested; the tests use pipes.

Model: opus-5-5

README.md

@@ -268,6 +268,8 @@ are now tracked only in the [issues](https://git.eeqj.de/sneak/mfer/issues).
 
 - `mfer gen` / `mfer gen .`
     - recurses under current directory and writes out an `index.mf`
+    - lists regular files only, and skips a named pipe, socket or device, which
+      it names with `-v`
     - records every file's mode as `0000` unless given `--include-permissions`,
       which records each file's permission bits (`0777` at most)
 - `mfer gen /media/drive`
@@ -294,9 +296,10 @@ are now tracked only in the [issues](https://git.eeqj.de/sneak/mfer/issues).
     - rewrites `index.mf` to list the files now under the directory holding it,
       or under the one `--base` names, hashing only the files that are new or
       changed
+    - lists regular files only, as gen does
     - leaves out hidden files unless given `--include-dotfiles`, and symlinks
-      unless given `--follow-symlinks`, which lists each symlink to a file under
-      its own name with the contents of the file it points to
+      unless given `--follow-symlinks`, which lists each symlink to a regular
+      file under its own name with the contents of the file it points to
 - `mfer gen --sign-key key.asc` / `mfer freshen --sign-key key.asc`
     - signs the manifest with the OpenPGP secret key in `key.asc`, armored or
       binary, as `gpg --export-secret-keys` writes it; `MFER_SIGN_KEY` names the

internal/cli/freshen.go

@@ -192,6 +192,13 @@ func (s *freshenScanner) walk(path string, info fs.FileInfo, walkErr error) erro
 		info = realInfo
 	}
 
+	// Opening a named pipe can block, and reading a device may never end.
+	if !info.Mode().IsRegular() {
+		log.Verbosef("skipping %s: not a regular file", relPath)
+
+		return nil
+	}
+
 	for _, excluded := range s.excluded {
 		if os.SameFile(info, excluded) {
 			return nil

internal/cli/pipe_test.go

@@ -0,0 +1,94 @@
+//go:build unix
+
+//nolint:testpackage // white-box tests exercise unexported internals
+package cli
+
+import (
+	"os"
+	"path/filepath"
+	"syscall"
+	"testing"
+	"time"
+
+	"github.com/spf13/afero"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+const (
+	// testPipe is the name of the named pipe the pipe tests make in a tree.
+	testPipe = "pipe"
+
+	// pipeDeadline is how long a test lets a run go before it opens the
+	// test's named pipes for writing.
+	pipeDeadline = 5 * time.Second
+)
+
+// makePipe makes a named pipe at path. Opening a named pipe for reading
+// blocks until it is opened for writing, so once pipeDeadline has passed,
+// unless the test has ended, makePipe opens it for writing and closes it
+// again. A run that wrongly reads the pipe then ends, and the test fails on
+// what it reports instead of hanging. That lets one read through, so each
+// path a wrong run would read gets a pipe of its own.
+func makePipe(t *testing.T, path string) {
+	t.Helper()
+
+	require.NoError(t, syscall.Mkfifo(path, 0o600))
+
+	timer := time.AfterFunc(pipeDeadline, func() {
+		f, err := os.OpenFile(path, os.O_WRONLY, 0) //nolint:gosec // test-controlled path
+		if err == nil {
+			_ = f.Close()
+		}
+	})
+
+	t.Cleanup(func() { timer.Stop() })
+}
+
+// TestGenerateSkipsPipe runs gen -v on a tree holding a named pipe: the
+// manifest lists the regular file only, and gen names the pipe it skipped.
+func TestGenerateSkipsPipe(t *testing.T) {
+	t.Parallel()
+
+	dir := t.TempDir()
+	fs := afero.NewOsFs()
+	writeTestFile(t, fs, filepath.Join(dir, testFileTxt), "regular")
+	makePipe(t, filepath.Join(dir, testPipe))
+
+	opts := testOpts([]string{testApp, cmdGenerate, "-v", dir}, fs)
+	require.Equal(t, 0, runCLI(opts), "stderr: %s", testStderr(t, opts))
+
+	assert.Equal(t, []string{testFileTxt},
+		manifestPaths(t, fs, filepath.Join(dir, defaultManifestName)))
+	assert.Contains(t, testStderr(t, opts), "skipping pipe: not a regular file")
+}
+
+// TestFreshenSkipsPipes adds a named pipe to a tree after gen made its
+// manifest, and a symlink to another pipe. freshen -v --follow-symlinks
+// leaves the manifest listing the regular file only, and names both entries
+// it skipped.
+func TestFreshenSkipsPipes(t *testing.T) {
+	t.Parallel()
+
+	fs := afero.NewOsFs()
+	root, manifestPath := setupFreshenDir(t, fs,
+		map[string]string{testFileTxt: "regular"})
+	makePipe(t, filepath.Join(root, testPipe))
+
+	// The symlink points outside the tree, so no pipe is read twice.
+	target := filepath.Join(t.TempDir(), testPipe)
+	makePipe(t, target)
+	require.NoError(t, os.Symlink(target, filepath.Join(root, testLink)))
+
+	opts := testOpts([]string{
+		testApp, cmdFreshen, "-v", "--follow-symlinks",
+		testFlagBase, root, manifestPath,
+	}, fs)
+	require.Equal(t, 0, runCLI(opts), "stderr: %s", testStderr(t, opts))
+
+	assert.Equal(t, []string{testFileTxt}, manifestPaths(t, fs, manifestPath))
+
+	stderr := testStderr(t, opts)
+	assert.Contains(t, stderr, "skipping pipe: not a regular file")
+	assert.Contains(t, stderr, "skipping link: not a regular file")
+}

mfer/checker.go

@@ -318,6 +318,11 @@ func (c *Checker) FindExtraFiles(ctx context.Context, results chan<- Result) err
 			return nil
 		}
 
+		// gen never lists a named pipe, a socket or a device.
+		if info.Mode()&(os.ModeNamedPipe|os.ModeSocket|os.ModeDevice) != 0 {
+			return nil
+		}
+
 		// A symlink is compared by what it points to, so a manifest reached
 		// through one is not reported either.
 		if info.Mode()&os.ModeSymlink != 0 {
@@ -371,6 +376,15 @@ func (c *Checker) checkFile(entry *MFFilePath, checkedBytes *FileSize) Result {
 		return Result{Path: relPath, Status: StatusError, Message: err.Error()}
 	}
 
+	// Opening a named pipe can block, and reading a device may never end.
+	if !info.Mode().IsRegular() {
+		return Result{
+			Path:    relPath,
+			Status:  StatusError,
+			Message: "not a regular file",
+		}
+	}
+
 	// Check size
 	if info.Size() != entry.GetSize() {
 		*checkedBytes += FileSize(info.Size())

mfer/pipe_test.go

@@ -0,0 +1,123 @@
+//go:build unix
+
+//nolint:testpackage // white-box tests exercise unexported internals
+package mfer
+
+import (
+	"bytes"
+	"context"
+	"os"
+	"path/filepath"
+	"syscall"
+	"testing"
+	"time"
+
+	"github.com/spf13/afero"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// pipeDeadline is how long a test lets a run go before it opens the test's
+// named pipes for writing.
+const pipeDeadline = 5 * time.Second
+
+// makePipe makes a named pipe at path. Opening a named pipe for reading
+// blocks until it is opened for writing, so once pipeDeadline has passed,
+// unless the test has ended, makePipe opens it for writing and closes it
+// again. A run that wrongly reads the pipe then ends, and the test fails on
+// what it reports instead of hanging. That lets one read through, so each
+// path a wrong run would read gets a pipe of its own.
+func makePipe(t *testing.T, path string) {
+	t.Helper()
+
+	require.NoError(t, syscall.Mkfifo(path, 0o600))
+
+	timer := time.AfterFunc(pipeDeadline, func() {
+		f, err := os.OpenFile(path, os.O_WRONLY, 0) //nolint:gosec // test-controlled path
+		if err == nil {
+			_ = f.Close()
+		}
+	})
+
+	t.Cleanup(func() { timer.Stop() })
+}
+
+// TestScannerSkipsPipes scans a tree holding a named pipe and a symlink to
+// another one, following symlinks: the manifest lists the regular file only.
+func TestScannerSkipsPipes(t *testing.T) {
+	t.Parallel()
+
+	dir := t.TempDir()
+	require.NoError(t, os.WriteFile(filepath.Join(dir, testFile1), []byte("a"), 0o600))
+	makePipe(t, filepath.Join(dir, "pipe"))
+
+	// The symlink points outside the tree, so no pipe is read twice.
+	target := filepath.Join(t.TempDir(), "pipe")
+	makePipe(t, target)
+	require.NoError(t, os.Symlink(target, filepath.Join(dir, "link")))
+
+	s := NewScannerWithOptions(&ScannerOptions{FollowSymLinks: true})
+	require.NoError(t, s.EnumeratePath(dir, nil))
+
+	var buf bytes.Buffer
+	require.NoError(t, s.ToManifest(context.Background(), &buf, nil))
+
+	m, err := NewManifestFromReader(&buf)
+	require.NoError(t, err)
+	require.Len(t, m.Files(), 1)
+	assert.Equal(t, testFile1, m.Files()[0].GetPath())
+}
+
+// TestCheckPipes checks a tree against a manifest that lists, as an empty
+// file, a path that is a named pipe on disk. The tree holds another pipe the
+// manifest does not list. check reports the listed pipe as an error without
+// reading it, and the search for extra files does not report the other.
+func TestCheckPipes(t *testing.T) {
+	t.Parallel()
+
+	dir := t.TempDir()
+	manifestPath := filepath.Join(dir, "index.mf")
+	content := []byte("in manifest")
+	listedPipe := RelFilePath("listed-pipe")
+
+	fs := afero.NewOsFs()
+	createTestManifest(t, fs, manifestPath, map[string][]byte{
+		testFile1:          content,
+		string(listedPipe): {},
+	})
+	require.NoError(t, afero.WriteFile(fs, filepath.Join(dir, testFile1), content, 0o600))
+	makePipe(t, filepath.Join(dir, string(listedPipe)))
+	makePipe(t, filepath.Join(dir, "unlisted-pipe"))
+
+	chk, err := NewChecker(&CheckerOptions{
+		ManifestPath: manifestPath,
+		BasePath:     dir,
+		Fs:           fs,
+	})
+	require.NoError(t, err)
+
+	results := make(chan Result, 10)
+	require.NoError(t, chk.Check(context.Background(), results, nil))
+
+	byPath := map[RelFilePath]Result{}
+	for r := range results {
+		byPath[r.Path] = r
+	}
+
+	assert.Equal(t, StatusOK, byPath[testFile1].Status)
+	assert.Equal(t, Result{
+		Path:    listedPipe,
+		Status:  StatusError,
+		Message: "not a regular file",
+	}, byPath[listedPipe])
+
+	extras := make(chan Result, 10)
+	require.NoError(t, chk.FindExtraFiles(context.Background(), extras))
+
+	var extraPaths []RelFilePath
+	for r := range extras {
+		extraPaths = append(extraPaths, r.Path)
+	}
+
+	assert.Empty(t, extraPaths)
+}

mfer/scanner.go

@@ -474,6 +474,13 @@ func (s *Scanner) enumerateFileWithInfo(
 		info = realInfo
 	}
 
+	// Opening a named pipe can block, and reading a device may never end.
+	if !info.Mode().IsRegular() {
+		log.Verbosef("skipping %s: not a regular file", cleanPath)
+
+		return nil
+	}
+
 	for _, excluded := range s.excluded {
 		if os.SameFile(info, excluded) {
 			return nil