trailofbits / trailofbits/necessist
`SOURCE_FILES` is never invalidated, so spans can be resolved against stale contents
Nobody has claimed this yet.
- Dominant language
- Rust
- Stars
- 145
- Forks
- 21
- Avg merge
- 10h 58m
- Merged PRs (30d)
- 41
Description
Every source file is read twice, and nothing keeps the two reads in agreement.
ParseAdapter::parse calls SourceFile::new, which reads the file and caches the contents in the thread-local SOURCE_FILES map, leaking them for the lifetime of the thread (source_file.rs#L15-L17, #L55-L74). It then hands the path to ParseLow::parse_source_file, so each backend reads the file again — e.g. rust/mod.rs#L418. Candidate spans carry coordinates from the second read; Span::source_text slices the first, using an offset calculator built on it.
The map is never cleared, so the window is not just the gap between the two reads within one parse: a second necessist() call on the same thread resolves its spans against the first call's contents.
Three consequences follow.
Wrong text, silently
When the stale offsets stay in bounds, source_text returns Ok holding text the span does not point at. Two necessist() calls with a one-character edit in between, both with dump_candidates:
--- run 1 (file says `n += 1;`) ---
.../src/lib.rs:4:5-4:12: `n += 1;`
--- run 2 (file says `n += 2;`) ---
.../src/lib.rs:4:5-4:12: `n += 1;`
Run 2 exits 0. In a real run that text is what emit records for the removal (core.rs#L376, #L423), so necessist.db and the console both describe a removal that did not happen.
Out-of-range offsets
Addressed by #1961: source_text returns an error instead of indexing past the contents. The error aborts the run.
Removals write stale contents
Span::remove builds its Rewriter over the cached contents and writes the result to the file (span.rs#L171-L187), so a removal replaces the file with the cached version minus the span, discarding any intervening change. Backup restores the on-disk state afterwards, but the test runs against content that is not what is on disk.
Possible fixes
Changing ParseLow::parse_source_file to take contents rather than a path removes the second read entirely, at the cost of touching every backend. Clearing SOURCE_FILES at the start of each necessist() call closes only the across-runs window, which is the one that is trivially reachable. The two are independent.
#1961 documented the hazard on source_text and made the loud half an error; this issue is the cause.
Written by Claude Code (Opus 5), filed from @smoelius's account.
Contributor guide
No contributing guide indexed for this repository
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Trace ParseAdapter::parse in backends/src/parsing.rs, SourceFile and SOURCE_FILES in core/src/source_file.rs, and Span::source_text and Span::remove in core/src/span.rs. Reproduce two necessist() calls with an intervening source edit and inspect the existing dump_candidates behavior. Done means spans and removals use current file contents without stale-cache results or discarded edits.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- rust
- Domain
- devtools, testing
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 52/100