Fix double IRP completion, filter factory error handling, and other bugs from patch review

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);
     }

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();
     }
 

SarAsio/tinyasio.cpp

@@ -30,8 +30,9 @@ std::vector<AsioDriver> 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;

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;
             }
 

SarConfigure/tinyasio.cpp

@@ -53,8 +53,9 @@ std::vector<AsioDriver> 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;

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);

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);

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,

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);
     }

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<SarEndpoint>

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);

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;