[release/7.0] [Mono] Fix OP_CHECK_THIS to read 1 byte instead of 4/8 on x86/x64/LLVM. #74762
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Current implementation of OP_CHECK_THIS on x86/x64 and LLVM does a memory read of at least 4 bytes. This creates an issue when the target is a managed pointer, since that could point to the interior of a type, meaning it can read pass the allocated memory causing a crash. Fix change the size of the read to one byte since the only reason doing the read is to validate that the reference, managed pointer is not NULL. Reading only one byte is also inline with how it is implemented on arm/arm64, and it will reduce potential unaligned reads on x86/x64. Full fix for, #74179.
github-actions
bot
requested review from
vargaz,
lambdageek and
SamMonoRT
as code owners
|
Ping @marek-safar , needs an approval. |
|
Re-running the failed legs. |
|
@lateralusX FYI the re-runs passed and it was approved/signed-off, but I'll wait for your confirmation to merge. |
|
/azp run runtime-extra-platforms |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Failure in tvOS arm64 unrelated and occurring on a number of PR's. |
|
@lateralusX thanks for running the additional legs. Is there anything else you wanted to check, or can I merge now? |
|
Merged PR into main today, could we let it run there for a day and if nothing pops up, merge it here during tomorrow? |
|
@lateralusX the CI went down for maintenance yesterday, so there were no CI runs between 4pm PDT yesterday and around 9am PDT today. So if you want to wait an extra day to monitor the main branch, that's good too. We still have time. |
|
CI is still catching up. I have monitored the few PR's running any Mono related in main so far, and looks OK, will look at a couple more once CI is getting back on track. |
|
@carlossanlop, checked a several PR builds on runtime main and failures seems unrelated to corresponding change in main, so believe this is good to go. |
|
Ping @marek-safar need an approval please. |
|
Approved and signed off. |
ghost
locked as resolved and limited conversation to collaborators
Backport of #74638 to release/7.0
/cc @lateralusX
Codegen fix for #74179.
Customer Impact
Testing
Run and pass in main where tests have been re-enabled that original triggered the issue. Tests will be re-enabled on release/7.0 through #74738.
Risk
Low, code path should be frequently run-on CI and have been hit in the past by several tests cases, re-enabled by #74738.
IMPORTANT: Is this backport for a servicing release? If so and this change touches code that ships in a NuGet package, please make certain that you have added any necessary package authoring and gotten it explicitly reviewed.