Skip to content

Add AudioSrv/ALPC audio support and fixes - #14

Merged
AdvDebug merged 2 commits into
mainfrom
sound-support-and-fixes
Aug 13, 2026
Merged

Add AudioSrv/ALPC audio support and fixes#14
AdvDebug merged 2 commits into
mainfrom
sound-support-and-fixes

Conversation

@AdvDebug

Copy link
Copy Markdown
Owner

Implement a host-backed audio subsystem and ALPC audio support: add AudioManager, platform sinks (Windows/ALSA/AAudio), AudioSrv and AudioClientRpc handlers, NDR/LRPC helpers, shared-section stream setup and AudioStreamEngine to feed host devices. Add ALPC handle attribute support and PortReply plumbing.

Also: add cursor warp (host/client) integration, scheduler/timing and emulated QPC fixes, various Win/syscall additions and robustness fixes (NtOpenEvent, NtQueryPerformanceCounter, NtMapViewOfSection view refcounting, registry enumeration improvements, GDI table validation, TEB impersonation default, CW_USEDEFAULT handling), and a --install-dxvk CLI option.

In addition to fixing quite a few bugs.

Implement a host-backed audio subsystem and ALPC audio support: add AudioManager, platform sinks (Windows/ALSA/AAudio), AudioSrv and AudioClientRpc handlers, NDR/LRPC helpers, shared-section stream setup and AudioStreamEngine to feed host devices. Add ALPC handle attribute support and PortReply plumbing.

Also: add cursor warp (host/client) integration, scheduler/timing and emulated QPC fixes, various Win/syscall additions and robustness fixes (NtOpenEvent, NtQueryPerformanceCounter, NtMapViewOfSection view refcounting, registry enumeration improvements, GDI table validation, TEB impersonation default, CW_USEDEFAULT handling), and a --install-dxvk CLI option.

In addition to fixing quite a few bugs.
@AdvDebug

Copy link
Copy Markdown
Owner Author

@claude review

@claude

claude Bot commented Aug 13, 2026

Copy link
Copy Markdown

Claude finished @AdvDebug's task in 1m 17s —— View job


Review complete

Todo list:

  • Gather context (diff, repo structure, CLAUDE.md — none found)
  • Review AudioManager / platform sinks (Windows/ALSA/AAudio)
  • Review ALPC/NDR/LRPC and AudioSrvPort/AudioStreamEngine
  • Review syscall & misc fixes (NtOpenEvent, NtQueryPerformanceCounter, NtMapViewOfSection, registry, GDI, cursor warp)
  • Review Program.cs --install-dxvk addition
  • Post final review summary

This is a large, well-structured PR. Most of it looks solid — the NtMapViewOfSection refcounting, the NtQueryValueKey KeyValuePartialInformationAlign64 header-size fix, the registry "ri"-index traversal with sign-bug fix, and the cursor-warp integration (consistently implemented across Linux/Windows/Android) all check out. --install-dxvk has no injection/traversal risk. Below are the issues worth addressing, ordered by severity.

High

Audio stream lifetime / use-after-freeBrovan/Core/Emulation/OS/Windows/RPC/Ports/AudioSrvPort.cs (Streams/NextStreamId, ~line 49-50) and AudioStreamEngine.cs (Run(), ~line 58). AudioSrvPortHandler's stream state is static, shared across all BinaryEmulator instances in the process, and AudioStreamEngine's background thread keeps dereferencing a raw host pointer (GetHostPointer) even after the owning BinaryEmulator.Dispose() runs — nothing disposes the engine/stream when the emulator instance is torn down; only the guest's explicit ReleaseStream RPC does. If a guest process is force-killed or the emulator session ends without a clean audio release, the audio thread keeps reading/writing freed/unmapped memory — an unmanaged access violation that can crash the whole host process. The static state also means a second emulated binary run in the same process inherits stale entries (and their live threads/native device handles) from a previous session that didn't clean up.
Fix this →

Medium

  • Ndr20.cs — unhandled overflow on attacker-controlled length (Ndr20Reader.TryReadConformantWideString, ~line 217): int Bytes = checked((int)ActualCount * 2); operates on ActualCount, which is fully guest-controlled and only checked against an equally guest-controlled MaxCount. Sending ActualCount/MaxCount0x40000000 throws OverflowException instead of failing the parse gracefully, aborting the ALPC call mid-flight and leaving the guest thread with a stale reply.
    Fix this →
  • AudioSrvPort.cs — cross-session stream-handle guessing: Streams/NextStreamId being static (see above) also means stream IDs are sequential and shared process-wide, so a guest in one session can guess a small integer to forge a context-handle cookie and call ReleaseStream/SetupStream/AcknowledgeStreamTransition on another session's stream — cross-session interference/DoS. Fixed by the same per-instance scoping as the High finding above.
  • AudioSrvPort.csWinPort leak in ReleaseStream (~line 310-326): removes the stream and disposes Engine, but never removes the corresponding entry PublishHandlePort (~line 440-452) added to Instance.WinHelper.WinPorts. Repeated setup/release cycles leak one WinPort entry per stream for the session's lifetime.
    Fix this →
  • WinInternalHelper.cs — undocumented handle-masking fallback (TryGetEntry, ~line 173-181): when an exact handle lookup misses, it retries with Handle & ~3UL and treats that as a hit, applying to every handle-table op (GetObjectByHandle, CheckAccess, RemoveHandle, etc.). Handles that differ by 1-3 from a real handle now silently alias to it instead of failing with STATUS_INVALID_HANDLE. No comment explains the rationale — worth documenting why (kernel-handle bit? pseudo-handle?), since it's a broad, silent semantic change.
  • BinaryEmulator.cs — scheduler idle path no longer fast-forwards virtual time (~line 2565-2660): idle waits now really sleep in 5ms slices instead of warping EmulatedTickCount64 forward instantly. This looks intentional (needed for audio timing), but it's a real behavior change — long guest Sleep() calls now burn real wall-clock time and continuously poll instead of one calculated sleep, which will slow down boot/startup and increase idle host CPU usage. Worth confirming this trade-off is intended and, if so, calling it out in the PR description.

Low

  • AudioStreamEngine.cs (~line 83): Take = Math.Min(Available, Chunk.Length) isn't forced to a multiple of BlockAlign; if the client cursor ever advances non-frame-aligned, subsequent audio becomes channel-misaligned (garbled, not a crash).
  • ApiPort.cs HandleGenericRpcPort (~line 206-211): when LrpcPacket.BuildBindAccept returns null for a malformed bind packet, the raw request bytes get echoed back as the "reply" (ReplyBytes = Reply.Data ?? SendBytes) instead of a fault — protocol-confusing but not memory-unsafe.
  • AudioSrvPort.cs FramesForDuration (~line 488-493): unchecked ulong * uint on guest-supplied duration values can silently wrap before being clamped to MaxFrames, giving an unpredictable (if still bounded) ring size.
  • NtCreateEvent.cs (line 36): unreachable return Instance.WinUnimplemented; after a block that always returns — dead code, same pattern already cleaned up in NtOpenKeyEx.cs in this same PR.
  • NtDeleteWnfStateName.cs / NtUnsubscribeWnfStateChange.cs: unconditional STATUS_SUCCESS stubs with no comment noting they're stubs — harmless (no guest buffers touched) but worth a one-line comment.
  • WindowsWinManager.cs WarpCursor (~line 853-863): if ClientToScreen fails (e.g. window destroyed mid-call), the failure is silently swallowed — consistent with existing style in the file, not a regression, but a debug log would help.

The one item I'd consider blocking on is the audio stream lifetime/use-after-free issue — it's a host-process crash vector reachable by an unclean guest teardown, which is a normal occurrence (app crash, force-kill), not an edge case. Everything else is fixable follow-up.

@AdvDebug

Copy link
Copy Markdown
Owner Author

@claude review

@claude

claude Bot commented Aug 13, 2026

Copy link
Copy Markdown

Claude finished @AdvDebug's task in 1m 52s —— View job


Review complete

Todo list:

  • Gather context (diff, repo structure)
  • Check if commit 8d6c277 ("Fix audio stream lifetime and RPC bounds checks") addresses the prior High/Medium findings
  • Review AudioSrvPort.cs / AudioStreamEngine.cs lifetime handling
  • Review Ndr20.cs overflow fix
  • Re-check remaining medium/low items
  • Post final review summary

I re-reviewed the PR, focusing on what commit 8d6c277 ("Fix audio stream lifetime and RPC bounds checks") changed relative to the issues raised in the earlier review pass.

