diff --git a/crates/librarian-app/src/main.rs b/crates/librarian-app/src/main.rs index f7de611..3c4e954 100644 --- a/crates/librarian-app/src/main.rs +++ b/crates/librarian-app/src/main.rs @@ -837,6 +837,14 @@ impl Librarian { } } + /// Adopt the current location's saved column layout and drop the cached + /// auto-fit measurements (refilled by `recompute_rows`/`remeasure_details` + /// once the new rows land). Called wherever a load commits to a location. + fn adopt_column_layout(&mut self) { + self.col_layout = self.column_layout_for(self.history.current()); + self.col_measure = ColumnMeasure::default(); + } + /// Begin dragging `col`'s divider: record the grab point and the column's /// current resolved width, so motion maps to `start_width + delta`. fn begin_divider_drag(&mut self, col: Column) { @@ -1172,11 +1180,9 @@ impl Librarian { } self.load_pending = false; // The previous folder was kept on screen (under the loading - // overlay) during the read, so its column widths didn't pinch; - // now adopt the new folder's saved layout and drop the stale - // measurements before `recompute_rows` re-measures. - self.col_layout = self.column_layout_for(self.history.current()); - self.col_measure = ColumnMeasure::default(); + // overlay) during the read, so its columns didn't pinch; now commit + // to the new location's columns before `recompute_rows` re-measures. + self.adopt_column_layout(); match result { Ok(entries) => { self.content = Content::Folder { entries }; @@ -1224,8 +1230,7 @@ impl Librarian { // doesn't think a load is still owed. An ordinary in-place // refresh (no pending load) keeps the current columns. if std::mem::take(&mut self.load_pending) { - self.col_layout = self.column_layout_for(self.history.current()); - self.col_measure = ColumnMeasure::default(); + self.adopt_column_layout(); } let previously = self.selected_paths(); self.content = Content::Folder { entries }; @@ -1701,17 +1706,13 @@ impl Librarian { let load = match location { Location::ThisPc => { - // Landing pages carry no persisted per-folder layout; reset to the - // default and let their (instant) load re-measure. - self.col_layout = ColumnLayout::default(); - self.col_measure = ColumnMeasure::default(); + self.adopt_column_layout(); self.content = Content::default(); self.rows.clear(); Task::perform(offload(list_drives), Message::ThisPcLoaded) } Location::Wsl => { - self.col_layout = ColumnLayout::default(); - self.col_measure = ColumnMeasure::default(); + self.adopt_column_layout(); self.content = Content::Wsl { distros: Vec::new(), }; @@ -3658,11 +3659,17 @@ fn window_for( } let first = (scroll.max(0.0) / row_h).floor() as usize; let onscreen = (viewport.max(0.0) / row_h).ceil() as usize + 1; - let start = first.saturating_sub(overscan); let end = first .saturating_add(onscreen) .saturating_add(overscan) .min(count); + // Clamp `start` to `end`, not just to `count`: if the list shrank under a + // stale scroll offset (an external change deleted rows while scrolled down), + // `first` can sit past the new end, leaving an un-clamped `start` above + // `count`. That violates the `0..count` contract and yields an *inverted* + // `start..end` that panics the instant a caller slices `rows[start..end]` + // (the grid does exactly that). Keep it a well-formed, possibly-empty range. + let start = first.saturating_sub(overscan).min(end); (start, end) } @@ -3996,6 +4003,107 @@ mod tests { let _ = std::fs::remove_dir_all(&dir); } + /// Regression guard for the "File Pilot crash class" (see + /// `docs/crash_class_review_filepilot.md`): a directory that *churns* — a + /// file rapidly created and deleted, exactly as a `.lock` file does — must + /// never leave a stale row index that later indexes out of bounds. That C + /// crash wrote through a NULL aggregate because an accumulation loop was + /// guarded by a cached item count, not by the destination's validity. + /// Librarian rebuilds rows from scratch on every change and reconciles + /// indices with [`Selection::retain_below`] plus the [`window_for`] clamp; + /// this drives a real create/delete burst — through the actual notify + /// watcher — and on every re-enumeration runs that reconciliation, carrying + /// selection + scroll from a *larger* prior listing into the new one. No + /// produced index may fall outside the current rows, however the count moved + /// between reads, and nothing may panic. + #[test] + fn rapid_churn_never_leaves_a_stale_row_index() { + use notify_debouncer_mini::{new_debouncer, notify::RecursiveMode}; + + let mut dir = std::env::temp_dir(); + dir.push(format!("librarian-churn-{}", std::process::id())); + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir_all(&dir).unwrap(); + + // A baseline of stable entries, so there's always a listing to hold a + // selection within while the lock file flickers in and out beside them. + for i in 0..8 { + std::fs::write(dir.join(format!("stable-{i}.txt")), b"x").unwrap(); + } + + // Arm the real watcher with the same settings `watch_stream` uses, so the + // burst runs through the genuine notify/debouncer pipeline. We don't + // assert on its timing (that's `notify_detects_directory_changes`) — + // `_rx` just keeps the channel alive for the duration. + let (tx, _rx) = std::sync::mpsc::channel(); + let mut debouncer = new_debouncer(Duration::from_millis(50), tx).expect("debouncer"); + debouncer + .watcher() + .watch(&dir, RecursiveMode::NonRecursive) + .expect("watch"); + + // Hammer a lock-style file create/delete from a background thread — the + // exact File Pilot trigger (`.claude.json.lock` churn). + let churn_dir = dir.clone(); + let churn = std::thread::spawn(move || { + let lock = churn_dir.join(".churn.lock"); + for _ in 0..400 { + let _ = std::fs::write(&lock, b""); + let _ = std::fs::remove_file(&lock); + } + }); + + // Mirror `recompute_rows`: take a selection/lead that was valid for a + // *previous* (possibly longer) listing, then reconcile it to the freshly + // enumerated one. `read_dir_all` itself must also tolerate an entry + // vanishing mid-scan (it skips a failed `metadata()` — the librarian-side + // analog of the entry "appearing/disappearing as the lock file churned"). + let mut prev_len = 0usize; + for _ in 0..400 { + let rows = read_dir_all(&dir).expect("enumerate churning dir"); + let len = rows.len(); + + // A selection established against the previous count, reconciled to + // the new one. Every surviving index must be a valid `rows` index. + let mut selection = Selection::default(); + selection.select_all(prev_len); + selection.retain_below(len); + for i in selection.iter() { + assert!(i < len, "retain_below left stale index {i} (len {len})"); + let _ = &rows[i]; // panics if `retain_below` ever stops clamping + } + assert!(selection.lead().is_none_or(|l| l < len)); + + // The virtualized render window, computed from a scroll offset that + // was valid for the *old* length, must clamp to the current rows — + // this is the slice `view` indexes directly at `self.rows[start..end]`. + let stale_scroll = prev_len as f32 * ROW_HEIGHT; + let (start, end) = visible_window(stale_scroll, 600.0, len); + assert!( + end <= len && start <= end, + "window {start}..{end} out of 0..{len}" + ); + let _ = &rows[start..end]; // panics if `window_for` ever stops clamping + + // Hardening: an *extreme* stale offset/selection (as if the prior + // listing had been vastly larger) must clamp just the same, so the + // guard holds even on a run where the churn timing barely moved `len`. + let mut huge = Selection::default(); + huge.select_all(len + 10_000); + huge.retain_below(len); + assert!(huge.iter().all(|i| i < len)); + let (s, e) = visible_window(1_000_000.0, 600.0, len); + assert!(e <= len && s <= e); + let _ = &rows[s..e]; + + prev_len = len; + } + + churn.join().unwrap(); + drop(debouncer); + let _ = std::fs::remove_dir_all(&dir); + } + #[test] fn visible_window_renders_only_the_viewport_plus_overscan() { // A 1000-row list, ~25 rows visible (600px / 24px). @@ -4022,6 +4130,16 @@ mod tests { // Empty list yields an empty window. assert_eq!(visible_window(0.0, viewport, 0), (0, 0)); + + // Stale scroll past the end of a shrunk list: the window must stay a + // well-formed, in-bounds range (`start <= end <= count`), never inverted. + // This is the `0..count` contract that keeps `rows[start..end]` panic-free + // after an external change deletes rows while scrolled down. + let (start, end) = visible_window(10_000.0 * ROW_HEIGHT, viewport, 5); + assert!( + start <= end && end <= 5, + "got {start}..{end}, must be within 0..5" + ); } #[test] diff --git a/docs/crash_class_review_filepilot.md b/docs/crash_class_review_filepilot.md new file mode 100644 index 0000000..00480fd --- /dev/null +++ b/docs/crash_class_review_filepilot.md @@ -0,0 +1,175 @@ +# Crash-class review: is Librarian prone to the File Pilot NULL-write bug? + +**Date:** 2026-06-17 +**Branch:** `feat-phase2` +**Scope:** A defensive review prompted by an *external* crash. A minidump from +File Pilot (Voidstar's file explorer, written in C) was analyzed separately; this +doc records whether Librarian — a different file explorer, in Rust — is exposed to +the **same class** of bug, what makes it (mostly) immune, and the one latent +instance the review found and fixed. + +This was a **crash-class** review, not a general bug hunt. The conclusion: Librarian +is structurally resistant to that class, the worst realistic outcome is a bounded +panic rather than memory corruption, and one latent footgun in the exact class was +hardened (`window_for`) with a regression test added. + +--- + +## The File Pilot bug (the thing we're checking for) + +File Pilot crashed with an access violation — a **write through a NULL pointer** — +in its per-directory **statistics roll-up**: + +- It maintains a long-lived per-name aggregate keyed by an **open-addressing hash + map**. +- On a **lookup miss** it sets the destination "aggregate record" pointer to NULL + as a sentinel. +- The accumulation loop that follows is guarded **only by a cached child-item + count** (`count == 32`), *not* by whether the destination is valid, so it runs + and executes `add qword ptr [rsi+30h], rax` with `rsi = 0` → write to `0x30` → + `0xC0000005`. + +**Trigger:** a tab open on a directory containing `.claude.json.lock`, a lock file +an external tool creates and deletes very rapidly. The churn drove a stats update +for an entry whose name was momentarily absent from the aggregation map → miss → +NULL destination → unguarded count-driven loop → NULL write. + +### The bug class, abstracted + +Two ingredients are required: + +1. **A long-lived aggregate that is mutated incrementally** as the directory + changes (rather than recomputed from scratch). +2. **A loop/index guarded by a separately-tracked count or stale offset**, instead + of by the validity/bounds of the destination it actually writes to. + +When the two drift apart — count says "32 items", destination says "absent" — you +get a write to an invalid location. + +--- + +## Why Librarian is structurally resistant + +**1. It recomputes from scratch; it never mutates a persistent aggregate.** +On any external change the watcher emits `Message::DirChanged`, which re-enumerates +the whole directory off-thread (`crates/librarian-app/src/main.rs`, `DirChanged` +handler ~L1211); `Message::Reloaded` then rebuilds *all* derived state via +`recompute_rows()` (~L2003). There is no per-directory stats structure being +incrementally patched as files churn, so there is no aggregate to fall out of sync. +This is the single biggest reason ingredient (1) is absent. + +**2. Reconciliation is unconditional.** `recompute_rows()` always calls +`self.selection.retain_below(self.rows.len())` (~L2034), which drops any +selection / lead / anchor index that no longer exists +(`selection.rs::retain_below`). The count and the data are rebuilt together from +the same source in the same step. + +**3. Indexing is guarded by the destination, not a stale count.** Every raw index +into `self.rows` is clamped or guarded against the *current* length: +- list view `&self.rows[index]` — `index ∈ start..end` from `visible_window`, + whose `end` is `.min(count)`; +- grid thumbs `self.rows[start..end]` — both ends `.min(self.rows.len())`; +- grid tiles `&self.rows[i]` — explicit `if i >= total { break; }` first. + +Everywhere else lookups use `self.rows.get(i)` → `Option`, and the one `HashMap` +lookup that matters degrades safely on miss: +`self.col_store.get(path).copied().unwrap_or_default()`. + +**4. Language backstop.** Even if a stale index slipped through, safe Rust turns it +into a **bounded `index out of bounds` panic**, not a wild write into a struct +field. The data/enumeration/stats paths contain no `unsafe`, no raw-pointer +arithmetic (`model.rs` documents "no `unsafe`"). The C failure mode — `add [rsi+30h]` +with `rsi = 0` — is simply not expressible there. The `windows`-facing crate has +`unsafe`, but it is not on the watcher/enumeration path. + +**5. The trigger is blunted, too.** The watcher is **debounced at 200 ms and +non-recursive** (`watch_stream`, ~L3566); each refresh supersedes the previous via +a load token (~L1223). A `.claude.json.lock`-style storm collapses into a single +re-enumeration, and a file that vanishes mid-scan is skipped, not fatal +(`enumerate.rs` — a failed `DirEntry::metadata()` → `continue`). + +--- + +## The closest analog in Librarian + +The Rust-shaped version of "count drifted from destination" is a **stale `usize` +index into `self.rows` that survives a listing shrink**: selection / lead / scroll +/ render-window indices computed against a *larger* prior listing, used after an +external change shrank it. Reasons (1)–(3) above keep all of these reconciled, so +there is no live panic on the current call sites. + +### Latent instance found and fixed: `window_for` + +While building the regression test, an extreme-stale-offset assertion failed and +surfaced a real latent footgun. `window_for` (which backs `visible_window`) +clamped `end` to `count` but **not** `start`: + +```rust +// before +let start = first.saturating_sub(overscan); +let end = first.saturating_add(onscreen).saturating_add(overscan).min(count); +``` + +When the list shrinks under a **stale scroll offset** (an external change deletes +rows while you're scrolled down), `first` can sit past the new end, leaving `start` +above `count`. That: + +- **violates the function's own documented `0..count` contract**, and +- produces an **inverted `start..end`** that panics the instant a caller *slices* + `rows[start..end]` — which the grid view does. + +It was not a live panic *today*: the list view *iterates* `start..end` (harmlessly +empty when inverted), and the grid computes its own separately-clamped range. But +it is exactly the "derived index not reconciled to the current count" shape we were +reviewing for — a panic waiting for the next caller that slices. Fixed by clamping +`start` to `end`: + +```rust +// after +let end = first.saturating_add(onscreen).saturating_add(overscan).min(count); +let start = first.saturating_sub(overscan).min(end); // start <= end <= count +``` + +Now the window is always a well-formed, in-bounds, possibly-empty range. + +--- + +## Regression guard added + +`crates/librarian-app/src/main.rs`, test module: + +- **`rapid_churn_never_leaves_a_stale_row_index`** — arms the *real* notify watcher + (non-recursive, the same settings `watch_stream` uses) on a temp dir, hammers a + `.churn.lock` create/delete burst from a background thread (the exact File Pilot + trigger), and on every re-enumeration runs the app's reconciliation + (`Selection::retain_below` + the `visible_window`/`window_for` clamp), carrying + selection + scroll from a *larger* prior listing into the new one. It asserts no + produced index ever falls outside the current rows and nothing panics, however + the count moves between reads. It also exercises `read_dir_all` tolerating an + entry vanishing mid-scan. +- **`visible_window_renders_only_the_viewport_plus_overscan`** — extended with a + pure-arithmetic case proving the `0..count` contract holds for a stale scroll + offset past the end of a shrunk list (`start <= end <= count`, never inverted). + +Both pass; full `librarian-app` suite green (51 tests), `cargo fmt --all --check` +and `cargo clippy -p librarian-app --all-targets` clean. + +--- + +## Verdict + +Librarian is **not susceptible** to File Pilot's bug class. It rebuilds derived +state wholesale instead of mutating a long-lived aggregate, reconciles indices +unconditionally, guards every raw index against the current length, and runs in +safe Rust where the worst outcome is a panic, not memory corruption. The one latent +footgun in the class (`window_for`'s un-clamped `start`) has been fixed and pinned +with tests. + +### Future-risk watch + +The immunity comes from the *recompute-from-scratch* design. The moment a feature +mutates a **persistent per-directory aggregate incrementally from watcher events** — +e.g. live folder-size totals, per-extension counts, or a running selection summary — +ingredient (1) reappears and this bug class is back in play. If that is ever added: +guard the accumulation on the **destination's existence/bounds**, not on a cached +item count, and re-run the churn test against it.