Skip to content

[release/7.0] Crash when region survives >2GB - #80392

Merged
carlossanlop merged 3 commits into
dotnet:release/7.0from
mrsharm:backport_AVFSharp
Jan 11, 2023
Merged

[release/7.0] Crash when region survives >2GB#80392
carlossanlop merged 3 commits into
dotnet:release/7.0from
mrsharm:backport_AVFSharp

Conversation

@mrsharm

@mrsharmmrsharm commented Jan 9, 2023

Copy link
Copy Markdown
Member

Backport of #80302 that fixes#79486

Customer Impact

The impact of this bug is constrained to users who have very large LOH segments where one segment survives more than 2GB - leading to a crash.

Testing

We were able to test locally after consistently repro'ing the issue and proved that the fix took care of the bug.

Risk

Low risk change since it fixes the semantics of a type that was meant to be unsigned (from an int).

@ghostghost added the area-GC-coreclr label Jan 9, 2023
@ghostghost assigned mrsharmJan 9, 2023
@mrsharm
mrsharm requested a review from cshungJanuary 9, 2023 21:13
@ghost

ghost commented Jan 9, 2023

Copy link
Copy Markdown

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

Issue Details

Backport of #80302 that fixes #79486

Author:mrsharm
Assignees:-
Labels:

area-GC-coreclr

Milestone:-

@mrsharmmrsharm added the Servicing-consider Issue for next servicing release review label Jan 9, 2023

@Maoni0Maoni0 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this actually doesn't look like it has most of the fix, for example, the part where it changes int to size_t for the survived field?

@ghostghost added the needs-author-action An issue or pull request that requires more info or actions from the author. label Jan 10, 2023

@jeffschwMSFTjeffschwMSFT left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

approved. we will take for consideration in 7.0.x

@jeffschwMSFTjeffschwMSFT added this to the 7.0.x milestone Jan 10, 2023
@mrsharm
mrsharm marked this pull request as draft January 10, 2023 16:11
@jeffschwMSFTjeffschwMSFT changed the title Backport to 7.0 of 80302 that fixes #79486[release/7.0] Crash when region survives >2GBJan 10, 2023
@ghostghost removed the needs-author-action An issue or pull request that requires more info or actions from the author. label Jan 10, 2023
@mrsharm
mrsharm marked this pull request as ready for review January 10, 2023 17:00
@mrsharm

Copy link
Copy Markdown
MemberAuthor

Thanks for the review! This PR is now ready and has been tested by confirming:

  1. That the fix causes us to not AV thereby fixing the issue.
  2. That if the user specifies COMPlus_GCRegionSize >= 2GB, we return a CLR_E_GC_BAD_REGION_SIZE.

@leecowleecow added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Jan 10, 2023
@leecowleecow modified the milestones: 7.0.x, 7.0.3Jan 10, 2023
@mrsharm
mrsharm requested a review from Maoni0January 10, 2023 21:18
@mrsharm

Copy link
Copy Markdown
MemberAuthor

Failures don't look related to the changes.

@PeterSolMSPeterSolMS left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good to me!

@carlossanlop

Copy link
Copy Markdown
Contributor

Approved by Tactics (7.0.3).
Signed off by area owners.
No OOB changes needed (native coreclr code).
All the CI failures are #80428
Ready to merge. :shipit:

@carlossanlop
carlossanlop merged commit f5f9a70 into dotnet:release/7.0Jan 11, 2023
@ghostghost locked as resolved and limited conversation to collaborators Feb 10, 2023
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-GC-coreclrServicing-approvedApproved for servicing release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants

@mrsharm@carlossanlop@jeffschwMSFT@Maoni0@PeterSolMS@mangod9@leecow