Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[release/9.0-staging] Fix handling of IDynamicInterfaceCastable wrt CastCache #110007

Open
wants to merge 1 commit into
base: release/9.0-staging
Choose a base branch
from

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Nov 20, 2024

Backport of #108328 to release/9.0-staging

/cc @MichalStrehovsky

Customer Impact

  • Customer reported
  • Found internally

One more IDynamicInterfaceCastable related issue. This one was found as we were unblocking tests as part of fixing a customer reported issue (that was known and had known disabled tests). The tests happened to also hit this problem.

The issue is that a cast of a IDynamicInterfaceCastable object to a variant interface would store the wrong value into CastCache - we'd answer the first cast correctly, but a second time the same pair of type is checked for castability, we'd use the wrong cached value and invalid cast exception would happen.

Regression

  • Yes
  • No

Regressed in .NET 8, likely in #90234.

Testing

Added targeted test that is part of this PR. We also subsequently enabled more testing in #108376 but that depends on both PRs so it's in a separate PR.

Risk

This should be low risk, it only affects IDynamicInterfaceCastable and we made sure the caching logic is same as CoreCLR. This has been in main for 2 months.

IMPORTANT: If this backport is for a servicing release, please verify that:

  • The PR target branch is release/X.0-staging, not release/X.0.

  • If the change touches code that ships in a NuGet package, you have added the necessary package authoring and gotten it explicitly reviewed.

Fixes #108229.

The actual fix is the addition of an `if` check where it originally wasn't. I also fixed the other checks for consistency - positive checks are fine to cache, and negative checks against non-interface targets are also fine to cache.
Copy link
Contributor

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

Copy link
Member

@jeffschwMSFT jeffschwMSFT left a comment

Choose a reason for hiding this comment

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

lgtm. we are holding on this one but may take with the other IDynamicInterfaceCastable issues for consideration

@jeffschwMSFT jeffschwMSFT added this to the 9.0.x milestone Nov 21, 2024
@jeffschwMSFT jeffschwMSFT added the Servicing-approved Approved for servicing release label Nov 26, 2024
@jeffschwMSFT jeffschwMSFT modified the milestones: 9.0.x, 9.0.1 Nov 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-NativeAOT-coreclr Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants