fix mock NMR deregister race.

.github/workflows/build.yml

@@ -81,7 +81,7 @@ jobs:
       working-directory: ./${{env.BUILD_PLATFORM}}/${{env.BUILD_CONFIGURATION }}
       shell: cmd
       run: |
-        powershell ..\..\scripts\Test-FaultInjection.ps1 ${{env.DUMP_PATH}} ${{env.TEST_TIMEOUT}} ".\usersim_tests.exe" 4
+        powershell ..\..\scripts\Test-FaultInjection.ps1 ${{env.DUMP_PATH}} ${{env.TEST_TIMEOUT}} ".\usersim_tests.exe" 4 "~[no_fi]"
 
   build-cmake:
     timeout-minutes: 15
@@ -151,4 +151,4 @@ jobs:
       working-directory: ./build/bin/${{env.BUILD_CONFIGURATION}}
       shell: cmd
       run: |
-        powershell ..\..\..\scripts\Test-FaultInjection.ps1 ${{env.DUMP_PATH}} ${{env.TEST_TIMEOUT}} ".\usersim_tests.exe" 4
+        powershell ..\..\..\scripts\Test-FaultInjection.ps1 ${{env.DUMP_PATH}} ${{env.TEST_TIMEOUT}} ".\usersim_tests.exe" 4 "~[no_fi]"

scripts/Test-FaultInjection.ps1

@@ -12,7 +12,7 @@
 param ($OutputFolder, $Timeout, $TestProgram, $StackDepth)
 
 # Gather list of all possible tests
-$tests = & $TestProgram "--list-tests" "--verbosity=quiet"
+$tests = & $TestProgram "--list-tests" "--verbosity=quiet" "~[no_fi]"
 
 $env:CXPLAT_FAULT_INJECTION_SIMULATION = $StackDepth
 

src/nmr_impl.cpp

@@ -189,9 +189,15 @@ nmr_t::bind(_Inout_ client_registration& client, _Inout_ provider_registration&
         if (!NT_SUCCESS(status)) {
             unbind_complete(*binding_ptr);
         } else {
+            bool should_begin_unbind = false;
             std::unique_lock l(lock);
             binding_ptr->client_binding_status = binding_status::Ready;
             binding_ptr->provider_binding_status = binding_status::Ready;
+            should_begin_unbind = binding_ptr->client.deregistering || binding_ptr->provider.deregistering;
+            l.unlock();
+            if (should_begin_unbind) {
+                (void)begin_unbind(*binding_ptr);
+            }
         }
     }};
 }
@@ -221,12 +227,13 @@ nmr_t::unbind_complete(_Inout_ binding& binding)
     bindings_changed.notify_all();
 }
 
-bool // true if pending, false if complete.
+bool
 nmr_t::begin_unbind(_Inout_ binding& binding)
 {
     std::unique_lock l(lock);
     if (binding.client_binding_status != Ready || binding.provider_binding_status != Ready) {
-        // Unbind already started.
+        // A Start binding is already published and contributes to binding_count, so deregistration
+        // must keep waiting even though detach cannot begin until attach finishes and reaches Ready.
         return true;
     }
     binding.client_binding_status = BeginUnbind;
@@ -356,7 +363,7 @@ nmr_t::perform_bind(
 }
 
 template <typename initiator_collection_t>
-bool // true if pending, false if complete
+bool
 nmr_t::perform_unbind(
     _Inout_ initiator_collection_t& initiator_collection,
     _In_ initiator_collection_t::value_type::first_type initiator_handle)

src/nmr_impl.h

@@ -148,7 +148,6 @@ typedef class nmr_t
         UnbindComplete ///< Client or provider detach returned STATUS_SUCCESS or called NmrBindingDetachClientComplete
                        ///< or NmrBindingDetachProviderComplete.
     };
