Add locking for cDAC APIs

[rcj1]

Ensure that cDAC APIs which share the same Target do not enter concurrently.

[azure-pipelines]

Azure Pipelines:
Successfully started running 4 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]

Tagging subscribers to this area: @steveisok, @tommcdon, @dotnet/dotnet-diag
See info in area-owners.md if you want to be subscribed.

[Copilot]

Pull request overview

This PR introduces a shared per-Target lock and threads it through the managed cDAC COM surface so that COM entrypoints/wrappers created for the same underlying Target serialize access (preventing concurrent re-entrancy across threads).

Changes:

• Add a per-target ApiLock stored alongside Target in the native handle, and pass it to created SOS/DacDbi instances.
• Add a lightweight ComInterfaceLock RAII helper and apply it broadly across cDAC COM method implementations.
• Update unit/dump tests to pass the new apiLock constructor parameter.

Show a summary per file

File	Description
src/native/managed/cdac/mscordaccore_universal/Entrypoints.cs	Stores {Target, ApiLock} together and passes shared lock into created COM implementations.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/ComInterfaceLock.cs	Adds a small IDisposable helper to scope Monitor locking.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/SOSDacImpl.cs	Adds _apiLock storage and locks around SOS DAC COM entrypoints.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/SOSDacImpl.IXCLRDataProcess.cs	Locks IXCLRDataProcess methods and flows _apiLock into returned wrapper objects; avoids holding the lock while invoking external notification callbacks.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/SOSDacImpl.ICLRDataEnumMemoryRegions.cs	Locks EnumMemoryRegions before delegating to legacy fallback.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/Dbi/DacDbiImpl.cs	Adds _apiLock storage and locks around DacDbi COM entrypoints.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/MetaDataImportImpl.cs	Threads _apiLock into the metadata import wrapper and locks COM methods.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/ClrDataValue.cs	Adds _apiLock field/ctor arg and locks IXCLRDataValue methods; passes lock to nested wrappers it creates.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/ClrDataTypeInstance.cs	Adds _apiLock field/ctor arg and locks IXCLRDataTypeInstance methods.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/ClrDataTypeDefinition.cs	Adds _apiLock field/ctor arg and locks IXCLRDataTypeDefinition methods.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/ClrDataTask.cs	Adds _apiLock field/ctor arg and locks IXCLRDataTask methods; passes lock to created wrappers.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/ClrDataStackWalk.cs	Adds _apiLock field/ctor arg and locks IXCLRDataStackWalk methods; passes lock to created frames.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/ClrDataModule.cs	Adds _apiLock field/ctor arg and locks IXCLRDataModule methods; uses lock for IMetaDataImport wrapper creation.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/ClrDataMethodInstance.cs	Adds _apiLock field/ctor arg and locks IXCLRDataMethodInstance methods; passes lock to created wrappers.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/ClrDataMethodDefinition.cs	Adds _apiLock field/ctor arg and locks IXCLRDataMethodDefinition methods; passes lock to created wrappers.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/ClrDataFrame.cs	Adds _apiLock field/ctor arg and locks IXCLRDataFrame methods; passes lock to created wrappers.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/ClrDataExceptionState.cs	Adds _apiLock field/ctor arg and locks IXCLRDataExceptionState methods; passes lock to created wrappers.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/ClrDataAppDomain.cs	Adds _apiLock field/ctor arg and locks IXCLRDataAppDomain methods.
src/native/managed/cdac/tests/UnitTests/TypeHandleTests.cs	Updates wrapper construction to supply an apiLock.
src/native/managed/cdac/tests/UnitTests/StackWalkTests.cs	Updates stack-walk construction to supply an apiLock.
src/native/managed/cdac/tests/UnitTests/SOSDacInterface8Tests.cs	Updates SOSDacImpl construction to supply an apiLock.
src/native/managed/cdac/tests/UnitTests/SOSDacInterface5Tests.cs	Updates SOSDacImpl construction to supply an apiLock.
src/native/managed/cdac/tests/UnitTests/ObjectTests.cs	Updates SOSDacImpl construction to supply an apiLock.
src/native/managed/cdac/tests/UnitTests/MethodTableTests.cs	Updates SOSDacImpl construction to supply an apiLock.
src/native/managed/cdac/tests/UnitTests/MetaDataImportImplTests.cs	Updates MetaDataImportImpl construction to supply an apiLock.
src/native/managed/cdac/tests/UnitTests/LoaderTests.cs	Updates SOSDacImpl/DacDbiImpl construction to supply an apiLock.
src/native/managed/cdac/tests/UnitTests/IXCLRDataProcessTests.cs	Updates method-definition/SOSDacImpl construction to supply an apiLock.
src/native/managed/cdac/tests/UnitTests/GetRegisterNameTests.cs	Updates SOSDacImpl construction to supply an apiLock.
src/native/managed/cdac/tests/UnitTests/FunctionTableAccessTests.cs	Updates SOSDacImpl/IXCLRDataProcess3 construction to supply an apiLock.
src/native/managed/cdac/tests/UnitTests/DacDbiImplTests.cs	Updates DacDbiImpl construction to supply an apiLock.
src/native/managed/cdac/tests/UnitTests/ClrDataTaskTests.cs	Updates ClrDataTask construction to supply an apiLock.
src/native/managed/cdac/tests/UnitTests/ClrDataRequestTests.cs	Updates request-type wrapper construction to supply an apiLock.
src/native/managed/cdac/tests/UnitTests/ClrDataExceptionStateTests.cs	Updates ClrDataTask/ClrDataExceptionState construction to supply an apiLock.
src/native/managed/cdac/tests/DumpTests/StressLogDumpTests.cs	Updates SOSDacImpl construction to supply an apiLock.
src/native/managed/cdac/tests/DumpTests/StackWalkDumpTests.cs	Updates SOSDacImpl/ClrDataMethodInstance construction to supply an apiLock.
src/native/managed/cdac/tests/DumpTests/MetaDataImportDumpTests.cs	Updates MetaDataImportImpl construction to supply an apiLock.
src/native/managed/cdac/tests/DumpTests/IXCLRDataValueDumpTests.cs	Updates ClrDataFrame construction to supply an apiLock.
src/native/managed/cdac/tests/DumpTests/IXCLRDataMethodDefinitionDumpTests.cs	Updates ClrDataMethodDefinition construction to supply an apiLock.
src/native/managed/cdac/tests/DumpTests/IXCLRDataFrameDumpTests.cs	Updates ClrDataFrame construction to supply an apiLock.
src/native/managed/cdac/tests/DumpTests/IXCLRDataAppDomainDumpTests.cs	Updates ClrDataFrame construction to supply an apiLock.
src/native/managed/cdac/tests/DumpTests/ISOSDacInterfaceTests.cs	Updates SOSDacImpl construction to supply an apiLock.
src/native/managed/cdac/tests/DumpTests/ISOSDacInterface13Tests.cs	Updates SOSDacImpl construction to supply an apiLock.
src/native/managed/cdac/tests/DumpTests/DacDbi/DacDbiThreadDumpTests.cs	Updates DacDbiImpl construction to supply an apiLock.
src/native/managed/cdac/tests/DumpTests/DacDbi/DacDbiStackWalkDumpTests.cs	Updates DacDbiImpl construction to supply an apiLock.
src/native/managed/cdac/tests/DumpTests/DacDbi/DacDbiRefWalkDumpTests.cs	Updates DacDbiImpl construction to supply an apiLock.
src/native/managed/cdac/tests/DumpTests/DacDbi/DacDbiRCWDumpTests.cs	Updates DacDbiImpl construction to supply an apiLock.
src/native/managed/cdac/tests/DumpTests/DacDbi/DacDbiObjectDumpTests.cs	Updates DacDbiImpl construction to supply an apiLock.
src/native/managed/cdac/tests/DumpTests/DacDbi/DacDbiMultiModuleDumpTests.cs	Updates DacDbiImpl construction to supply an apiLock.
src/native/managed/cdac/tests/DumpTests/DacDbi/DacDbiLoaderDumpTests.cs	Updates DacDbiImpl construction to supply an apiLock.
src/native/managed/cdac/tests/DumpTests/DacDbi/DacDbiHeapWalkDumpTests.cs	Updates DacDbiImpl construction to supply an apiLock.
src/native/managed/cdac/tests/DumpTests/DacDbi/DacDbiGCDumpTests.cs	Updates DacDbiImpl construction to supply an apiLock.
src/native/managed/cdac/tests/DumpTests/DacDbi/DacDbiExactTypeHandleDumpTests.cs	Updates DacDbiImpl construction to supply an apiLock.
src/native/managed/cdac/tests/DumpTests/DacDbi/DacDbiDebuggerDumpTests.cs	Updates DacDbiImpl construction to supply an apiLock.
src/native/managed/cdac/tests/DumpTests/DacDbi/DacDbiComWrappersDumpTests.cs	Updates DacDbiImpl construction to supply an apiLock.
src/native/managed/cdac/tests/DumpTests/DacDbi/DacDbiCCWDumpTests.cs	Updates DacDbiImpl construction to supply an apiLock.
src/native/managed/cdac/tests/DumpTests/DacDbi/DacDbiApproxTypeHandleDumpTests.cs	Updates DacDbiImpl construction to supply an apiLock.
src/native/managed/cdac/tests/DumpTests/DacDbi/DacDbiAppDomainDumpTests.cs	Updates DacDbiImpl construction to supply an apiLock.

Review details

• Files reviewed: 57/57 changed files
• Comments generated: 0
• Review effort level: Lite

[hoyosjs]

Any reason to use object and not Lock?

[rcj1]

@copilot let’s start a session

[Copilot]

Review details

Suppressed comments (3)

src/native/managed/cdac/mscordaccore_universal/Entrypoints.cs:203

• The per-target API lock isn’t held while validating the contracts and creating the SOS DAC interface for an existing handle. If this entrypoint is called concurrently with other cDAC APIs sharing the same Target, it can still race against those operations, undermining the PR’s stated goal of serializing access per Target.

            // the complete data-access contract set before publishing the interface.
            if (legacyImpl is null)
                Contracts.CoreCLRContracts.ValidateForDataAccess(cdacHandle.Target);

            Legacy.SOSDacImpl impl = new(cdacHandle.Target, legacyImpl, cdacHandle.ApiLock);

src/native/managed/cdac/mscordaccore_universal/Entrypoints.cs:250

• The per-target API lock isn’t held while creating the DacDbi interface for an existing handle. If this entrypoint is invoked concurrently with other cDAC APIs sharing the same Target, construction and any Target-touching initialization can race with in-flight calls, which conflicts with the PR’s goal of preventing concurrent entry per Target.

            Legacy.DacDbiImpl impl = new(cdacHandle.Target, legacyObj, cdacHandle.ApiLock);
            *obj = (nint)ComInterfaceMarshaller<IDacDbiInterface>.ConvertToUnmanaged(impl);

