fix: read grep content into an owned buffer to avoid SIGBUS on concurrent file modification by kwojcik-blockether · Pull Request #626 · dmtrKovalenko/fff

kwojcik-blockether · 2026-06-22T09:32:30Z

Problem

Grep can SIGBUS and abort the entire host process when a file is modified while it is being searched.

FileItem::get_content_for_search returns file content via mmap — a per-FileItem OnceLock<Mmap> cache, plus a fresh mmap for files >= FRESH_MMAP_THRESHOLD (introduced in #533). mmap-backed reads are unsound under concurrent modification: when an editor or a coding agent rewrites/truncates a file between or during greps — the normal case for the Neovim picker and the MCP agent — the mapping's pages past the new EOF become invalid, and the next read (including the SIMD prefilter case_insensitive_memmem::search_packed_pair scanning the whole file) faults with SIGBUS, killing the process. A SIGBUS from an invalid mapping cannot be caught from safe Rust, so there is no recovery.

The cached mapping makes it worse: an OnceLock<Mmap> created at index time can be served long after the file changed on disk.

This surfaced in production as:

SIGBUS (0xa) ...
C  [libfff_c.dylib]  fff_search::case_insensitive_memmem::search_packed_pair
C  [libfff_c.dylib]  fff_search::grep::grep_search

Root cause, not the search kernel

To rule out a real over-read in the SIMD memmem, I added a guard-page unit test (case_insensitive_memmem::tests::guard_page_no_overread): it places haystacks ending exactly at a PROT_NONE page and fuzzes lengths 1..600 × needle 2..48. It passes — the kernel never reads past a valid slice. The fault is the invalid mapping, not the search.

Fix

Read grep content into the existing per-worker scratch Vec<u8> instead of mmap-ing it. The buffer is reused across files (one bounded copy per file, no per-query allocation) and is immune to truncation — a shrinking file simply yields its new, shorter bytes.

Reproduction (deterministic)

Added grep_reflects_file_rewrite_without_stale_mmap: index a ~2 MB file, grep it, rewrite it much shorter with the needle removed (no re-index), grep again.

Before: the second grep returns the stale cached match, or — once the file shrinks past a page boundary — aborts with process didn't exit successfully ... (signal: 10, SIGBUS: access to undefined memory).
After: the grep correctly reflects the new content (0 matches).

Tradeoff

Grep content is no longer zero-copy mmap. If the repeatable-large-file perf from #533 matters, the cache could instead hold an owned Box<[u8]> (read once, invalidated when the file's size/mtime changes) — same speedup, still crash-safe. Happy to follow up that way if you prefer.

Tests

cargo test -p fff-search — all green (94 lib + integration), including the new regression and guard-page tests.

🤖 Generated with Claude Code

…rent modification Grep reads each candidate file's content through mmap — a per-FileItem `OnceLock<Mmap>` cache plus a fresh mmap for files >= FRESH_MMAP_THRESHOLD (dmtrKovalenko#533). mmap'd reads are unsound when the file changes concurrently: when an editor or coding agent rewrites/truncates a file between (or during) greps — the common case for the nvim picker and the MCP agent — the mapping's pages past the new EOF become invalid and the next read, including the SIMD prefilter scanning the whole file, faults with SIGBUS and aborts the entire host process. The cached mapping makes it worse: a map created at index time can be served long after the file changed on disk. Read grep content into the existing per-worker scratch `Vec<u8>` instead. It is reused across files (one bounded copy per file, no per-query allocation) and is immune to truncation — a shrinking file simply yields its new, shorter bytes. Adds a deterministic regression test (grep_reflects_file_rewrite_without_stale_mmap): index a ~2MB file, grep it, rewrite it much shorter with the needle removed, grep again. Before this change the second grep returns the stale cached match or, once the file shrinks past a page boundary, SIGBUS-aborts the process (signal: 10); after it, the grep correctly reflects the new content. Also adds a guard-page unit test confirming the case-insensitive SIMD memmem never over-reads a valid slice — the fault is the invalid mapping, not the search kernel. Tradeoff: grep content is no longer zero-copy mmap. If the repeatable-large-file perf from dmtrKovalenko#533 matters, the cache could instead hold an owned Box<[u8]> (read once, invalidated on change) — same speedup, still crash-safe; happy to follow up that way if preferred. Co-Authored-By: Claude <noreply@anthropic.com>

Copilot

Pull request overview

This PR makes grep crash-safe under concurrent file rewrites/truncation by avoiding mmap-backed reads (which can SIGBUS when pages past a new EOF are touched) and instead reading grep input into an owned per-worker buffer. It also adds regression coverage for the stale-mmap scenario and a guard-page test to validate the SIMD prefilter doesn’t over-read beyond slice bounds.

Changes:

Switch FileItem::get_content_for_search to read grep content into a reusable Vec<u8> instead of using cached/fresh mmaps.
Add an integration regression test that rewrites a large indexed file between greps and asserts the second grep reflects new content.
Add a guard-page unit test to ensure search_packed_pair never reads past the haystack slice end.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File	Description
crates/fff-core/src/types.rs	Stop using mmap for grep content; read into reusable buffer instead.
crates/fff-core/tests/grep_integration.rs	Add regression test covering file rewrite between greps without reindex.
crates/fff-core/src/case_insensitive_memmem.rs	Add guard-page test to validate SIMD prefilter doesn’t over-read.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

        let mut file = std::fs::File::open(&abs).ok()?;
-        file.read_exact(buf).ok()?;
+        buf.clear();
+        file.read_to_end(buf).ok()?;
        Some(buf.as_slice())


+        // Read the file into an owned, reusable buffer rather than mmap it.
+        //
+        // mmap-backed grep races with concurrent modification: if the file is
+        // truncated or rewritten while a mapping is live (an editor or coding
+        // agent changing the workspace), the pages past the new EOF become
+        // invalid and the next read — including a SIMD prefilter load over the
+        // whole file — faults with SIGBUS, aborting the entire process. The old
+        // cached fast path made it worse: an `OnceLock<Mmap>` created at index
+        // time could be served long after the file changed on disk. An owned
+        // read() copy is immune; a shrinking file just yields a shorter buffer,
+        // never a fault. `buf` is a per-worker scratch Vec reused across files,
+        // so this is one bounded copy per file, not a per-query allocation. (The
+        // content cache stays warm for the file picker via `get_cached_content`;
+        // only the grep reader stopped trusting a possibly-stale mapping.)


+    #[test]
+    fn guard_page_no_overread() {
+        // Place each haystack so it ENDS exactly at a PROT_NONE guard page; any


… 0.9.6-2 fff's mmap-backed grep reader faults with SIGBUS when a file is rewritten mid-search (an editor/coding agent editing the workspace) — see upstream PR dmtrKovalenko/fff#626. Point the native build at the Blockether/fff fork @ v0.9.6-blockether.1, which carries the fix (read grep content into an owned buffer instead of mmap), and cut 0.9.6-2. Revert FFF_REPO to dmtrKovalenko/fff once #626 merges. Co-Authored-By: Claude <noreply@anthropic.com>

…m comment - get_content_for_search: read_to_end was unbounded — a file that grew past max_file_size after indexing (self.size is the stale indexed size) could balloon memory. Read at most max_file_size+1 via Read::take and bail if it exceeds the budget. - guard_page_no_overread: uses libc mmap/mprotect — gate #[cfg(unix)] so the Windows build's `cargo test` still compiles. - Condense the hot-path comment to a short SIGBUS rationale. Co-Authored-By: Claude <noreply@anthropic.com>

Copilot AI review requested due to automatic review settings June 22, 2026 09:32

Copilot started reviewing on behalf of kwojcik-blockether June 22, 2026 09:33 View session

Copilot AI reviewed Jun 22, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: read grep content into an owned buffer to avoid SIGBUS on concurrent file modification#626

fix: read grep content into an owned buffer to avoid SIGBUS on concurrent file modification#626
kwojcik-blockether wants to merge 2 commits into
dmtrKovalenko:mainfrom
kwojcik-blockether:fix/grep-content-mmap-sigbus

kwojcik-blockether commented Jun 22, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

kwojcik-blockether commented Jun 22, 2026

Problem

Root cause, not the search kernel

Fix

Reproduction (deterministic)

Tradeoff

Tests

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants