From 666cc540e3cc8e5b72396410b4919d5f918cdd15 Mon Sep 17 00:00:00 2001 From: Mack Straight Date: Sun, 13 Sep 2026 03:47:02 -0400 Subject: [PATCH] Fix double IRP completion, filter factory error handling, and other bugs Cherry-picked and reworked from a review of fGeorjje's patch (fGeorjje/SynchronousAudioRouter@dcaf4b18a01907737a148ca1fe5179095c264505). Only the changes that fix verified bugs are taken; the rest of that patch is either a no-op or introduces new bugs (see the PR description). Driver: - SarWaitHandleQueue completed the IRP itself when the output buffer was too small, and SarIrpDeviceControl completed it again (bugcheck 0x44). - SarWaitHandleQueue leaked the remaining queue items and their process handles when SarTransferQueuedHandle failed part way through. - SarCreateEndpoint overwrote the status of the first KsCreateFilterFactory call with the second and used both factories without checking, so a failed factory creation (e.g. duplicate endpoint IDs) dereferenced null. - SarDeleteControlContext unmapped the client's section view with ZwCurrentProcess(), but it can run in an arbitrary process when a client releases the last reference to an orphaned context. Move the unmap to SarOrphanControlContext, which runs in the mapping process. - SarKsPinRtGetBufferCore dereferenced endpoint->owner before its own null check, and left buffer cells and a mapped view behind on failure. - getPhysicalConnection in both filter descriptors had a no-op statement where the symbolic link terminator should have been written. - Track the pending-endpoint work item with an explicit flag instead of inferring it from the pending list being empty. - Reject endpoints with a channel count of zero. SarAsio / SarConfigure: - getChannelInfo used strcpy_s into a 32-byte name, which aborts the host process for long endpoint names. - SarClient::stop leaked the notification event handles. - initInnerDriver left a driver whose init() failed in _innerDriver. - InstalledAsioDrivers tested an LSTATUS with SUCCEEDED(), so a failed RegOpenKeyEx was never detected. - Guard the PKEY_Unknown_DevicePath read against an empty PROPVARIANT. - Handle calloc failure when allocating virtual channel buffers. Co-authored-by: Paul Schwandes Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_015Ae1KbSGxTuh8F6i85MGxp --- SarAsio/mmwrapper.cpp | 5 ++- SarAsio/sarclient.cpp | 10 +++++ SarAsio/tinyasio.cpp | 5 ++- SarAsio/wrapper.cpp | 18 ++++++++- SarConfigure/tinyasio.cpp | 5 ++- .../SarTopologyFilterDescriptor.cpp | 2 +- .../SarWaveFilterDescriptor.cpp | 2 +- SynchronousAudioRouter/control.cpp | 37 +++++++++++++++---- SynchronousAudioRouter/entry.cpp | 23 +++++++++++- SynchronousAudioRouter/sar.h | 1 + SynchronousAudioRouter/utility.cpp | 26 +++++++------ SynchronousAudioRouter/wavert.cpp | 37 +++++++++++++++++-- 12 files changed, 139 insertions(+), 32 deletions(-) diff --git a/SarAsio/mmwrapper.cpp b/SarAsio/mmwrapper.cpp index e6bbd0b..a697112 100644 --- a/SarAsio/mmwrapper.cpp +++ b/SarAsio/mmwrapper.cpp @@ -422,7 +422,10 @@ HRESULT STDMETHODCALLTYPE SarActivateAudioInterfaceWorker::Initialize( break; } - defaultDevicePath = pvalue.pwszVal; + if (pvalue.vt == VT_LPWSTR && pvalue.pwszVal) { + defaultDevicePath = pvalue.pwszVal; + } + PropVariantClear(&pvalue); } while(0); } diff --git a/SarAsio/sarclient.cpp b/SarAsio/sarclient.cpp index 6a4414e..421ebcf 100644 --- a/SarAsio/sarclient.cpp +++ b/SarAsio/sarclient.cpp @@ -238,6 +238,16 @@ void SarClient::stop() _registers = nullptr; _sharedBuffer = nullptr; _sharedBufferSize = 0; + + for (auto& notificationHandle : _notificationHandles) { + if (notificationHandle.handle) { + CloseHandle(notificationHandle.handle); + notificationHandle.handle = nullptr; + } + + notificationHandle.generation = 0; + } + _registersLock.unlock(); } diff --git a/SarAsio/tinyasio.cpp b/SarAsio/tinyasio.cpp index 6a524db..fa746d4 100644 --- a/SarAsio/tinyasio.cpp +++ b/SarAsio/tinyasio.cpp @@ -30,8 +30,9 @@ std::vector InstalledAsioDrivers() LOG(INFO) << "Querying installed ASIO drivers."; - if (!SUCCEEDED(err = RegOpenKeyEx( - HKEY_LOCAL_MACHINE, TEXT("SOFTWARE\\ASIO"), 0, KEY_READ, &asio))) { + if ((err = RegOpenKeyEx( + HKEY_LOCAL_MACHINE, TEXT("SOFTWARE\\ASIO"), 0, KEY_READ, &asio)) != + ERROR_SUCCESS) { LOG(INFO) << "Failed to open HKLM\\SOFTWARE\\ASIO: status " << err; return result; diff --git a/SarAsio/wrapper.cpp b/SarAsio/wrapper.cpp index 3a45a46..41c46b2 100644 --- a/SarAsio/wrapper.cpp +++ b/SarAsio/wrapper.cpp @@ -309,7 +309,8 @@ AsioStatus SarAsioWrapper::getChannelInfo(AsioChannelInfo *info) info->group = 0; info->sampleType = (long)_sampleType; info->isActive = AsioBool::False; // TODO: when is this true? - strcpy_s(info->name, channels[index].name.c_str()); + strncpy_s(info->name, sizeof(info->name), + channels[index].name.c_str(), _TRUNCATE); return AsioStatus::OK; } @@ -469,6 +470,17 @@ AsioStatus SarAsioWrapper::createBuffers( infos[i].asioBuffers[0] = calloc(bufferFrameSize, getSampleSize(_sampleType)); channel.asioBuffers[1] = infos[i].asioBuffers[1] = calloc(bufferFrameSize, getSampleSize(_sampleType)); + + if (!channel.asioBuffers[0] || !channel.asioBuffers[1]) { + LOG(ERROR) << "Couldn't allocate virtual channel buffers."; + free(channel.asioBuffers[0]); + free(channel.asioBuffers[1]); + channel.asioBuffers[0] = infos[i].asioBuffers[0] = nullptr; + channel.asioBuffers[1] = infos[i].asioBuffers[1] = nullptr; + disposeBuffers(); + return AsioStatus::NoMemory; + } + _bufferConfig .asioBuffers[0][channel.endpointIndex][channel.channelIndex] = channel.asioBuffers[0]; @@ -574,6 +586,10 @@ bool SarAsioWrapper::initInnerDriver() } if (_innerDriver->init(_hwnd) != AsioBool::True) { + // Drop the driver so the wrapper falls back to running + // without an inner driver instead of calling into one + // that never initialized. + _innerDriver = nullptr; return false; } diff --git a/SarConfigure/tinyasio.cpp b/SarConfigure/tinyasio.cpp index 965a9ab..67462e3 100644 --- a/SarConfigure/tinyasio.cpp +++ b/SarConfigure/tinyasio.cpp @@ -53,8 +53,9 @@ std::vector InstalledAsioDrivers() LOG(INFO) << "Querying installed ASIO drivers."; - if (!SUCCEEDED(err = RegOpenKeyEx( - HKEY_LOCAL_MACHINE, TEXT("SOFTWARE\\ASIO"), 0, KEY_READ, &asio))) { + if ((err = RegOpenKeyEx( + HKEY_LOCAL_MACHINE, TEXT("SOFTWARE\\ASIO"), 0, KEY_READ, &asio)) != + ERROR_SUCCESS) { LOG(INFO) << "Failed to open HKLM\\SOFTWARE\\ASIO: status " << err; return result; diff --git a/SynchronousAudioRouter/SarTopologyFilterDescriptor.cpp b/SynchronousAudioRouter/SarTopologyFilterDescriptor.cpp index f7211ec..5216956 100644 --- a/SynchronousAudioRouter/SarTopologyFilterDescriptor.cpp +++ b/SynchronousAudioRouter/SarTopologyFilterDescriptor.cpp @@ -193,7 +193,7 @@ NTSTATUS SarTopologyFilterDescriptor::getPhysicalConnection(PIRP irp, PKSIDENTIF pinData->Size = symlink->Length + sizeof(KSPIN_PHYSICALCONNECTION); RtlCopyMemory(pinData->SymbolicLinkName, symlink->Buffer, symlink->Length); - pinData->SymbolicLinkName[symlink->Length/2]; + pinData->SymbolicLinkName[symlink->Length / sizeof(WCHAR)] = UNICODE_NULL; pinData->Pin = 1; SarReleaseEndpointAndContext(endpoint); diff --git a/SynchronousAudioRouter/SarWaveFilterDescriptor.cpp b/SynchronousAudioRouter/SarWaveFilterDescriptor.cpp index 732f7ba..08ecf0d 100644 --- a/SynchronousAudioRouter/SarWaveFilterDescriptor.cpp +++ b/SynchronousAudioRouter/SarWaveFilterDescriptor.cpp @@ -337,7 +337,7 @@ NTSTATUS SarWaveFilterDescriptor::getPhysicalConnection(PIRP irp, PKSIDENTIFIER pinData->Size = symlink->Length + sizeof(KSPIN_PHYSICALCONNECTION); RtlCopyMemory(pinData->SymbolicLinkName, symlink->Buffer, symlink->Length); - pinData->SymbolicLinkName[symlink->Length / 2]; + pinData->SymbolicLinkName[symlink->Length / sizeof(WCHAR)] = UNICODE_NULL; pinData->Pin = 0; SarReleaseEndpointAndContext(endpoint); diff --git a/SynchronousAudioRouter/control.cpp b/SynchronousAudioRouter/control.cpp index 07b7e13..00695f8 100644 --- a/SynchronousAudioRouter/control.cpp +++ b/SynchronousAudioRouter/control.cpp @@ -391,6 +391,7 @@ VOID SarProcessPendingEndpoints(PDEVICE_OBJECT deviceObject, PVOID context) goto retry; } + controlContext->workItemRunning = FALSE; ExReleaseFastMutex(&controlContext->mutex); SarReleaseControlContext(controlContext); } @@ -413,7 +414,8 @@ NTSTATUS SarCreateEndpoint( } if (request->index >= SAR_MAX_ENDPOINT_COUNT || - request->channelCount > SAR_MAX_CHANNEL_COUNT) { + request->channelCount > SAR_MAX_CHANNEL_COUNT || + request->channelCount == 0) { return STATUS_INVALID_PARAMETER; } @@ -518,13 +520,27 @@ NTSTATUS SarCreateEndpoint( device, &endpoint->filterDescriptor.filterDesc, endpoint->deviceIdMangled.Buffer, nullptr, KSCREATE_ITEM_FREEONSTOP, nullptr, nullptr, &endpoint->filterFactory); - status = KsCreateFilterFactory( - device, &endpoint->topologyDescriptor.filterDesc, endpoint->topologyFilterRefId.Buffer, - nullptr, KSCREATE_ITEM_FREEONSTOP, - nullptr, nullptr, &endpoint->topologyFilterFactory); - KsFilterFactoryUpdateCacheData(endpoint->filterFactory, NULL); - KsFilterFactoryUpdateCacheData(endpoint->topologyFilterFactory, NULL); + if (!NT_SUCCESS(status)) { + SAR_ERROR("Couldn't create wave filter factory: %08X", status); + endpoint->filterFactory = nullptr; + } else { + status = KsCreateFilterFactory( + device, &endpoint->topologyDescriptor.filterDesc, endpoint->topologyFilterRefId.Buffer, + nullptr, KSCREATE_ITEM_FREEONSTOP, + nullptr, nullptr, &endpoint->topologyFilterFactory); + + if (!NT_SUCCESS(status)) { + SAR_ERROR("Couldn't create topology filter factory: %08X", status); + endpoint->topologyFilterFactory = nullptr; + } + } + + if (NT_SUCCESS(status)) { + KsFilterFactoryUpdateCacheData(endpoint->filterFactory, NULL); + KsFilterFactoryUpdateCacheData(endpoint->topologyFilterFactory, NULL); + } + KsReleaseDevice(ksDevice); if (!NT_SUCCESS(status)) { @@ -538,11 +554,16 @@ NTSTATUS SarCreateEndpoint( ExAcquireFastMutex(&controlContext->mutex); - BOOLEAN runWorkItem = IsListEmpty(&controlContext->pendingEndpointList); + // Track the work item explicitly instead of inferring it from the list + // being empty: the work item pops entries one at a time with the mutex + // dropped, so an empty list doesn't mean the work item has finished (or + // even started) running. + BOOLEAN runWorkItem = !controlContext->workItemRunning; InsertTailList(&controlContext->pendingEndpointList, &endpoint->listEntry); if (runWorkItem) { + controlContext->workItemRunning = TRUE; SarRetainControlContext(controlContext); IoQueueWorkItem( controlContext->workItem, diff --git a/SynchronousAudioRouter/entry.cpp b/SynchronousAudioRouter/entry.cpp index 1daba52..91d4049 100644 --- a/SynchronousAudioRouter/entry.cpp +++ b/SynchronousAudioRouter/entry.cpp @@ -107,8 +107,13 @@ VOID SarDeleteControlContext(SarControlContext *controlContext) controlContext->workItem = nullptr; } + // The section view is unmapped in SarOrphanControlContext, which runs in + // the context of the process that mapped it. This function can run in an + // arbitrary process context (e.g. a client releasing the last reference to + // an orphaned context), so it must not try to unmap it here. if (controlContext->sectionViewBaseAddress) { - ZwUnmapViewOfSection(ZwCurrentProcess(), controlContext->sectionViewBaseAddress); + SAR_WARNING("Section view %p still mapped when deleting controlContext %p", + controlContext->sectionViewBaseAddress, controlContext); controlContext->sectionViewBaseAddress = nullptr; } @@ -130,6 +135,7 @@ BOOLEAN SarOrphanControlContext(SarDriverExtension *extension, PIRP irp) PIO_STACK_LOCATION irpStack = IoGetCurrentIrpStackLocation(irp); SarControlContext *controlContext; LIST_ENTRY orphanEndpoints; + PVOID sectionViewBaseAddress = nullptr; ExAcquireFastMutex(&extension->mutex); controlContext = (SarControlContext *)SarGetTableEntry( @@ -152,6 +158,8 @@ BOOLEAN SarOrphanControlContext(SarDriverExtension *extension, PIRP irp) ExAcquireFastMutex(&controlContext->mutex); controlContext->orphan = TRUE; + sectionViewBaseAddress = controlContext->sectionViewBaseAddress; + controlContext->sectionViewBaseAddress = nullptr; InitializeListHead(&orphanEndpoints); if (!IsListEmpty(&controlContext->endpointList)) { @@ -174,6 +182,19 @@ BOOLEAN SarOrphanControlContext(SarDriverExtension *extension, PIRP irp) SarCancelAllHandleQueueIrps(&controlContext->handleQueue); + // IRP_MJ_CLEANUP runs in the context of the process that mapped the view + // in SarSetBufferLayout, so this is the only place it can safely be + // unmapped via ZwCurrentProcess(). + if (sectionViewBaseAddress) { + NTSTATUS status = ZwUnmapViewOfSection( + ZwCurrentProcess(), sectionViewBaseAddress); + + if (!NT_SUCCESS(status)) { + SAR_WARNING("Couldn't unmap section view %p: %08X", + sectionViewBaseAddress, status); + } + } + if (SarReleaseControlContext(controlContext) == FALSE) { SAR_TRACE("controlContext orphaned but not deleted: %p, refs: %d", controlContext, controlContext->refs); } diff --git a/SynchronousAudioRouter/sar.h b/SynchronousAudioRouter/sar.h index 64c2baf..d0733d6 100644 --- a/SynchronousAudioRouter/sar.h +++ b/SynchronousAudioRouter/sar.h @@ -227,6 +227,7 @@ typedef struct SarControlContext FAST_MUTEX mutex; BOOLEAN orphan; + BOOLEAN workItemRunning; PFILE_OBJECT fileObject; PIO_WORKITEM workItem; LIST_ENTRY endpointList; // List diff --git a/SynchronousAudioRouter/utility.cpp b/SynchronousAudioRouter/utility.cpp index 891e4e0..40a95d4 100644 --- a/SynchronousAudioRouter/utility.cpp +++ b/SynchronousAudioRouter/utility.cpp @@ -431,9 +431,9 @@ NTSTATUS SarWaitHandleQueue(SarHandleQueue *queue, PIRP irp) irp->IoStatus.Information = 0; if (maxItems == 0) { + // SarIrpDeviceControl completes the IRP for any non-pending status, + // so it must not be completed here as well. irp->IoStatus.Information = sizeof(SarHandleQueueResponse); - irp->IoStatus.Status = STATUS_BUFFER_TOO_SMALL; - IoCompleteRequest(irp, IO_NO_INCREMENT); return STATUS_BUFFER_TOO_SMALL; } @@ -484,17 +484,21 @@ NTSTATUS SarWaitHandleQueue(SarHandleQueue *queue, PIRP irp) SarHandleQueueItem *queueItem = CONTAINING_RECORD(entry, SarHandleQueueItem, listEntry); - status = SarTransferQueuedHandle( - irp, kernelProcessHandle, nextItem++, - queueItem->kernelProcessHandle, queueItem->userHandle, - queueItem->associatedData); - ZwClose(queueItem->kernelProcessHandle); - ExFreePoolWithTag(queueItem, SAR_TAG); - irp->IoStatus.Information += sizeof(SarHandleQueueResponse); + // Keep draining the list after a failure so the remaining items + // and their process handles aren't leaked. + if (NT_SUCCESS(status)) { + status = SarTransferQueuedHandle( + irp, kernelProcessHandle, nextItem++, + queueItem->kernelProcessHandle, queueItem->userHandle, + queueItem->associatedData); - if (!NT_SUCCESS(status)) { - break; + if (NT_SUCCESS(status)) { + irp->IoStatus.Information += sizeof(SarHandleQueueResponse); + } } + + ZwClose(queueItem->kernelProcessHandle); + ExFreePoolWithTag(queueItem, SAR_TAG); } ZwClose(kernelProcessHandle); diff --git a/SynchronousAudioRouter/wavert.cpp b/SynchronousAudioRouter/wavert.cpp index d09e2f3..18fda11 100644 --- a/SynchronousAudioRouter/wavert.cpp +++ b/SynchronousAudioRouter/wavert.cpp @@ -16,12 +16,36 @@ #include "sar.h" +// Undo a failed SarKsPinRtGetBufferCore: release the buffer cells reserved +// for the endpoint and, if the view was already mapped into the calling +// process, unmap it. Without this a retried buffer allocation leaks the +// cells until the control context is destroyed. +static VOID SarKsPinRtGetBufferCleanup( + SarEndpoint *endpoint, SarEndpointProcessContext *processContext, + ULONG cellIndex, ULONG cellCount) +{ + SarControlContext *controlContext = endpoint->owner; + + if (processContext && processContext->bufferUVA) { + ZwUnmapViewOfSection(ZwCurrentProcess(), processContext->bufferUVA); + processContext->bufferUVA = nullptr; + } + + ExAcquireFastMutex(&controlContext->mutex); + RtlClearBits(&controlContext->bufferMap, cellIndex, cellCount); + ExReleaseFastMutex(&controlContext->mutex); + + endpoint->activeCellIndex = 0; + endpoint->activeViewSize = 0; + endpoint->activeBufferSize = 0; +} + NTSTATUS SarKsPinRtGetBufferCore( PIRP irp, PVOID baseAddress, ULONG requestedBufferSize, ULONG notificationCount, PKSRTAUDIO_BUFFER buffer) { SarEndpoint *endpoint = SarGetEndpointFromIrp(irp, TRUE); - SarControlContext *controlContext = endpoint->owner; + SarControlContext *controlContext; SarEndpointProcessContext *processContext; NTSTATUS status; @@ -30,6 +54,8 @@ NTSTATUS SarKsPinRtGetBufferCore( return STATUS_NOT_FOUND; } + controlContext = endpoint->owner; + if (baseAddress != nullptr) { SAR_ERROR("It wants a specific address"); SarReleaseEndpointAndContext(endpoint); @@ -58,6 +84,7 @@ NTSTATUS SarKsPinRtGetBufferCore( endpoint->activeChannelCount), controlContext->sampleSize * endpoint->activeChannelCount); SIZE_T viewSize = ROUND_UP(actualSize, SAR_BUFFER_CELL_SIZE); + ULONG cellCount = (ULONG)(viewSize / SAR_BUFFER_CELL_SIZE); ExAcquireFastMutex(&controlContext->mutex); @@ -69,8 +96,7 @@ NTSTATUS SarKsPinRtGetBufferCore( } ULONG cellIndex = RtlFindClearBitsAndSet( - &controlContext->bufferMap, - (ULONG)(viewSize / SAR_BUFFER_CELL_SIZE), 0); + &controlContext->bufferMap, cellCount, 0); if (cellIndex == 0xFFFFFFFF) { SAR_ERROR("Cell index full 0xFFFFFFFF"); @@ -97,6 +123,7 @@ NTSTATUS SarKsPinRtGetBufferCore( if (!NT_SUCCESS(status)) { SAR_ERROR("Section mapping failed %08X", status); + SarKsPinRtGetBufferCleanup(endpoint, nullptr, cellIndex, cellCount); SarReleaseEndpointAndContext(endpoint); return status; } @@ -108,6 +135,7 @@ NTSTATUS SarKsPinRtGetBufferCore( if (!NT_SUCCESS(status)) { SAR_ERROR("Read endpoint registers failed %08X", status); + SarKsPinRtGetBufferCleanup(endpoint, processContext, cellIndex, cellCount); SarReleaseEndpointAndContext(endpoint); return status; } @@ -120,8 +148,9 @@ NTSTATUS SarKsPinRtGetBufferCore( if (!NT_SUCCESS(status)) { SAR_ERROR("Couldn't write endpoint registers: %08X %p %p", status, processContext->process, PsGetCurrentProcess()); + SarKsPinRtGetBufferCleanup(endpoint, processContext, cellIndex, cellCount); SarReleaseEndpointAndContext(endpoint); - return status; // TODO: goto err_out + return status; } buffer->ActualBufferSize = actualSize;