What got fixed

  • Ndr20.cs overflow (TryReadConformantWideString): now bounds-checks ActualCount against (Data.Length - Position) / 2 before computing Bytes, instead of relying on checked to throw. Correct fix.
  • AudioSrvPort.cs WinPort leak: ReleaseStream now calls the new RemoveHandlePort, which removes the matching entry from Instance.WinHelper.WinPorts. Leak is closed.
  • FramesForDuration overflow: now computes MaxDurationHns and early-returns MaxFrames when DurationHns >= MaxDurationHns, avoiding the wraparound path. Correct fix.
  • AudioStreamEngine.Run() frame alignment (Low item): Take is now rounded down to a multiple of BlockAlign, fixing the channel-misalignment issue.
  • NtCreateEvent.cs: dead code after the always-returning block removed.
  • WinInternalHelper.cs handle masking: now has a comment explaining the Handle & ~3UL fallback (OBJ_HANDLE_TAGBITS rationale).
  • Sink teardown (Sink.Dispose()) moved into the Run() loop's finally block, so the device isn't freed while a Sink.Write call is in flight — a good robustness improvement.

Still open: the High-severity lifetime issue

The core problem from the previous pass is not fixed: AudioSrvPortHandler.Streams and NextStreamId (Brovan/Core/Emulation/OS/Windows/RPC/Ports/AudioSrvPort.cs:50-51) remain private static, so stream state is still shared process-wide across every BinaryEmulator instance, and nothing hooks into BinaryEmulator.Dispose() (Brovan/Core/Emulation/BinaryEmulator.cs:3533-3555) to stop/dispose in-flight AudioStreamEngines for that instance. Dispose() still only calls _emulator.StopEmulation() / _emulator.Dispose() / clears _memory, with no reference to AudioSrvPortHandler at all.

The new ServerSectionHandle mitigation (HoldServerSectionReference, ~line 449-459) helps in the clean teardown path — ReleaseStream now only closes the section handle once Stream.Engine.Stop() confirms the worker thread joined (AudioSrvPort.cs:319-322), which avoids freeing the section out from under a still-running engine in that one code path. But this only covers the guest explicitly calling ReleaseStream. It does nothing for the scenario the original finding was about: the guest process is force-killed or the emulator session is torn down (BinaryEmulator.Dispose()) without that RPC ever being sent. In that case:

  • AudioStreamEngine.Run() (AudioStreamEngine.cs:58 onward) keeps running on its background thread, still dereferencing Block — a raw pointer obtained from Instance.GetHostPointer(...), which just forwards to _emulator.GetHostPointer(...) (BinaryEmulator.cs:1414-1417).
  • BinaryEmulator.Dispose() calls _emulator.Dispose() and clears _memory, which is exactly what backs that host pointer — so the audio thread is left dereferencing memory that's being torn down/freed concurrently. This is still a real unmanaged-crash / use-after-free vector for the host process on unclean shutdown.
  • The Streams dictionary being static also still means a new BinaryEmulator instance in the same process inherits/collides with stream IDs and live engines from a previous, undisposed session.

Fix this →

Remaining minor items from the earlier pass (unchanged, still low-severity follow-ups)

  • ApiPort.cs HandleGenericRpcPort (~line 206-211): malformed bind packet still falls back to echoing SendBytes as the reply instead of a fault.
  • NtDeleteWnfStateName.cs / NtUnsubscribeWnfStateChange.cs: unconditional STATUS_SUCCESS stubs, still uncommented — harmless.
  • WindowsWinManager.cs WarpCursor (~line 853-863): ClientToScreen failure still silently swallowed — consistent with existing file style, not a regression.
  • BinaryEmulator.cs idle-wait scheduler change (no longer fast-forwarding EmulatedTickCount64) — worth confirming this real-time-sleep trade-off (needed for audio timing) is intentional, since it increases idle host CPU and slows boot/startup.

Summary

Good, well-scoped fix commit — it correctly closes the Ndr20 overflow, the WinPort leak, the FramesForDuration overflow, and the frame-alignment issue, and it adds a real (if partial) safety improvement for the clean ReleaseStream path. The process-wide static stream state and the missing dispose-time cleanup in BinaryEmulator.Dispose() are the one item I'd still consider blocking: it's a host-process crash vector reachable by ordinary unclean guest teardown (crash/force-kill), not an edge case, and isn't addressed by this commit.

@AdvDebug

AdvDebug commented Aug 13, 2026

Copy link
Copy Markdown
Owner Author

For the high severity issue it shouldn't cause an issue since the emulator instance are static and isn't reusable, but the Dispose issue might be worth fixing later if we ever decided to reuse the instance.

@AdvDebug
AdvDebug merged commit 0494730 into main Aug 13, 2026
2 checks passed
@AdvDebug
AdvDebug deleted the sound-support-and-fixes branch August 13, 2026 23:29
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.

1 participant