Skip to content

[release/7.0] This fixes Github issue 78206 - a heap corruption problem associated with mark stack overflow #78855

New issue

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

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

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Nov 29, 2022

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Nov 25, 2022

Backport of #78756 to release/7.0

/cc @mangod9 @PeterSolMS

Customer Impact

In rare cases, mark stack overflow causes heap corruption, and thus access violations or other kinds of crashes.

Testing

Customer verified fix, we could not reproduce the issue locally.

Risk

Risk is low, because adding a call to drain_mark_queue can not cause problems. The places where a call to drain_mark_queue was replaced by a check (in debug) that the mark queue is empty were verified by code inspection.

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.

…with mark stack overflow.

Dumps provided by the customer showed in all cases that the min_overflow_address/max_overflow_address fields had values different from their initial values of MAX_PTR and 0. This implies that a mark stack overflow has occurred, but has not been properly handled.

Looking at the code, we realized that we may still have objects in the mark prefetch queue as we enter process_mark_overflow. These objects may cause another mark stack overflow when they are traced. So we need to drain the mark prefetch queue before we check the min_overflow_address/max_overflow_address fields.

We provided a private build of clrgc.dll to the customer reporting the issue, and customer has validated that the fix resolves the issue.
…ere the mark queue should be empty with asserts.
…_t::verify_empty instead of an assert testing the result from mark_queue_t::get_next_marked().
@ghost ghost added the area-GC-coreclr label Nov 25, 2022
@ghost
Copy link

ghost commented Nov 25, 2022

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

Issue Details

Backport of #78756 to release/7.0

/cc @mangod9 @PeterSolMS

Customer Impact

Testing

Risk

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.

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

area-GC-coreclr

Milestone: -

@carlossanlop
Copy link
Contributor

Please fill out the template. Request approval from Jeff Schwartz. If approved, add the servicing-consider label and send email to Tactics, then get a code review sign-off and verify CI is green.

Friendly reminder that the window for merging backports this month is only going to be of one day (29th-30th) so I want to make sure everything is ready for me to just press "merge" when the branch opens.

Copy link
Member

@jeffschwMSFT jeffschwMSFT left a comment

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

@jeffschwMSFT jeffschwMSFT added the Servicing-consider Issue for next servicing release review label Nov 29, 2022
@jeffschwMSFT jeffschwMSFT added this to the 7.0.x milestone Nov 29, 2022
@rbhanda rbhanda added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Nov 29, 2022
@rbhanda rbhanda modified the milestones: 7.0.x, 7.0.2 Nov 29, 2022
@carlossanlop
Copy link
Contributor

Branding has been done. Milestone is 7.0.2. Signed-off by area owners. Approved by Tactics. No OOB package authoring changes needed. CI is green.
Ready to merge. :shipit:

@carlossanlop carlossanlop merged commit d41078d into release/7.0 Nov 29, 2022
@carlossanlop carlossanlop deleted the backport/pr-78756-to-release/7.0 branch November 29, 2022 23:24
@ghost ghost locked as resolved and limited conversation to collaborators Dec 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-GC-coreclr Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants