fix(identity): reject a truncated WAV fmt chunk instead of indexing past the end - #901
fix(identity): reject a truncated WAV fmt chunk instead of indexing past the end#901minion1227 wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe WAV reader now detects ChangesWAV truncation validation
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/genie-core/src/voice/identity.rs`:
- Around line 864-867: Update the test around read_wav_mono_f32 to inspect the
returned error rather than only asserting result.is_err(). Verify that the error
text or equivalent context identifies the truncated fmt chunk as an invalid WAV
fmt chunk, while preserving the existing cleanup and failure behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e46d6edd-c59e-4194-9502-f83e3b8c578e
📒 Files selected for processing (1)
crates/genie-core/src/voice/identity.rs
Review follow-up: the regression test accepted any `Err`, so a change that skipped the malformed chunk instead of rejecting it would still pass — the later "expected 16-bit PCM" bail would satisfy it without exercising this guard. Assert the error text instead. Re-verified it still fails against the old `chunk_end > data.len()` condition with the original out-of-bounds panic.
|
Fair point — taken in 6ed6f6a. You're right that Now asserts the error text: let error = result.expect_err("a truncated fmt chunk must be an error");
assert!(error.to_string().contains("invalid WAV fmt chunk"), "expected the fmt-chunk rejection, got: {error}");Re-verified in both directions: passes with the fix, and still FAILS against the old |
Summary
read_wav_mono_f32panics with an out-of-bounds index on a WAV whosefmtchunk declares 16 bytes but is truncated by the end of the file. The guard meant to catch that can never fire.The dead guard
chunk_endis clamped with.min(data.len())two lines earlier, sochunk_end > data.len()is always false. Afmtchunk that declares 16 bytes near the end of the file therefore passes the guard and the fixed-offset reads below index past the end.Replaced with a check against the bytes actually present:
The
datachunk keeps its tolerant clamping — a slightly short recording is still usable, and the sample loop is already bounded bycursor + frame_bytes <= end.Real Behavior Proof
Tested profile / hardware (check all that apply):
jetsonraspberry_piportable_sbclaptopmacWhat I ran
Linux x86_64 laptop. This is pure byte parsing with no device dependency, so the laptop exercises it fully.
I confirmed the panic before fixing it, by writing the regression test first and running it against unmodified
main:What I observed
Against unmodified
main, a 44-byte input built fromRIFF/WAVE+ a 10-byteJUNKchunk + afmtheader declaring 16 bytes with only 6 present:Note the input is exactly 44 bytes, so it clears the
data.len() < 44gate at the top of the function — the length check does not protect these reads.With the fix, the same input returns
Err("invalid WAV fmt chunk"). I re-verified the test is load-bearing by restoring the oldchunk_end > data.len()condition, confirming FAILED with the same out-of-bounds message, then restoring the fix and confirming pass.cargo test -p genie-core --lib voice::identity— 8 passed.cargo test --workspace— green, 0 failures.-D warnings(workspace and--no-default-features); fmt clean.Reachability
enroll_speaker_fileandidentify_speaker_fileboth parse a WAV from a path, so this is reachable viagenie-ctl speaker enroll <file>with a corrupt file, and by the daemon on any recording truncated by an interrupted write or a full disk. A panic there aborts the enrolling/identifying task rather than returning the error the caller already handles.Test plan
cargo test -p genie-core --lib truncated_fmt_chunkRestoring
chunk_end > data.len()in place ofpresent < 16reproduces the panic.Notes for reviewers
voice/vad.rs::trim_wavandvoice/aec.rsboth guarddata.len() <= 44before slicing, so neither has this problem and I left them alone rather than widen the diff on paths that are already correct.fmtreads are at fixed offsets, so requiring 16 present bytes is the whole precondition; I did not make the chunk walk strict in general, because the tolerantdata-chunk clamping is load-bearing for real recordings.Summary by CodeRabbit
fmtchunks; these files now return a clearinvalid WAV fmt chunkerror.