Skip to content

[cDAC] Validate contract presence in interface returning functions - #131009

Merged
hoyosjs merged 6 commits into
dotnet:mainfrom
hoyosjs:juhoyosa/cdac-policy
Jul 29, 2026
Merged

[cDAC] Validate contract presence in interface returning functions#131009
hoyosjs merged 6 commits into
dotnet:mainfrom
hoyosjs:juhoyosa/cdac-policy

Conversation

@hoyosjs

@hoyosjs hoyosjs commented Jul 18, 2026

Copy link
Copy Markdown
Member

Before the cDAC returns an  IXCLRDataProcess, ISosDac*, or CorDB object, eagerly validate that the target actually exposes every contract those interfaces depend on, and fail with a specific, classifiable HRESULT when it doesn't.

  • ValidateForDataAccess(Target) checks the full set of contracts the data-access surface consumes (direct and transitive), without instantiating them.
  • Failures carry classifiable codes:
    - CDAC_E_CONTRACT_NOT_ADVERTISED  - target lacks a cDAC required contract.
    • CDAC_E_CONTRACT_UNRECOGNIZED  - advertised version unknown to this cDAC (runtime newer than tool).
    • CDAC_E_CONTRACT_UNSUPPORTED  - version recognized but intentionally unimplemented (deprecated contract version).
    • CDAC_E_DESCRIPTOR_NOT_FOUND  /  CDAC_E_DESCRIPTOR_MALFORMED  for descriptor-level failures.
  • Platform-conditional contracts are gated on the target OS ( IBuiltInCOM ,  IWindowsErrorReporting  are only required on Windows targets), so non-Windows runtimes aren't rejected for contracts they never advertise.
  • Early-attach safe: in-box (main-descriptor) contracts are required unconditionally, but a contract published by a sub-descriptor (e.g. GC) is deferred while its sub-descriptor is still pending. A target attached before subdescriptors are published are not rejected.  Subdescs are enforced once the provider resolves - a missing or unsupported version then fails. A contract that hasn't been published yet is never version-checked and the tools must react to the E_NOTIMPL surfaced through the COM objects.

@azure-pipelines

Copy link
Copy Markdown
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

Copy link
Copy Markdown
Contributor

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

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR tightens cDAC target/interface creation by switching from “try” patterns to exception-based creation with cDAC-specific HRESULTs, and by eagerly validating that all contracts required for SOS/IXCLRDataProcess (and related interfaces) are available before publishing those interfaces.

Changes:

  • Replace ContractDescriptorTarget.TryCreate usage with Create(...) and surface descriptor failures as FormatException with cDAC-specific CdacHResults codes.
  • Add a contract-availability exception hierarchy plus registry support for distinguishing missing vs unrecognized vs intentionally-unsupported contract versions, and introduce TryValidate for no-instantiation validation.
  • Add CoreCLRContracts.ValidateForDataAccess(...) and call it from interface-creation entrypoints; expand unit coverage for malformed descriptors and contract/version validation behavior.

Reviewed changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
src/native/managed/cdac/tests/UnitTests/ContractDescriptor/TargetTests.cs Adds tests for malformed descriptors, contract missing/unrecognized/obsolete behaviors, and eager validation paths.
src/native/managed/cdac/tests/TestInfrastructure/TestPlaceholderTarget.cs Updates test contract registry to return structured exceptions and track unsupported versions.
src/native/managed/cdac/tests/TestInfrastructure/DumpTestBase.cs Switches dump tests to exception-based target creation.
src/native/managed/cdac/tests/TestInfrastructure/ContractDescriptor/ContractDescriptorBuilder.cs Updates descriptor builder to emit contract versions and to create targets via Create(...) with a Try-wrapper.
src/native/managed/cdac/tests/DataGenerator/TestTarget.cs Updates generator test registry to new TryGetContract signature and new abstract members.
src/native/managed/cdac/scripts/DumpHelpers.cs Switches dump-inspection helper to exception-based target creation.
src/native/managed/cdac/mscordaccore_universal/Entrypoints.cs Uses Create(...) and adds eager ValidateForDataAccess gating for interface publication.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader/ContractDescriptorTarget.cs Implements exception-based descriptor read with CdacHResults classification and stricter malformed/not-found distinction.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader/CachingContractRegistry.cs Adds unsupported-version tracking and TryValidate that avoids instantiation/target reads.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/CoreCLRContracts.cs Adds ValidateForDataAccess and maps validation failures to distinct CdacHResults.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/ContractRegistry.cs Changes public contract retrieval signature and adds TryValidate/RegisterUnsupported.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/ContractNotAvailableException.cs Introduces public exceptions describing contract availability and validation failures.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/CdacHResults.cs Introduces cDAC-specific HRESULT constants for descriptor/contract validation failures.

Comment thread src/native/managed/cdac/mscordaccore_universal/Entrypoints.cs Outdated
noahfalk and others added 4 commits July 24, 2026 21:08
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…point)

Intermediate checkpoint of the cDAC 'policy by support' work:
- CdacHResults: CDAC_E_CONTRACT_{NOT_ADVERTISED,UNRECOGNIZED,UNSUPPORTED}
  and CDAC_E_DESCRIPTOR_{NOT_FOUND,MALFORMED} (customer-bit failure codes).
