fix(fspy): tolerate torn shared-memory frames when collecting traced accesses#545
Closed
wan9chi wants to merge 2 commits into
Closed
fix(fspy): tolerate torn shared-memory frames when collecting traced accesses#545wan9chi wants to merge 2 commits into
wan9chi wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 43da79ec8d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Add a `vtt write-garbage-fspy-frame` helper that connects a sender to the running task's own fspy channel — through the channel's public `Sender` API, exactly like any traced process — and writes one frame whose content doesn't decode as a `PathAccess`. This reconstructs the in-bounds variant of the frame-stream corruption described in issue 544 (a dying or racing writer leaving a frame the reader misparses), without touching the shared memory directly. The helper finds the channel through the payload fspy hands every traced process. The new fspy_garbage_frame e2e fixture runs such a task twice: today `vp run` panics while collecting the traced accesses after the task itself succeeded — the crash-after-success user impact of issue 544. The snapshot captures the crash to demonstrate the bug; the follow-up commit makes the reader degrade gracefully and updates the snapshot. The out-of-bounds skip misparse from the issue's backtrace requires a mid-claim writer interleaving that no API sequence can produce quiescently; the follow-up commit covers that arm byte-exactly with unit tests in shm_io.rs. The fixture is skipped on musl because the shared-memory channel does not exist there (fspy uses seccomp-unotify arenas instead). Issue: #544 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017ps7hxpYTqwk3t9qnPgyK8
43da79e to
015c674
Compare
`ShmReader::iter_frames` now stops (instead of panicking with a slice out-of-bounds) when a frame's recorded extent overruns the used region — the misparse a dying or racing writer leaves behind (issue 544's `1919954688 = "\0/pr"` path bytes read as a frame header) — and reports it via `is_tail_truncated()`. The new shm_io unit tests craft that byte state exactly. A frame that fails to decode as a `PathAccess` ends the iteration the same way instead of unwrapping. The truncation propagates through `PathAccessIterable::is_truncated` and `TrackedPathAccesses` to `update_cache`, which skips caching with a new `TrackingIncomplete` reason — mirroring `FspyUnsupported` — because the inferred inputs/outputs may be missing entries and a cache entry built from them could produce stale hits. The summary shows "Not cached: file access tracking data was incomplete". The fspy_garbage_frame e2e snapshot now proves the graceful path: the run exits 0 with the not-cached notice, and the second run stays a cache miss instead of hitting a bogus entry. Closes #544 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017ps7hxpYTqwk3t9qnPgyK8
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
Closes #544.
vp runcan crash while collecting fspy path accesses after the task itself completed successfully. Issue 544's backtrace shows the shared-memory frame reader misparsing frame content as a frame header (1919954688 = 0x72702F00is exactly"\0/pr"from an absolute path) and panicking on the resulting out-of-bounds skip. The same corruption class has an in-bounds variant the issue calls out explicitly: a misparsed frame that decodes into garbagePathAccessrecords — or fails to decode and panics theunwrap()on the same collection path. Either way the run aborts (or the cache silently ingests incomplete data) even though the task succeeded.The live race window behind the misparse is nanoseconds wide (see the issue's fresh-relink correlation), so the e2e reproduction feeds the corruption through the channel's own public
SenderAPI instead of racing: a well-formed frame whose content is not a decodablePathAccess. No direct shared-memory writes are involved; the helper connects a sender to the task's own channel exactly like any traced process does, via the payload fspy hands every traced process. The out-of-bounds misparse from the issue's backtrace cannot be produced quiescently by any API sequence (the writer stores the header before any content, and every claimed extent is covered by the end offset), so that arm is covered byte-exactly by unit tests instead.Approach
Two commits, meant to be read in order:
test(e2e): reproduce fspy channel garbage-frame crash (#544)— adds avtt write-garbage-fspy-framehelper and thefspy_garbage_framee2e fixture, which runs such a task twice. The snapshot at this commit captures the crash-after-success (panic initer_path_accesses, exit 101 on both runs). CI on this commit is green with the panic snapshot, demonstrating the bug is reproduced stably on every platform with the shared-memory channel (the fixture is skipped on musl, where the channel doesn't exist and fspy uses seccomp-unotify arenas instead).fix(fspy): treat unreadable shm frames as truncated instead of panicking— makes the reader defensive, following the issue's suggestion:ShmReader::iter_framesstops (instead of panicking) when a frame's recorded extent overruns the used region, and reports it viais_tail_truncated(); new shm_io unit tests craft the issue's exact byte state ("\0/pr"read as a 1.9 GB frame header) and assert the truncation;unwrap()-panicking;update_cache, which skips caching with a newTrackingIncompletereason — mirroring howFspyUnsupporteddegrades — because the inferred inputs/outputs may be missing entries and a cache entry built from them could produce stale hits;→ Not cached: file access tracking data was incomplete.The e2e snapshot is updated to prove it: the run now exits 0 with the not-cached notice, and the second run stays a cache miss (nothing bogus was cached).
The root-cause audit of receiver-lock coverage (which descendants can write without holding the shared flock) and acquire-ordering for a non-quiescent reader remain follow-up work per the issue; this PR removes the crash and the unsound caching path regardless of which hypothesis produces the corrupted frame stream.
🤖 Generated with Claude Code
https://claude.ai/code/session_017ps7hxpYTqwk3t9qnPgyK8