JMR-dev/librarian
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Harden virtualized window against stale scroll; guard crash class

Browse Source 
A defensive review (prompted by an external File Pilot NULL-write crash in
its per-directory stats roll-up) checked whether Librarian shares that bug
class: a derived index guarded by a stale count/offset rather than the
destination's bounds.

Found and fixed one latent instance: `window_for` clamped `end` to `count`
but not `start`, so a list that shrank under a stale scroll offset could
yield an inverted `start..end` that violates the documented `0..count`
contract and panics any caller that slices `rows[start..end]` (the grid
does). Clamp `start` to `end` so the window is always well-formed.

- Add `rapid_churn_never_leaves_a_stale_row_index`: drives a real
  create/delete burst (the `.lock` trigger) through the notify watcher and
  asserts reconciliation (`retain_below` + the window clamp) never leaves an
  out-of-bounds row index.
- Extend `visible_window_*` with a stale-scroll-past-end contract case.
- Document the analysis in docs/crash_class_review_filepilot.md.

Full librarian-app suite green (51); fmt + clippy clean.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
JMR-dev
2026-06-17 11:51:45 -05:00
co-authored by Claude Opus 4.8
parent de6499e09f
commit afd23ed2c1
2 changed files with 307 additions and 14 deletions
Download Patch File Download Diff File Expand all files Collapse all files
crates/librarian-app/src/main.rs
+132 -14
View File
@@ -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]
docs/crash_class_review_filepilot.md
+175
View File
@@ -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.