Skip to content

Fix Unix named mutex ownership recovery races - #136737

Open
jkoritzinsky wants to merge 1 commit into
mainfrom
dev/jkoritzinsky/redesigned-potato
Open

jkoritzinsky wants to merge 1 commit into
mainfrom
dev/jkoritzinsky/redesigned-potato

Conversation

@jkoritzinsky

Copy link
Copy Markdown
Member

Summary

Native recovery of a Unix named mutex can happen before the runtime cleans up an externally created thread's managed ownership state. Deferred cleanup can then overwrite another process's ownership metadata, while stale local recursion state can misclassify a new acquisition or leak the dead owner's ownership reference.

  • Use local thread identity to retire ownership bookkeeping. Deferred cleanup of pthread-backed mutexes no longer changes shared state or attempts to unlock a mutex that another process may own; owner-thread cleanup and the non-pthread backend retain their existing behavior.
  • Require current-thread ownership before treating an acquisition as recursive, and retire the previous dead owner's chain entry and reference before recording replacement ownership.
  • Add seven regression cases covering recovery before/after deferred cleanup, recovery before ownership publication, recursion, cross-process exclusion, and final reference retirement. Tests use shared interop declarations and direct [UnmanagedCallersOnly] callbacks. The finalizer-dependent scenarios require the new PlatformDetection.ManagedThreadCleanupOnFinalizerThread capability.

Validation

Linux x64, using the existing checked baseline:

  • ./build.sh clr.corelib+clr.nativecorelib+libs.pretest -rc checked -arch x64 - passed.
  • .dotnet/dotnet build src/libraries/System.Threading/tests/System.Threading.Tests.csproj /t:Test /p:Outerloop=true /p:RuntimeConfiguration=Checked - passed: 775 passed, 17 existing platform skips, zero failures. All seven new cases ran without skips.
  • The same seven regression cases fail against the preserved unfixed Release baseline: seven failed, zero skipped.
  • NativeAOT Release compilation passed; the two finalizer-dependent theories are intentionally skipped. Mono was not run.

Performance

Ad hoc BenchmarkDotNet measurements on WSL Ubuntu x64 with Release runtime/libraries, three launches, six warmups, and 15-30 measurement iterations per launch:

Named mutex acquire/release Baseline Updated
Uncontended 141.5 ns 135.6 ns
Recursive 101.4 ns 104.5 ns

The recursive benchmark holds an initial acquisition outside the measured operation. Neither case allocates managed memory. The configured Mann-Whitney(3%) comparison reported Same for both, but distributions were noisy and multimodal; these observations are not evidence of a speedup or a precise bound on small overhead.

Note

This pull request description was generated with GitHub Copilot.

Keep deferred pthread cleanup local and retire stale ownership before recording a new acquisition. Add native-thread regression coverage using shared interop declarations and runtime cleanup capability checks.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 3 pipeline(s).
12 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @JulieLeeMSFT, @VSadov
See info in area-owners.md if you want to be subscribed.

@jkoritzinsky

Copy link
Copy Markdown
Member Author

I think this will fix spurious "cannot release lock not owned by this thread" cases that I've seen locally with .NET 11 RC1 and .NET 11 RC2 sdks locally. Haven't been able to get a dump from those cases so it's difficult to figure out the exact race case that got hit. Copilot found these ones from source-inspection and they seem like reasonable candidates.

@agocke

agocke commented Nov 3, 2026

Copy link
Copy Markdown
Member

honestly, I can't tell if this code is right. It's not obviously wrong, but it's not obviously right, either.

@@ -176,6 +176,9 @@ private static bool GetIsMultithreadingSupported()
public static bool IsWasmThreadingSupported => IsWasm && IsMultithreadingSupported;
public static bool IsNotWasmThreadingSupported => !IsWasmThreadingSupported;

// Whether managed cleanup of externally created threads is deferred to the finalizer thread.
public static bool ManagedThreadCleanupOnFinalizerThread => IsCoreCLR || IsMonoRuntime;

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 is a very corner-case condition. It may be better if it lives with the one test that uses it.

private static bool IsRemoteExecutorAndPThreadNamedMutexSupported =>
IsRemoteExecutorAndCrossProcessNamedMutexSupported &&
PlatformDetection.ManagedThreadCleanupOnFinalizerThread &&
RuntimeInformation.ProcessArchitecture is not (Architecture.Arm or Architecture.Arm64);

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.

I am wondering whether it would be worth it to run the test everywhere and just suppress the validations that depend on implementation details as appropriate.

This branch has not been deployed

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

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

3 participants