-
     struct binding
     {
         provider_registration& provider;
@@ -250,7 +249,7 @@ typedef class nmr_t
      * @brief Start the process of unbinding a client from a provider.
      *
      * @param[in] binding_handle Binding handle to unbind.
-     * @retval true Either the client or provider returned pending.
+     * @retval true Either unbind cannot start yet or it is pending/in progress.
      * @retval false Both the client and provider returned successfully.
      */
     bool

tests/nmr_test.cpp

@@ -8,6 +8,9 @@
 #endif
 #include "../src/framework.h"
 #include <../km/netioddk.h>
+#include <atomic>
+#include <iostream>
+#include <thread>
 
 NPIID test_npiid = {0};
 
@@ -154,6 +157,98 @@ NPI_PROVIDER_CHARACTERISTICS _test_provider_characteristics = {
 
 #pragma endregion test_nmr_provider
 
+#pragma region smoke_nmr_client_provider
+
+// Use a distinct NPI ID so smoke registrations never match the test_npiid registrations above,
+// preventing cross-contamination when test_npiid registrations are left over from failed tests.
+NPIID smoke_npiid = {1};
+
+NPI_REGISTRATION_INSTANCE _smoke_client_registration_instance = {
+    .Size = sizeof(NPI_REGISTRATION_INSTANCE), .NpiId = &smoke_npiid};
+NPI_REGISTRATION_INSTANCE _smoke_provider_registration_instance = {
+    .Size = sizeof(NPI_REGISTRATION_INSTANCE), .NpiId = &smoke_npiid};
+
+static NTSTATUS
+_smoke_client_attach_provider(
+    _In_ HANDLE nmr_binding_handle,
+    _In_opt_ void* client_context,
+    _In_ NPI_REGISTRATION_INSTANCE* provider_registration_instance)
+{
+    UNREFERENCED_PARAMETER(client_context);
+    UNREFERENCED_PARAMETER(provider_registration_instance);
+
+    void* provider_binding_context = nullptr;
+    const void* provider_dispatch = nullptr;
+    return NmrClientAttachProvider(
+        nmr_binding_handle,
+        reinterpret_cast<void*>(nmr_binding_handle),
+        TEST_CLIENT_DISPATCH,
+        &provider_binding_context,
+        &provider_dispatch);
+}
+
+static NTSTATUS
+_smoke_client_detach_provider(_In_ void* client_binding_context)
+{
+    UNREFERENCED_PARAMETER(client_binding_context);
+    return STATUS_SUCCESS;
+}
+
+static void
+_smoke_client_cleanup_binding_context(_In_ void* client_binding_context)
+{
+    UNREFERENCED_PARAMETER(client_binding_context);
+}
+
+NPI_CLIENT_CHARACTERISTICS _smoke_client_characteristics = {
+    .Length = sizeof(NPI_CLIENT_CHARACTERISTICS),
+    .ClientAttachProvider = (PNPI_CLIENT_ATTACH_PROVIDER_FN)_smoke_client_attach_provider,
+    .ClientDetachProvider = _smoke_client_detach_provider,
+    .ClientCleanupBindingContext = _smoke_client_cleanup_binding_context,
+    .ClientRegistrationInstance = _smoke_client_registration_instance};
+
+static NTSTATUS
+_smoke_provider_attach_client(
+    _In_ HANDLE nmr_binding_handle,
+    _In_opt_ void* provider_context,
+    _In_ NPI_REGISTRATION_INSTANCE* client_registration_instance,
+    _In_ void* client_binding_context,
+    _In_ const void* client_dispatch,
+    _Outptr_ void** provider_binding_context,
+    _Outptr_ const void** provider_dispatch)
+{
+    UNREFERENCED_PARAMETER(nmr_binding_handle);
+    UNREFERENCED_PARAMETER(provider_context);
+    UNREFERENCED_PARAMETER(client_registration_instance);
+    UNREFERENCED_PARAMETER(client_dispatch);
+
+    *provider_binding_context = client_binding_context;
+    *provider_dispatch = TEST_PROVIDER_DISPATCH;
+    return STATUS_SUCCESS;
+}
+
+static NTSTATUS
+_smoke_provider_detach_client(_In_ void* provider_binding_context)
+{
+    UNREFERENCED_PARAMETER(provider_binding_context);
+    return STATUS_SUCCESS;
+}
+
+static void
+_smoke_provider_cleanup_binding_context(_In_ void* provider_binding_context)
+{
+    UNREFERENCED_PARAMETER(provider_binding_context);
+}
+
+NPI_PROVIDER_CHARACTERISTICS _smoke_provider_characteristics = {
+    .Length = sizeof(NPI_PROVIDER_CHARACTERISTICS),
+    .ProviderAttachClient = (PNPI_PROVIDER_ATTACH_CLIENT_FN)_smoke_provider_attach_client,
+    .ProviderDetachClient = _smoke_provider_detach_client,
+    .ProviderCleanupBindingContext = _smoke_provider_cleanup_binding_context,
+    .ProviderRegistrationInstance = _smoke_provider_registration_instance};
+
+#pragma endregion smoke_nmr_client_provider
+
 TEST_CASE("NmrRegisterClient", "[nmr]")
 {
     HANDLE nmr_client_handle;
@@ -322,4 +417,50 @@ TEST_CASE("NmrRegisterProvider with async deregister", "[nmr]")
     REQUIRE(_test_provider_binding_context.nmr_binding_handle == nullptr);
 
     REQUIRE(NmrDeregisterClient(nmr_client_handle) == STATUS_SUCCESS);
+}
+
+TEST_CASE("concurrent register/deregister smoke", "[nmr][no_fi]")
+{
+    constexpr size_t iteration_count = 1000;
+    std::atomic<size_t> provider_iterations{0};
+    std::atomic<size_t> client_iterations{0};
+
+    std::thread provider_thread([iteration_count, &provider_iterations]() {
+        for (size_t i = 0; i < iteration_count; i++) {
+            provider_iterations++;
+            HANDLE nmr_provider_handle = nullptr;
+            NTSTATUS register_status =
+                NmrRegisterProvider(&_smoke_provider_characteristics, nullptr, &nmr_provider_handle);
+            if (!NT_SUCCESS(register_status)) {
+                continue;
+            }
+
+            NTSTATUS deregister_status = NmrDeregisterProvider(nmr_provider_handle);
+            if (deregister_status == STATUS_PENDING) {
+                (void)NmrWaitForProviderDeregisterComplete(nmr_provider_handle);
+            }
+        }
+    });
+
+    std::thread client_thread([iteration_count, &client_iterations]() {
+        for (size_t i = 0; i < iteration_count; i++) {
+            client_iterations++;
+            HANDLE nmr_client_handle = nullptr;
+            NTSTATUS register_status = NmrRegisterClient(&_smoke_client_characteristics, nullptr, &nmr_client_handle);
+            if (!NT_SUCCESS(register_status)) {
+                continue;
+            }
+
+            NTSTATUS deregister_status = NmrDeregisterClient(nmr_client_handle);
+            if (deregister_status == STATUS_PENDING) {
+                (void)NmrWaitForClientDeregisterComplete(nmr_client_handle);
+            }
+        }
+    });
+
+    provider_thread.join();
+    client_thread.join();
+
+    std::cout << "provider_iterations=" << provider_iterations.load() << ", client_iterations="
+              << client_iterations.load() << std::endl;
 }