[GC] Fix freelist crash - don't modify during check-only operation #104876
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Tagging subscribers to this area: @dotnet/gc |
mangod9
approved these changes
Jul 15, 2024
|
The build analysis failure is #104883, which wasn't open yet when this testing ran but is closed now so rerunning just build analysis doesn't work. |
cshung
reviewed
Jul 15, 2024
| @@ -21507,10 +21507,13 @@ int gc_heap::generation_to_condemn (int n_initial, | |||
| } | |||
|
|
|||
| #ifdef USE_REGIONS | |||
| if (!try_get_new_free_region()) | |||
| if (!check_only_p) | |||
There was a problem hiding this comment.
NIT: Using && to avoid changing many lines?
There was a problem hiding this comment.
That annoyed me a bit too, but I followed the pattern at line 21544 instead. I assumed that it was to make it clear by separating the "mode" of the function and the check itself, which made sense to me.
|
/ba-g see above comment about #104883 |
|
/backport to release/8.0-staging |
|
Started backporting to release/8.0-staging: https://github.com/dotnet/runtime/actions/runs/9962984209 |
Merged
4 tasks
markples
pushed a commit
that referenced
this pull request
Jul 25, 2024
…104992) Backport of #104876 to release/8.0-staging /cc @markples ## Customer Impact - [x] Customer reported - [ ] Found internally GC can crash in region code if GC.RegisterForFullGCNotification is used. ## Regression - [x] Yes - [ ] No Regression is part of general move to regions, but specific code was introduced in #48691 ## Testing The behavior is very difficult to repro and likely hadn't occurred before because it relies on a race condition, a region allocation failing, and GC.RegisterForFullGCNotification be used. I built a custom GC that always triggers the region allocation failure path and was able to repro and verify the fix in GCPerfSim. ## Risk Low: only impacts GC.RegisterForFullGCNotification path; only impacts notifications; may cause a notification for a full GC to be skipped, but only in a case where no other reasons for full gc occur -and- the code would be subject to the race condition and crash
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
RegisterForFullGCNotificationsetsgc_heap::fgn_maxgen_percent, which guards calls tocheck_for_full_gcto determine if a notification should be sent.gc_heap::generation_to_condemn. One part ofcheck_for_full_gccallsgeneration_to_condemnwith the parametercheck_only_pset toTRUE.generation_to_condemncallstry_get_new_free_region, which ensures that a free region is available. It checks the freelist, but if none are found it creates a new one and adds it to the freelist.check_for_full_gcdoes not (and should not) contain the necessary synchronization to make this safe. This can lead to corruption of the basic region freelist, often ending up withnullptrlinks between nodes.This caused crashed in production and the cause was determined by using a custom GC build with additional runtime checks. The fix is to simply honor the
check_only_pparameter and not calltry_get_new_free_regionwhen it is set. The problem can be reproed by changingtry_get_new_free_regionto always create a new region. This fix handles that repro.This fix does not impact normal GC operation. It can cause
check_for_full_gcto no longer detect that full collection is going to occur, but only if the failure to create or initialize a new region is the only reason why that full collection is going to occur. However, those cases are open to the race condition that can cause the freelist corruption is possible.