From 7d6308080bc7aaa4b2f69daaf2ef2d8e56d1cd79 Mon Sep 17 00:00:00 2001 From: "Shankar Seal (from Dev Box)" Date: Mon, 3 Aug 2026 01:51:25 -0700 Subject: [PATCH] fix mock NMR deregister race. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 6e257781-cd5a-4ae3-9943-0c217ccad54e --- .github/workflows/build.yml | 4 +- scripts/Test-FaultInjection.ps1 | 2 +- src/nmr_impl.cpp | 13 ++- src/nmr_impl.h | 3 +- tests/nmr_test.cpp | 141 ++++++++++++++++++++++++++++++++ 5 files changed, 155 insertions(+), 8 deletions(-) diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 6316a59..f6f2298 100644 --- a/.github/workflows/build.yml +++ b/.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]" diff --git a/scripts/Test-FaultInjection.ps1 b/scripts/Test-FaultInjection.ps1 index 402ce3a..15e76d2 100644 --- a/scripts/Test-FaultInjection.ps1 +++ b/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 diff --git a/src/nmr_impl.cpp b/src/nmr_impl.cpp index 2b6feb3..66caa95 100644 --- a/src/nmr_impl.cpp +++ b/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 -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) diff --git a/src/nmr_impl.h b/src/nmr_impl.h index a8b7ea7..c10bffd 100644 --- a/src/nmr_impl.h +++ b/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 diff --git a/tests/nmr_test.cpp b/tests/nmr_test.cpp index 16a8d04..274aba4 100644 --- a/tests/nmr_test.cpp +++ b/tests/nmr_test.cpp @@ -8,6 +8,9 @@ #endif #include "../src/framework.h" #include <../km/netioddk.h> +#include +#include +#include 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(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 provider_iterations{0}; + std::atomic 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; } \ No newline at end of file