fix(coreaudio): fix undefined behavior causing sanitizer crashes #1052
Conversation
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
…tizer crashes In audio_devices(), data_size was declared as immutable but AudioObjectGetPropertyDataSize needs to write the actual property size into it. This caused undefined behavior that sanitizers (ASAN/TSAN) detected as writes to immutable memory. This bug was introduced in commit ed9d643 when migrating from coreaudio-rs to objc2-core-audio. The old raw pointer code hid the UB with an unsafe cast (&data_size as *const _ as *mut _), but the new NonNull API properly enforces mutability contracts. The fix changes: - let data_size = 0u32; → let mut data_size = 0u32; - NonNull::from(&data_size) → NonNull::from(&mut data_size) Fixes crashes during device enumeration under AddressSanitizer and ThreadSanitizer on macOS.
Fixed 7 more instances of the same UB pattern where data_size was declared immutable but passed to CoreAudio APIs that write to it: In set_sample_rate() (lines 64, 81, 128): - AudioObjectGetPropertyData writes the actual data size back - Both calls to get sample rates and sample rate ranges In Device::id() (line 376): - AudioObjectGetPropertyData for kAudioDevicePropertyDeviceUID In Device::supported_configs() (lines 420, 471): - AudioObjectGetPropertyDataSize writes the required buffer size - AudioObjectGetPropertyData confirms the size In Device::default_config() (line 598): - AudioObjectGetPropertyData for stream format All of these were causing sanitizer crashes because the CoreAudio APIs expect mutable references (the size parameter is 'inout').
There was a problem hiding this comment.
Pull Request Overview
This PR fixes critical undefined behavior in the CoreAudio backend introduced during the migration to objc2-core-audio. The bug involved passing immutable references to CoreAudio APIs that expect to write through their parameters, which sanitizers correctly detect as UB.
Key Changes:
- Fixed 10 instances of UB by making
data_sizevariables mutable in CoreAudio API calls - Added comprehensive sanitizer CI workflow to prevent future UB issues
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/host/coreaudio/macos/enumerate.rs | Fixed 1 data_size variable declaration and 2 NonNull::from calls to use mutable references |
| src/host/coreaudio/macos/device.rs | Fixed 7 data_size variable declarations and 14 NonNull::from calls to use mutable references |
| .github/workflows/sanitizers.yml | Added new CI workflow running AddressSanitizer and ThreadSanitizer on macOS and Linux |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
roderickvd
reviewed
Nov 28, 2025
Member
roderickvd
left a comment
roderickvd
left a comment
There was a problem hiding this comment.
Very clear description and nice enhancement to ensure safety. I think the workflow can be cleaned up by using matrix strategy.
Added comprehensive sanitizer testing using matrix strategy: - AddressSanitizer (ASAN) for memory safety bugs - ThreadSanitizer (TSAN) for race conditions and threading issues - Runs on both macOS (CoreAudio) and Linux (ALSA) These sanitizers run the existing test suite with runtime instrumentation to catch undefined behavior that normal builds miss. This would have caught the data_size mutability bugs fixed in the previous commits.
sinkingsugar
force-pushed
the
fix/coreaudio-sanitizer-data-size-mut
branch
from
418dd62 to
4379b3f
Compare
Contributor
Author
Thank you, I pushed a cleanup using matrix strategy. |
Member
|
Looks great, thanks. Merging now. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Summary
Fixes undefined behavior in CoreAudio backend that causes crashes under AddressSanitizer and ThreadSanitizer. This was introduced in #943 when migrating from
coreaudio-rstoobjc2-core-audio.The Bug
In 10 locations across
macos/enumerate.rsandmacos/device.rs,data_sizevariables were declared immutable but passed to CoreAudio APIs that write to them:CoreAudio's size parameter is
inout- the API writes the actual data size back. CreatingNonNullfrom an immutable reference and then writing through it is undefined behavior.Why It Wasn't Caught Earlier
The old raw pointer code hid the UB with unsafe casts:
The new
objc2-core-audioAPI properly enforces mutability contracts viaNonNull, exposing the latent bug. Normal builds work fine because at the machine level the stack memory is writable, but sanitizers enforce Rust's safety contracts and crash.The Fix
Changed all affected variables to
mutand updatedNonNullcalls:Files changed:
src/host/coreaudio/macos/enumerate.rs: 2data_sizevariables, 4 API callssrc/host/coreaudio/macos/device.rs: 7data_sizevariables, 14 API callsReproducing the Bug
The existing tests (
test_play,test_record) trigger device enumeration, which hits the buggy code.Bonus: Sanitizer CI Workflow
This PR includes a new
.github/workflows/sanitizers.ymlthat would have caught this bug before release. It runs ASAN and TSAN on both macOS (CoreAudio) and Linux (ALSA) since UB can be platform-specific.Benefits:
Checklist
cargo check)Related
coreaudio-rs#943 (objc2-core-audio migration)