Uh oh!
There was an error while loading. Please reload this page.
[release/7.0] [Mono] Fix OP_CHECK_THIS to read 1 byte instead of 4/8 on x86/x64/LLVM. - #74762
Conversation
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.
carlossanlop
commented
Aug 30, 2022
Ping @marek-safar , needs an approval. |
carlossanlop
commented
Aug 30, 2022
Re-running the failed legs. |
carlossanlop
commented
Aug 31, 2022
@lateralusX FYI the re-runs passed and it was approved/signed-off, but I'll wait for your confirmation to merge. |
lateralusX
commented
Aug 31, 2022
/azp run runtime-extra-platforms |
|
Azure Pipelines successfully started running 1 pipeline(s). |
lateralusX
commented
Aug 31, 2022
Failure in tvOS arm64 unrelated and occurring on a number of PR's. |
carlossanlop
commented
Aug 31, 2022
@lateralusX thanks for running the additional legs. Is there anything else you wanted to check, or can I merge now? |
lateralusX
commented
Aug 31, 2022
Merged PR into main today, could we let it run there for a day and if nothing pops up, merge it here during tomorrow? |
carlossanlop
commented
Sep 1, 2022
@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. |
lateralusX
commented
Sep 2, 2022
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. |
lateralusX
commented
Sep 5, 2022
@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. |
carlossanlop
commented
Sep 6, 2022
Ping @marek-safar need an approval please. |
carlossanlop
commented
Sep 7, 2022
Approved and signed off. |
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.