Skip to content

fix mock NMR deregister race. - #319

Open
Shankar Seal (shankarseal) wants to merge 2 commits into
microsoft:mainfrom
shankarseal:shankarseal-fix-nmr-deregister-race
Open

fix mock NMR deregister race.#319
Shankar Seal (shankarseal) wants to merge 2 commits into
microsoft:mainfrom
shankarseal:shankarseal-fix-nmr-deregister-race

Conversation

@shankarseal

Copy link
Copy Markdown
Collaborator

Summary

  • fix the mock NMR deregister race where a Start binding could be treated as already unbinding
  • when a binding transitions to Ready/Ready, immediately start unbind if either side is already deregistering
  • add a concurrent register/deregister smoke test that loops provider and client registration/deregistration and prints executed iteration counts

Testing

  • run usersim_tests with filter: "concurrent register/deregister smoke"

Comment thread tests/nmr_test.cpp Outdated
Shankar Seal (from Dev Box) and others added 2 commits August 5, 2026 16:21
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 6e257781-cd5a-4ae3-9943-0c217ccad54e
@shankarseal
Shankar Seal (shankarseal) force-pushed the shankarseal-fix-nmr-deregister-race branch from 7d63080 to d87efd1 Compare August 5, 2026 23:26
Comment thread src/nmr_impl.cpp
should_begin_unbind = binding_ptr->client.deregistering || binding_ptr->provider.deregistering;
l.unlock();
if (should_begin_unbind) {
(void)begin_unbind(*binding_ptr);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This new call can expose a duplicate-completion bug in begin_unbind(). If both detach callbacks return STATUS_PENDING and their completion routines race before begin_unbind() reacquires the lock, the second completion can call unbind_complete() and erase the binding. This begin_unbind() call can then also observe both sides complete and call unbind_complete() again, causing duplicate cleanup callbacks and double-decrementing binding_count. I checked the lifetime angle: the caller retains a shared_ptr, so this is not a use-after-free, but the duplicate cleanup/reference accounting remains. NMR permits each pending detach to complete independently.

Comment thread tests/nmr_test.cpp

NTSTATUS deregister_status = NmrDeregisterProvider(nmr_provider_handle);
if (deregister_status == STATUS_PENDING) {
(void)NmrWaitForProviderDeregisterComplete(nmr_provider_handle);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This smoke test does not validate the wait result. If NmrWaitForProviderDeregisterComplete() fails, the test silently continues and can pass with a live registration or incomplete detach; the client-side wait below has the same issue. Also, the test only relies on probabilistic overlap and does not force deregistration during ClientAttachProvider, so it may miss the race this PR targets.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Todo

Development

Successfully merging this pull request may close these issues.

3 participants