Skip to content

[release/6.0] Add explicit null-check for tailcalls to VSD - #62769

Merged
jeffschwMSFT merged 2 commits into
release/6.0from
backport/pr-62719-to-release/6.0
Jan 7, 2022
Merged

[release/6.0] Add explicit null-check for tailcalls to VSD#62769
jeffschwMSFT merged 2 commits into
release/6.0from
backport/pr-62719-to-release/6.0

Conversation

@github-actions

@github-actionsgithub-actionsBot commented Dec 14, 2021

Copy link
Copy Markdown
Contributor

Backport of #62719 to release/6.0

/cc @jakobbotsch

Customer Impact

Certain reflection invocations and exception handling patterns that should throw NullReferenceException may instead crash the runtime due to an interaction between tailcalls to interface methods with a null receiver, and how the runtime maps access violations to NRE. Hit by customer in #61486.

Testing

Regression test included.

Regression

The same case reproduces in .NET 5, however the issue may be more common in .NET 6 because the JIT tailcalls more often.

Risk

Low, this changes the JIT to null-check ahead of time in this particular case.

There is already a comment that this is necessary, but it is only being
done for x86 tailcalls via jit helper. Do it for normal tailcalls to VSD
as well.
Fix#61486
@ghostghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Dec 14, 2021
@ghost

Copy link
Copy Markdown

Tagging subscribers to this area: @JulieLeeMSFT
See info in area-owners.md if you want to be subscribed.

Issue Details

Backport of #62719 to release/6.0

/cc @jakobbotsch

Customer Impact

Testing

Risk

Author:github-actions[bot]
Assignees:-
Labels:

area-CodeGen-coreclr

Milestone:-

@safernsafern added the Servicing-consider Issue for next servicing release review label Dec 15, 2021
@safern

Copy link
Copy Markdown
Member

@jeffschwMSFT for awareness and approval.

@jakobbotsch

Copy link
Copy Markdown
Member

Note that #62058 was reopened, so we may wish to hold off on this until that has been investigated further.

@jeffschwMSFTjeffschwMSFT removed the Servicing-consider Issue for next servicing release review label Jan 3, 2022
@janvorli

Copy link
Copy Markdown
Member

This can be merged. The other issue @jakobbotsch mentioned as reopened turned out to be unrelated and I am just about to create a PR and then a porting PR for it.

@jeffschwMSFTjeffschwMSFT added the Servicing-approved Approved for servicing release label Jan 7, 2022
@jeffschwMSFTjeffschwMSFT added this to the 6.0.2 milestone Jan 7, 2022
@jeffschwMSFT

Copy link
Copy Markdown
Member

Approved offline

@jeffschwMSFT
jeffschwMSFT merged commit d575a6f into release/6.0Jan 7, 2022
@safern
safern deleted the backport/pr-62719-to-release/6.0 branch January 7, 2022 19:01
@ghostghost locked as resolved and limited conversation to collaborators Feb 7, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-CodeGen-coreclrCLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMIServicing-approvedApproved for servicing release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@safern@jakobbotsch@janvorli@jeffschwMSFT@jkotas