src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/SOSDacImpl.IXCLRDataProcess.cs:1171

• This change introduces new reentrancy/locking behavior (building a callback under the lock and invoking it after releasing the lock) to avoid deadlocks when notification code calls back into the DAC. There isn’t a unit test covering this behavior (e.g., a notification implementation that re-enters the DAC on another thread and would deadlock if the lock were held during the callback). Adding a regression test would help prevent future refactors from reintroducing the deadlock.

            Action? callback = null;
            {
                using Lock.Scope scope = _apiLock.EnterScope();

                // External notification code can call back into the DAC, so prepare everything

• Files reviewed: 56/56 changed files
• Comments generated: 0 new
• Review effort level: Lite

[hoyosjs]

• ValidateForDataAccess also needs cdacHandle.ApiLock.EnterScope() since it reads the registry.
• What used to use regmeta is now accessing the target. While that's correct, idk if we'll end up in any deadlock situations (needs testing)
• It might be interesting to port non-reentrancy asserts in a follow-up.

[Copilot]

Review details

Suppressed comments (1)

src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/SOSDacImpl.IXCLRDataProcess.cs:33

• This PR introduces per-Target locking as a behavioral change (APIs now serialize through _apiLock). I couldn't find any unit/dump tests that assert the new contract (e.g., that different COM surfaces created for the same Target/handle share the same lock and cannot run concurrently, and that external callbacks don’t execute while the lock is held). Adding a focused regression test would help prevent accidental gaps (missing EnterScope() in a new API) or future deadlocks/regressions.

        using Lock.Scope scope = _apiLock.EnterScope();
        if (bytesNeeded is null || entries is null)

• Files reviewed: 57/57 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

Review details

Suppressed comments (2)

src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Legacy/MetaDataImportImpl.cs:33

• The comment about needing ConcurrentDictionary due to multi-threaded COM calls is now misleading since all access to enum-handle tracking is serialized by _apiLock. Either update the comment to reflect the locking, or consider switching to a non-concurrent collection if you intend the lock to be the only synchronization.

    // Tracks GCHandle values allocated by AllocEnum so that CountEnum, ResetEnum,
    // and CloseEnum can distinguish cDAC-created enum handles from legacy HENUMInternal*.
    // ConcurrentDictionary is used because COM objects may be called from multiple threads.
    private readonly ConcurrentDictionary<nint, byte> _cdacEnumHandles = new();

src/native/managed/cdac/mscordaccore_universal/Entrypoints.cs:476

• This call to ValidateForDataAccess() is part of the work done while constructing an interface that will use apiLock, but it currently doesn't pass the lock. Passing apiLock keeps validation consistent with the PR's goal of serializing contract reads for a given Target/lock pair and avoids future surprises if validation ever runs concurrently with other API calls.

        // the complete data-access contract set before publishing the interface.
        if (legacyImpl is null)
            Contracts.CoreCLRContracts.ValidateForDataAccess(target);

        Legacy.SOSDacImpl impl = new(target, legacyImpl, apiLock);

• Files reviewed: 57/57 changed files
• Comments generated: 0 new
• Review effort level: Lite