Skip to content

Fix: Prevent BSOD from RtPacketsCount race in RtPacketObject - #33

Open
kosmos.ebi (buchizo) wants to merge 3 commits into
microsoft:mainfrom
buchizo:fix-rtpacket-race-divide-by-zero
Open

kosmos.ebi (buchizo) wants to merge 3 commits into
microsoft:mainfrom
buchizo:fix-rtpacket-race-divide-by-zero

Conversation

@buchizo

Copy link
Copy Markdown

Summary

Fixes a 0x1000007E (SYSTEM_THREAD_EXCEPTION_NOT_HANDLED) BSOD caused by an integer divide-by-zero (STATUS_INTEGER_DIVIDE_BY_ZERO, 0xC0000094) inside the mixing thread. A kernel minidump (attached, 081826-11625-01.dmp) captured while a USB Audio 2.0 render stream was active on USBAudio2-ACX.sys was analyzed with WinDbg to identify the exact faulting instruction and root cause.

Crash analysis (from the attached dump)

!analyze -v against the dump (with local build symbols) resolved the fault precisely:

BUGCHECK_CODE:  7e
BUGCHECK_P1:    ffffffffc0000094   (STATUS_INTEGER_DIVIDE_BY_ZERO)
BUGCHECK_P2:    fffff8043692a020

EXCEPTION_RECORD: ...
ExceptionAddress: fffff8043692a020 (USBAudio2_ACX!RtPacketObject::CopyFromRtPacketToOutputData+0xcb0)
ExceptionCode: c0000094 (Integer divide-by-zero)

fffff804`3692a020 43f7743e08      div     eax,dword ptr [r14+r15+8]

FAULTING_SOURCE_LINE:        RtPacketObject.cpp
FAULTING_SOURCE_LINE_NUMBER: 556

STACK_TEXT:
USBAudio2_ACX!RtPacketObject::CopyFromRtPacketToOutputData+0xcb0
USBAudio2_ACX!StreamObject::MixingEngineThreadMain+0x1448
USBAudio2_ACX!StreamObject::MixingEngineThreadFunction+0xa0
USBAudio2_ACX!MixingEngineThread::ThreadMain+0x1f3
USBAudio2_ACX!WorkerThread::ThreadRoutine+0x11
nt!PspSystemThreadStartup+0x5a

The divisor operand [r14+r15+8] lines up exactly with the RtPacketsCount field offset (8 bytes, right after the leading PVOID* RtPackets) in RT_PACKET_INFO (RtPacketObject.h), confirming the crashing statement is:

rtPacketIndex %= rtPacketInfo->RtPacketsCount;   // RtPacketObject.cpp:556

with RtPacketsCount == 0 at the moment of the modulo.

Why

RtPacketObject::UnsetRtPackets() resets RtPackets / RtPacketsCount / RtPacketSize to zero when a stream's RtPacket buffers are freed (EvtStreamFreeRtPackets → CStreamEngine::FreeRtPackets → USBAudioAcxDriverStreamUnsetRtPackets). This path is serialized only by DEVICE_CONTEXT::StreamWaitLock.

RtPacketObject::CopyFromRtPacketToOutputData() / CopyToRtPacketFromInputData() are called from the mixing/capture thread (StreamObject::MixingEngineThreadMain) while holding a different lock, DEVICE_CONTEXT::StreamEngineWaitLock. These functions read RtPacketsCount (and RtPackets) repeatedly across their copy loop and divide/modulo by it.

Because StreamWaitLock and StreamEngineWaitLock do not exclude each other, there is a window where:

  1. The mixing thread checks RenderStreamEngine[deviceIndex]->GetCurrentState() == AcxStreamStateRun and enters CopyFromRtPacketToOutputData.
  2. Concurrently, UnsetRtPackets runs (e.g., stream teardown, or a re-AllocateRtPackets failure path in CStreamEngine::AllocateRtPackets that calls FreeRtPackets on error) and zeroes RtPacketsCount.
  3. The mixing thread's next % RtPacketsCount divides by zero → bugcheck.

The existing RtPacketSize == 0 guard at function entry does not close this window: RtPacketSize and RtPacketsCount are always set/cleared together, but the guard is checked once at entry while the divisor is re-read every loop iteration ? a single point-in-time check cannot protect against a value that changes mid-loop on another thread.

Changes

  • RtPacketObject.h: add a private WDFWAITLOCK m_rtPacketLock, created in the constructor and parented to the device object (same pattern as existing locks in StreamObject).
  • RtPacketObject.cpp:
    • SetRtPackets / UnsetRtPackets: acquire m_rtPacketLock around the read/write of RtPackets / RtPacketsCount / RtPacketSize.
    • CopyFromRtPacketToOutputData / CopyToRtPacketFromInputData: acquire m_rtPacketLock immediately after resolving the RT_PACKET_INFO* for the device, and release it at the single shared _Exit label each function already funnels every early-return path through, so the lock now covers the entire copy loop (all reads of RtPacketsCount / RtPackets).

This makes Set/UnsetRtPackets and the copy paths mutually exclusive, so RtPacketsCount can never be observed as zero mid-copy while a divide/modulo against it is in flight.

Notes

  • m_rtPacketLock is always the innermost lock acquired (never held while acquiring StreamWaitLock or StreamEngineWaitLock), so this does not introduce a new lock-ordering hazard with existing usages of those two locks.
  • Both sides of the race run at PASSIVE_LEVEL (PAGED_CODE_SEG), so a WDFWAITLOCK is appropriate; no DISPATCH_LEVEL callers are involved.
  • This fix addresses the specific divide-by-zero confirmed by this dump (RtPacketObject.cpp:556, dividing by RtPacketsCount). It is unrelated to a separate, not-yet-merged upstream fix (ef7a512d) for a different divide-by-zero in TransferObject::CalculateEstimatedQPCPosition (dividing by m_transferredBytesInThisIrp, tracked as issue [BUG]: Crash #27) ? that one is out of scope for this PR.
  • Kernel dump used for analysis (081826-11625-01.dmp) is attached for reference.

081826-11625-01.dmp

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Lock creation failures are unhandled, and packet state is still read before locking.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Adds synchronization to prevent RT packet teardown from racing with copy operations and causing divide-by-zero crashes.

Changes:

  • Adds a dedicated RT packet wait lock.
  • Protects packet setup, teardown, and copy loops.
File summaries
File Description
RtPacketObject.h Declares the packet-state lock.
RtPacketObject.cpp Creates and applies the lock around packet access.
Review details

Suppressed comments (1)

src/uac2-driver/RtPacketObject.cpp:749

  • Acquire the packet lock before reading packet state. The entry trace and RtPacketSize assertion currently run first, so concurrent UnsetRtPackets can still change these fields during those reads and can trigger the assertion in checked builds. Validate the index first, then resolve rtPacketInfo, acquire the lock, and only then log/assert packet fields.
    WdfWaitLockAcquire(m_rtPacketLock, nullptr);
  • Files reviewed: 2/2 changed files
  • Comments generated: 2
  • Review effort level: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/uac2-driver/RtPacketObject.cpp Outdated
Comment thread src/uac2-driver/RtPacketObject.cpp Outdated

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

Kernel synchronization changes warrant human review and runtime stress validation of concurrent stream teardown.

Review effort: Balanced
Findings: None

Resolved since last review (2)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the changes and verified the fix during testing.
Thanks for the fix. LGTM.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants