Add locking for cDAC APIs by rcj1 · Pull Request #132302 · dotnet/runtime

rcj1 · 2026-08-14T05:02:13Z

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

Co-authored-by: rcj1 <77995559+rcj1@users.noreply.github.com>

azure-pipelines · 2026-08-14T05:03:00Z

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 · 2026-08-14T05:03:49Z

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 · 2026-08-14T06:45:44Z

Any reason to use object and not Lock?

rcj1 · 2026-08-14T07:04:25Z

@copilot let’s start a session

Co-authored-by: rcj1 <77995559+rcj1@users.noreply.github.com>

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

Add shared locking for cDAC APIs

096cc40

Co-authored-by: rcj1 <77995559+rcj1@users.noreply.github.com>

Copilot AI lite review requested due to automatic review settings August 14, 2026 05:02

github-actions Bot added the area-Diagnostics-cdac label Aug 14, 2026

Copilot started reviewing on behalf of rcj1 August 14, 2026 05:02 View session

dotnet-policy-service Bot assigned rcj1 Aug 14, 2026

Copilot AI reviewed Aug 14, 2026

View reviewed changes

Copilot started work on behalf of rcj1 August 14, 2026 07:04 View session

Use Lock for cDAC API serialization

1945b67

Co-authored-by: rcj1 <77995559+rcj1@users.noreply.github.com>

Copilot AI review requested due to automatic review settings August 14, 2026 07:30

Copilot started reviewing on behalf of rcj1 August 14, 2026 07:30 View session

Copilot finished work on behalf of rcj1 August 14, 2026 07:31

Copilot AI reviewed Aug 14, 2026

View reviewed changes

rcj1 requested review from hoyosjs and noahfalk August 14, 2026 14:03

Navigation Menu

Uh oh!

Add locking for cDAC APIs - #132302

Add locking for cDAC APIs#132302
rcj1 wants to merge 2 commits into
mainfrom
copilot/lock-cdac-dacdbi-api

rcj1 commented Aug 14, 2026

Uh oh!

azure-pipelines Bot commented Aug 14, 2026

Uh oh!

dotnet-policy-service Bot commented Aug 14, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

hoyosjs commented Aug 14, 2026

Uh oh!

rcj1 commented Aug 14, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

rcj1 commented Aug 14, 2026

Uh oh!

azure-pipelines Bot commented Aug 14, 2026

Uh oh!

dotnet-policy-service Bot commented Aug 14, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Review details

Uh oh!

hoyosjs commented Aug 14, 2026

Uh oh!

rcj1 commented Aug 14, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Review details

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants