Skip to content
New issue

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

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

Already on GitHub? # to your account

JIT: Ensure no overflow in ContainBlockStoreAddress #76532

Merged
merged 2 commits into from
Oct 3, 2022

Conversation

jakobbotsch
Copy link
Member

The offset here can be a "base" address due to various JIT transformations so we should ensure the range [offset, offset+size) does not overflow.

Fix #76506

The offset here can be a "base" address due to various JIT
transformations so we should ensure the range [offset, offset+size) does
not overflow.

Fix dotnet#76506
@dotnet-issue-labeler dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Oct 3, 2022
@ghost ghost assigned jakobbotsch Oct 3, 2022
@ghost
Copy link

ghost commented Oct 3, 2022

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

Issue Details

The offset here can be a "base" address due to various JIT transformations so we should ensure the range [offset, offset+size) does not overflow.

Fix #76506

Author: jakobbotsch
Assignees: -
Labels:

area-CodeGen-coreclr

Milestone: -

@jakobbotsch
Copy link
Member Author

Before #76273 we would see GT_ADDEX here so we wouldn't end up containing it.

@jakobbotsch
Copy link
Member Author

/azp run runtime-coreclr jitstress

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jakobbotsch jakobbotsch requested a review from EgorBo October 3, 2022 12:25
@kunalspathak
Copy link
Member

cc: @tannergooding

@@ -688,7 +688,12 @@ void Lowering::ContainBlockStoreAddress(GenTreeBlk* blkNode, unsigned size, GenT
{
return;
}
#endif // TARGET_ARM
#else // !TARGET_ARM
if ((ClrSafeInt<int>(offset) + ClrSafeInt<int>(size)).IsOverflow())
Copy link
Member

Choose a reason for hiding this comment

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

Can this overflow on arm too? Should this be outside the ifdef?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the ARM specific check above is fine -- we only get here for unrolled block copies so size is guaranteed to be small.

@tannergooding
Copy link
Member

tannergooding commented Oct 3, 2022

Some of the remaining failures may be related to the pending fix: #76517

@tannergooding
Copy link
Member

The failure for Runtime_40607 is #76546

@jakobbotsch
Copy link
Member Author

#76507 also still seems to be failing even with this fix.

@jakobbotsch
Copy link
Member Author

Bruce just opened #76550 for the remaining failure.

@jakobbotsch
Copy link
Member Author

jakobbotsch commented Oct 3, 2022

superpmi-diffs failure is #76542. superpmi-replay failure is #76511. Still just waiting for the win-arm64 testing to finish...

@jakobbotsch jakobbotsch merged commit a45611a into dotnet:main Oct 3, 2022
@jakobbotsch jakobbotsch deleted the fix-76506 branch October 3, 2022 19:18
@ghost ghost locked as resolved and limited conversation to collaborators Nov 3, 2022
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Test failure: access violation in cpblk tests
4 participants