WASAPI hardening / fixing - #1384
ErwanLegrand wants to merge 15 commits into
Conversation
… users The gate admitted all of Windows and any build with the `audioworklet` feature, but on Windows only the ASIO and JACK backends call the helper. WASAPI does not.
format_from_waveformatex_ptr cast any WAVE_FORMAT_EXTENSIBLE format to WAVEFORMATEXTENSIBLE and read SubFormat and wValidBitsPerSample from it. A format that declares the extensible tag without `cbSize` covering the extension guarantees only the 18-byte header, so the read ran 22 bytes past its end; such formats are now reported as unsupported. The extension size is pinned to its ABI value of 22 with a const assert on the windows-rs struct layout.
…ENSIBLE nAvgBytesPerSec was computed as channels * sample_rate * sample_bytes in u32 and nBlockAlign as channels * sample_bytes in u16. The probe path walks every rate in COMMON_SAMPLE_RATES up to 24.576 MHz using at most 4-byte sample formats, so a wide interface (44+ channels) overflows the first product there; a user-chosen 8-byte format reaches the same overflow with 22 channels. The unexpressible pair is now skipped while probing and rejected as UnsupportedConfig at build time.
…IBLE `IsFormatSupported` and `Initialize` were handed a pointer derived from the 18-byte `WAVEFORMATEX` prefix, but with `cbSize` set to 22 the driver reads all 40 bytes of the `WAVEFORMATEXTENSIBLE`. The read stays in bounds of the actual allocation, so it works in practice, but the pointer's provenance only covered the prefix, which is out-of-bounds under Stacked/Tree Borrows. The helper is also renamed to `as_waveformatex_ptr` so the `waveformatex_ptr` parameters of the format-reading functions stay nameable.
The `DeviceTrait` docs now summarize the contract in two sentences; each host module documents what its `timeout` bounds, which backends ignore it, and the WASAPI activation path is annotated at the unbounded `IMMDevice::Activate` call.
`IAudioClient::GetBufferSize()` was trusted verbatim. The capture path multiplies it by the frame size to allocate the i24 scratch buffer, so a wild report produced a multi-gigabyte allocation (and an allocation failure aborts the process rather than returning an error); on 32-bit Windows the same product overflows `usize`. A reported size beyond the request plus a 10 s headroom, or whose byte product exceeds a 1 GiB ceiling, is now rejected at build time.
GetCurrentPadding is documented to never exceed the buffer size, but a driver that violates that made get_available_frames underflow, handing GetBuffer and slice::from_raw_parts_mut a frame count near u32::MAX in release builds.
`process_output` returned early through `?` between `GetBuffer` and `ReleaseBuffer`, leaving the render packet checked out for the rest of the client's life. The render loop treats that error as fatal today, so nothing user-visible follows from it, but the GetBuffer/ReleaseBuffer pairing should not depend on the loop's current error policy. A `RenderPacket` guard now releases on every exit path, panics included; the timestamp-error path releases zero frames so the unwritten buffer is not presented.
AUDCLNT_S_BUFFER_EMPTY is a success HRESULT, so windows-rs reports it through Ok(()) and the error arm matching it could never fire. The documented contract is that ppData is left unwritten in that case, so an empty packet fell through to the data callback carrying whatever pointer the previous iteration had left behind, starting with a null one. Packets marked AUDCLNT_BUFFERFLAGS_SILENT are now filled with silence: the engine is not required to have initialized their contents, and a stale buffer would otherwise reach the callback as captured audio. The scratch those packets are served from is sized in i64 words, the widest sample format, so a buffer handed to the callback through it stays aligned for every format. Its slices are bounded before the raw pointer casts, so a length error panics rather than reading out of bounds.
`IAudioCaptureClient::GetBuffer` opens a packet transaction that must be closed with `ReleaseBuffer`. The timestamp query between the two returns early on failure, leaving the packet held. `run_input` ends the run loop on the first `process_input` error, so nothing issues another `GetBuffer` today and the leak has no observable effect; it is still a broken COM contract.
process_input drained packets until GetNextPacketSize reported zero, while run_input polls the command channel only between wakeups. GetNextPacketSize is implemented by the audio engine and always reports an empty packet once the ring is drained, but a data callback slower than realtime refills the ring while the loop runs, so the drain may never reach zero and stop()/Drop go unserviced. The per-event bound caps the drain so command polling is guaranteed.
`GetBuffer` writes the packet's frame count, so the value is driver controlled and was never rechecked against the buffer size reported at build time. The i24 path slice-indexed a scratch buffer sized to that report, turning an oversized packet into a panic on the worker thread; every other format handed `Data::from_parts` a length that reads past the packet. The `frames * bytes_per_frame` product also wraps a 32-bit `usize`.
The checked_mul overflow guard and the PCM/extensible field layout are only reachable through real misconfigurations; unit-test them directly. Also covers the values the i24 and PCM paths hand to the engine.
The SILENT-packet path serves capture buffers filled by fill_equilibrium; pin the per-format byte patterns (0x80 for U8, zeros for signed/float/i24, 0x69 for DSD). The typed path (U16/U24/U32/U64) is the only one that casts the buffer to a wider pointer, so pin the value it writes and that the byte chunks account for every byte.
|
Pure AI slop. Not gonna waste my time writing an elaborate review |
|
Would it help if I contributed fuzzers? |
|
Depends on what you plan to fuzz exactly, and how you'd go about evaluating the outcome. |
|
My initial thinking is that we need less, not more. I'm gonna go to bed now, but from a cursory look I was also like "when could this happen / which actual problem is this solving?" |
|
OK. Let's try to structure this a little bit! I'm thinking that, since this is a Rust project, some of what I have made could possibly be better addressed by using the type system to enforce correctness by construction. This is clearly the right direction from an engineering perspective, with the caveat that it will break API compatibility. I'll give it a little more thought and probably open issues, assuming this bears fruits, rather than make PRs. Now that this is out of the way, do you agree that input validation should be performed at trust boundaries and in front of unsafe code? If you disagree with this, 90% of this PR is going to look useless to you. When the above two points are handled, what remains is the question of runtime checks in APIs which can't be hardened using the Rust type system and do not sit either on a trust boundary or directly in front of unsafe Rust code. I guess that's about 20-30% of this PR and such checks can be seen as redundant, provided there is a clear API contract. (I wish the Rust compiler checked operations on numerals, when possible. When it comes to numerals, Rust code is vulnerable to the same classes of issues as C code: integer overflows, unsafe casts and the like.) Would the following look like a good plan to you? I can open issues to track places where correctness by construction can be used to harden APIs according to my assessment, I can open issues to track where a clear API contract needs to be defined and then update this PR to keep only checks in front of unsafe code and on the driver-app boundary. |
Or just not at all, since it is comically overkill.
Certainly not to the degree you seem to have in mind.
It literally does.
I'm a bit sceptical, but will give you the benefit of doubt |
3 participants
Summary
The WASAPI backend trusts values that come from outside cpal: the buffer size
IAudioClient::GetBufferSize()reports, the padding countGetCurrentPadding()reports, the frame countIAudioCaptureClient::GetBuffer()writes, thecbSizea driver declares for a format, and the flag bits on a capture packet. It also read aWAVEFORMATEXTENSIBLEthrough a pointer derived from the 18-byteWAVEFORMATEXprefix while handing the drivercbSize = 22, and theGetBuffer/ReleaseBufferpairing on both the render and the capture path could be left open on an error return.A misreporting or misbehaving driver turns them into a panic on the audio worker thread (which cpal cannot report to the user), an out-of-bounds read, a multi-gigabyte allocation, or a capture buffer served with stale contents. Two of them need no misbehaviour at all: a wide interface (44+ channels), or a user-chosen 8-byte sample format at 22 channels, overflows the
WAVEFORMATEXfields derived from the config and the probe path walks sample rates up to 24.576 MHz, so it overflows there first.Every fix is a bound, a rejection, or a pairing fix at the point the untrusted value enters. No public API changes; the user-facing summary is in
CHANGELOG.md.Changes
src/host/wasapi/device.rscbSizechecked before the extension is read.WAVEFORMATEX.WAVEFORMATEXTENSIBLEprovenance.GetBufferSize()bounded.src/host/wasapi/stream.rsCOM pairing
Docs
DeviceTraittimeout contract is now summarized in the trait docs, each host module documents what itstimeoutbounds and which backends ignore it, and the WASAPI activation path is annotated at the unboundedIMMDevice::Activatecall.cfg gate
frames_to_durationcfg gate admitted all of Windows and any build with theaudioworkletfeature; on Windows only the ASIO and JACK backends call it.Tests
src/host/wasapi/device.rs: pins thechecked_muloverflow guard and the PCM/extensible field layout (only reachable through real misconfiguration) plus the values the i24 and PCM paths hand to the engine.src/host/equilibrium.rs: pins the per-format byte patternsfill_equilibriumwrites (0x80forU8, zeros for signed/float/i24,0x69for DSD) and the typed path (U16/U24/U32/U64), the only one that casts the buffer to a wider pointer, including that itschunks_exactaccounts for every byte.Verification
Automated.
cargo fmt --check;cargo clippy --all-targets --target x86_64-pc-windows-msvc -- -D warnings;cargo clippy --all-targets --target i686-pc-windows-msvc -- -D warnings;cargo test --lib(17 passed). Clean on Windows 11, MSVC toolchain linked with LLVM lld 23.1.1. Every commit in the series also builds withcargo check --all-targetsforx86_64-pc-windows-msvc.Hardware, Windows 11 (build 26200), PreSonus AudioBox 22VSL and a VB-Audio Virtual Cable endpoint, 48 kHz,
a 440 Hz tone piped into it. Capture was exercised end to end through the virtual and the native 24-bit
interfaces.
Branches no healthy driver reaches, forced under
lldbon a liverecord_wavcapture stream.AUDCLNT_BUFFERFLAGS_SILENTflag forced before the branch.frames_available = 1057, one past the buffer): the guard returned itsBackendErrorto the user's error callback, and the packet was released.i32values equalengine_i32 >> 8exactly.I used coding agents extensively while working on this. I don't expect regressions on other architectures, but I have not ran tests apart from the ones I ran on Windows 11.
Fixes #1376