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] Fix invalid handle bug happening when TypeBuilder type used in exception catch clause #106704

Merged
merged 3 commits into from
Aug 23, 2024

Conversation

github-actions[bot]
Copy link
Contributor

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

Backport of #106665 to release/9.0

/cc @buyaa-n

Customer Impact

  • Customer reported
  • Found internally

This is found while reviewing PersistedAssemblyBuilder implementation entirely. When newly created TypeBuilder instance is used as exception type, when ILGenerator.BeginCatchBlock(Type? exceptionType) called ILGenerator will throw because the TypeBuilder token is invalid, it works fine when existing exception types used.

Root cause:

A newly created TypeBuilder token is not valid when ILGenerator.BeginCatchBlock(Type? exceptionType) called and the token will not be populated properly until the assembly saved into a file or stream.

Solution

Instead of adding exception handler blocks directly into the ControlFlowBuilder instance, now collecting them into a list and adding them to the ControlFlowBuilder during Save where all tokens will have populated properly.

Regression

  • Yes
  • No

It is a bug in a new PersistedAssemblyBuilder added in .NET 9

Testing

A unit test added that reproes the issue

Risk

Low, the fix is straight forward and clean, should not cause a regression

Side note

No new API added with the PR, instead if removed an overriden API from ref that is not needed to be overriden

Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

1 similar comment
Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

Copy link
Contributor

Tagging subscribers to this area: @dotnet/area-system-reflection-emit
See info in area-owners.md if you want to be subscribed.

Copy link
Member

@steveharter steveharter left a comment

Choose a reason for hiding this comment

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

FYI some unrelated errors in Build Analysis.

Copy link
Member

@jeffhandley jeffhandley left a comment

Choose a reason for hiding this comment

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

Once this is green, it's approved for merge into RC2 as it's a bug in a new feature in .NET 9.

@buyaa-n
Copy link
Contributor

buyaa-n commented Aug 22, 2024

/ba-g no failure related to PR or unknown

FYI: @carlossanlop

@buyaa-n buyaa-n added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Aug 22, 2024
Copy link
Member

@artl93 artl93 left a comment

Choose a reason for hiding this comment

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

M2 approved

@artl93 artl93 merged commit 6f23d04 into release/9.0 Aug 23, 2024
84 of 87 checks passed
@jkotas jkotas deleted the backport/pr-106665-to-release/9.0 branch August 24, 2024 04:47
@github-actions github-actions bot locked and limited conversation to collaborators Sep 23, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants