Repository navigation
Conversation
|
@kyzer1023 is attempting to deploy a commit to the Cap Software Inc Team on Vercel. A member of the Team first needs to authorize it. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if let Some(window) = range { | ||
| return window | ||
| .end | ||
| .saturating_sub(window.start) | ||
| .saturating_mul(channels); |
There was a problem hiding this comment.
decoded_sample_capacity reserves the entire requested range, but load_track passes usize::MAX / 4 as the end when rendering near the recording's end or when its duration is unknown. Even for mono audio, this requests more bytes than Vec<f32> can hold. samples.reserve therefore panics before decoding, and the render returns an error.
Skip or cap the reservation for open-ended ranges, and add a test using this existing caller's end value.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/audio/src/audio_data.rs
Line: 326-330
Comment:
**End-of-recording renders fail**
`decoded_sample_capacity` reserves the entire requested range, but `load_track` passes `usize::MAX / 4` as the end when rendering near the recording's end or when its duration is unknown. Even for mono audio, this requests more bytes than `Vec<f32>` can hold. `samples.reserve` therefore panics before decoding, and the render returns an error.
Skip or cap the reservation for open-ended ranges, and add a test using this existing caller's end value.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
|
@greptileai review |
![superagent-security[bot]](https://avatars.githubusercontent.com/in/3287076?s=60&v=4){kind=small}
| let mut decoded_frame = ffmpeg::frame::Audio::empty(); | ||
| let mut resampler_output = ResamplerOutput::new(); | ||
| let mut samples = Vec::new(); | ||
| let _ = samples.try_reserve(decoded_sample_capacity( |
There was a problem hiding this comment.
P2: Container-reported audio duration can force unbounded decoded-sample memory reservation
New reserve trusts container duration, so a tiny crafted audio file can request a multi-GB Vec capacity before decoding any samples.
Cap the reserve by actual file size and a maximum duration/sample count before try_reserve; never use unvalidated container duration as the allocation bound.
AI prompt
Check if this security scanner issue is valid. If so, understand the root cause and fix it. If appropriate, update or add tests. Keep the change focused and preserve intended behavior.
<file name="crates/audio/src/audio_data.rs">
<violation number="1" location="crates/audio/src/audio_data.rs:109">
<priority>P2</priority>
<title>Container-reported audio duration can force unbounded decoded-sample memory reservation</title>
<evidence>The new decode path reads the demuxer-reported stream duration at line 92 and passes it to decoded_sample_capacity, which computes from_duration as duration * time_base * 48 kHz plus a 64-frame margin (lines 329-335). When no explicit range is supplied, from_file returns that unbounded value (lines 336-338), and line 109 asks the allocator to reserve it before any audio frames are decoded. The only cap is the requested window (lines 339-343); load_track deliberately supplies the open-ended usize::MAX / 4 end, so a forged container duration still controls the reservation. Duration is header metadata, not proof of file size: a small crafted FLAC, Matroska, MP3, or similar file can declare hours of audio and reach this code through desktop imports/project playback, captions, and render-farm load_track (crates/render-farm/src/audio.rs:349,361). try_reserve prevents a panic on allocation failure, but it does not prevent the large allocation request or retain-and-repeat behavior when the allocator succeeds; on allocators that commit address space this can exhaust process or shared-worker memory even though the actual decoded payload is tiny.</evidence>
<recommendation>Do not size the initial reservation from unvalidated container duration alone. Clamp the hint against the actual file/stream size and a hard maximum duration/sample count, or cap the initial reserve to a small fixed window and let the vector grow only as real decoded frames arrive. Keep try_reserve, and add a regression test with a tiny file declaring an enormous duration to prove the reservation stays bounded.</recommendation>
</violation>
</file>
There was a problem hiding this comment.
Valid. Duration is demuxer metadata, so a tiny file could still drive try_reserve after the usize::MAX/4 window cap. Fixed in c3e34b1 (now the PR head): the hint is min(duration, range, file-size bound, 8 h stereo 48 kHz cap). The file-size bound is packed-f32 samples from the fs::metadata length at 1024× (~6 kbps to 48 kHz stereo f32 is ~512×). try_reserve is unchanged.
Regression test decoded_sample_capacity_clamps_forged_duration_to_file_size: an 8-frame WAV with a forged 10-hour duration and an open-ended MAX/4 window stays at the file-size bound (<1e6 samples). The existing MAX/4 tests still pass, and the 60s WAV bench is still ~1.2×.
There was a problem hiding this comment.
Fix accepted: the developer replaced the unvalidated container-duration hint with min(duration, range, file-size bound, 8 h stereo 48 kHz cap), directly addressing the root cause. Bounding by actual fs::metadata size prevents a tiny forged file from driving a large reservation, the hard duration/sample cap protects against large legitimate files with inflated headers, try_reserve is preserved, and a regression test with an 8-frame WAV claiming 10 hours verifies the reservation stays bounded. Good-faith resolution.
Skip Vec growth during AudioData::from_file by reserving from the container duration or the requested sample window.
load_track passes usize::MAX / 4 as the end near EOF. Reserving that range overflowed Vec capacity. Cap the hint at the stream duration, skip it when duration is unknown, and use try_reserve.
Container duration is untrusted header metadata. Bound try_reserve by the file's compressed size (1024× f32 expansion) and an 8-hour stereo cap so a tiny crafted file cannot request multi-GB capacity.
4ad549e to
c3e34b1
Compare
|
@greptileai review @superagent review: please re-scan after the file-size clamp in c3e34b1 (addresses the P2 on |
This branch has not been deployed
1 participant
Summary
AudioData::from_file/from_file_rangegrew aVec<f32>from empty while decoding to packed 48 kHz, reallocating through the whole file on editor open. This reserves capacity up front, capped so it can never over-allocate or panic.Change
min(duration, requested window, file-size bound, hard max). The file-size bound is compressed bytes × 1024 packed-f32 samples (~6 kbps speech to 48 kHz stereo f32 is ~512×, so 2× margin); the hard max is 8 hours of stereo 48 kHz. A tiny file declaring hours of audio can no longer request a multi-GB reserve.try_reserveinstead ofreserve, so an oversized hint can never panicload_track's open-endedusize::MAX / 4end (unit + realfrom_file_rangedecode), invalid/NaN duration, stereo/offset start, anddecoded_sample_capacity_clamps_forged_duration_to_file_size(8-frame WAV under 1 KiB with a forged 10-hour duration and an open-ended window: reserve stays at the file-size bound, under 1e6 samples vs >3e9 unclamped)Rebased onto
main(0bc36e0d).Benchmarks
Independent run, rustc 1.88.0 (pinned),
--release, 15 runs, median:Decoded output is byte-identical before and after.
With the file-size clamp (same machine, 5 runs, median): 60s stereo WAV 42.0 ms → 36.0 ms (1.17×). The clamp doesn't bite normal files.
Machine: Intel Xeon, 4 cores, KVM; Ubuntu 24.04.5 LTS, Linux 6.12.94+.
Repro
Tests
cargo +1.88 test --locked -p cap-audio --lib: 84 passed, 2 ignored, 0 failed.
The audio change appears safe to merge; the previous open-ended reservation panic is addressed.Summary
This PR reserves space before decoding audio, using duration and the requested range. It caps the hint by file size and a fixed maximum.
Review scope: the requested three-dot diff failed because the shallow checkout had no merge base, and the previous review commit was unavailable. Review used the endpoint diff restricted to the supplied changed file,
crates/audio/src/audio_data.rs. Tests were inspected but not run.Reviews (3) · Last reviewed commit: "fix(audio): clamp decoded sample reserve..." · Reviewed by Greptile