trailofbits / trailofbits/necessist

`SOURCE_FILES` is never invalidated, so spans can be resolved against stale contents

Open
#1,963 0 comments 0 reactions 0 assignees View on GitHub

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

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.