[Core] Fix unaligned SIMD load crash in ArrayMath and race conditions during shutdown#515
[Core] Fix unaligned SIMD load crash in ArrayMath and race conditions during shutdown#515Aper-mesa wants to merge 1 commit intoValveSoftware:masterfrom
Conversation
… shutdown 1. src/core/array_math.cpp Fix: In ArrayMath::multiplyAccumulate (complex version), specifically within the else branch handling unaligned memory, changed float4::load to float4::loadu. Reason: The previous code incorrectly used an aligned load instruction (load) inside the fallback branch explicitly meant for unaligned data. This caused crashes (Access Violation) when the accumulation buffer (accum) was not 16-byte aligned. 2. src/core/hrtf_database.cpp Fix: Added validity checks for this pointer and input parameters in HRTFDatabase::ambisonicsHRTF. Reason: During application shutdown (e.g., stopping PIE in Unreal Engine), the main thread may destroy the HRTFDatabase instance while the audio thread is still processing the final frame. This resulted in a "read access violation" where this was nullptr or invalid. The check prevents the crash by returning early. 3. src/core/path_effect.cpp Fix: Added null pointer checks for hrtfForChannel before passing it to ArrayMath::scaleAccumulate inside PathEffect::apply. Reason: As a side effect of the HRTFDatabase fix, if ambisonicsHRTF returns early (due to the shutdown race condition), the hrtfForChannel pointers remain nullptr. Without this check, ArrayMath attempts to read from a null pointer, leading to a secondary crash. Impact Significantly improved stability when using Steam Audio with Wwise/Unreal, particularly when exiting the game or switching levels. Fixed deterministic crashes on hardware/compilers that strictly enforce SIMD alignment requirements.
| const complex_t** hrtf) const | ||
| { | ||
| { | ||
| if (!this) |
There was a problem hiding this comment.
I'm curious as to why the check for !this is needed. What scenario leads to this function being called with this being null? I would say the issue lies somewhere in the caller to this function, which probably needs to check whether the HRTFDatabase instance is valid.
There was a problem hiding this comment.
Not to mention, this being nullptr in C++ invokes undefined behavior. The compiler is free to completely ignore the statement and assume that this is not nullptr anyway.
| const complex_t* in2, | ||
| complex_t* accum) | ||
| { | ||
| if (!in1 || !in2 || !accum || size <= 0 || reinterpret_cast<uintptr_t>(accum) == 0xFFFFFFFFFFFFFFFF) |
There was a problem hiding this comment.
Why the explicit check for accum being 0xFFFFFFFFFFFFFFFF? Curious as to what scenario leads to this specific value. If it's invalid memory you're testing for, it may have some other value too, which this wouldn't catch.
1. src/core/array_math.cpp
Fix: In ArrayMath::multiplyAccumulate (complex version), specifically within the else branch handling unaligned memory, changed float4::load to float4::loadu.
Reason: The previous code incorrectly used an aligned load instruction (load) inside the fallback branch explicitly meant for unaligned data. This caused crashes (Access Violation) when the accumulation buffer (accum) was not 16-byte aligned.
2. src/core/hrtf_database.cpp
Fix: Added validity checks for this pointer and input parameters in HRTFDatabase::ambisonicsHRTF.
Reason: During application shutdown (e.g., stopping PIE in Unreal Engine), the main thread may destroy the HRTFDatabase instance while the audio thread is still processing the final frame. This resulted in a "read access violation" where this was nullptr or invalid. The check prevents the crash by returning early.
3. src/core/path_effect.cpp
Fix: Added null pointer checks for hrtfForChannel before passing it to ArrayMath::scaleAccumulate inside PathEffect::apply.
Reason: As a side effect of the HRTFDatabase fix, if ambisonicsHRTF returns early (due to the shutdown race condition), the hrtfForChannel pointers remain nullptr. Without this check, ArrayMath attempts to read from a null pointer, leading to a secondary crash.
Impact
Significantly improved stability when using Steam Audio with Wwise/Unreal, particularly when exiting the game or switching levels.
Fixed deterministic crashes on hardware/compilers that strictly enforce SIMD alignment requirements.