- ContractNotAvailableException hierarchy (Missing/Unrecognized/Obsolete)
  and ContractValidationException carrying the distinct HRESULT.
- ContractRegistry.TryValidate + CachingContractRegistry no-instantiation
  resolution (safe on partial/triage dumps).
- CoreCLRContracts.ValidateForSos eager validation of the SOS surface,
  run at CLRDataCreateInstance only when no legacy DAC is supplied.
- ContractDescriptorTarget descriptor-read failures mapped to
  CDAC_E_DESCRIPTOR_* via FormatException.HResult.
- Design docs: contract-validation.md + consumer guide.
- Tests in TargetTests.cs.
Copilot AI review requested due to automatic review settings July 25, 2026 05:56
@hoyosjs
hoyosjs force-pushed the juhoyosa/cdac-policy branch from 3200f05 to 0d02967 Compare July 25, 2026 05:56

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (2)

src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/ContractRegistry.cs:172

  • This change introduces breaking public API surface in cDAC abstractions (e.g., ContractRegistry.TryGetContract signature change, new TryValidate/RegisterUnsupported APIs) and adds new public types (CdacHResults, ContractNotAvailableException and derived exceptions). These need the normal API review/approval linkage (or the new APIs should be made internal if they aren't intended for external consumption).
    public abstract bool TryGetContract<TContract>([NotNullWhen(true)] out TContract contract, [NotNullWhen(false)] out System.Exception? failureException) where TContract : IContract;

    public TContract GetContract<TContract>() where TContract : IContract
    {
        if (!TryGetContract(out TContract contract, out System.Exception? failureException))
        {
            throw failureException ?? new ContractMissingException(TContract.Name);

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

  • CreateTargetFromCorDebugDataTarget throws on GetContractDescriptor failure, but the thrown InvalidOperationException doesn't set a cDAC-specific HRESULT. Since entrypoints propagate Exception.HResult and CLRDataCreateInstanceCore already maps this case to CDAC_E_DESCRIPTOR_NOT_FOUND, it seems worth doing the same here so callers can reliably distinguish 'descriptor not found' from other failures.
        int hr = contractLocator.GetContractDescriptor(&contractAddress);
        if (hr != 0)
        {
            throw new InvalidOperationException(
                $"{nameof(ICLRContractLocator)} failed to fetch the contract descriptor with HRESULT: 0x{hr:x}.");
        }

@hoyosjs
hoyosjs marked this pull request as ready for review July 27, 2026 20:33
Copilot AI review requested due to automatic review settings July 27, 2026 20:34

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.

Copilot AI review requested due to automatic review settings July 29, 2026 00:05

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (3)

src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Abstractions/ContractRegistry.cs:166

  • This change introduces/changes public API surface in the cDAC abstractions (e.g., TryGetContract now returns an exception, new TryValidate/RegisterUnsupported, new public exception types/HRESULT constants). dotnet/runtime policy requires an approved API proposal/issue (api-approved) for new public APIs and breaking signature changes; please link the approval issue in the PR description or make the new surface internal until API review is completed.
    /// <summary>
    /// Attempts to get an instance of the requested contract for the target.
    /// </summary>
    /// <typeparam name="TContract">The contract type to retrieve.</typeparam>
    /// <param name="contract">
    /// When this method returns <see langword="true"/>, contains the requested contract instance; otherwise, <see langword="null"/>.
    /// </param>
    /// <param name="failureException">
    /// When this method returns <see langword="false"/>, contains the exception that describes why the contract could not be retrieved; otherwise, <see langword="null"/>.
    /// </param>
    /// <returns>
    /// <see langword="true"/> if the requested contract is present and was retrieved successfully; <see langword="false"/> if the contract is not present or registered"/>.
    /// </returns>
    public abstract bool TryGetContract<TContract>([NotNullWhen(true)] out TContract contract, [NotNullWhen(false)] out System.Exception? failureException) where TContract : IContract;

src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.Contracts/CoreCLRContracts.cs:160

  • Comment grammar: "Since cDAC is all-or-nothing for some tools debugger" reads like a missing punctuation/word and is hard to parse. Consider clarifying the sentence so future readers understand why IRuntimeMutableTypeSystem is validated eagerly.
        // DBI-only in-box contract: used by the DacDbi path (Legacy/Dbi/DacDbiImpl.cs).
        // Since cDAC is all-or-nothing for some tools debugger, a target either
        // exposes a fully serviceable set of contracts, or we fall back/fail.
        Validate<IRuntimeMutableTypeSystem>(registry); // DacDbiImpl.cs (edit-and-continue mutable type system)

src/native/managed/cdac/tests/UnitTests/ContractDescriptor/TargetTests.cs:409

  • This test comment says GCInfo's creator "reads RuntimeInfo from target memory", but RuntimeInfo_1 reads from contract-descriptor globals (TryReadGlobalString), not target memory. The test is still valid (creator would chain into missing RuntimeInfo), but the comment is misleading.
        // GCInfo's creator reads RuntimeInfo from target memory. Advertise GCInfo but omit RuntimeInfo:
        // TryValidate must succeed (a creator is registered) without invoking it, whereas a real
        // GetContract would chain into RuntimeInfo and fail.
        TargetTestHelpers targetTestHelpers = new(arch);

@hoyosjs
hoyosjs merged commit a3ee88f into dotnet:main Jul 29, 2026
70 of 72 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants