Skip to content

Conversation

@cshung
Copy link
Contributor

@cshung cshung commented Nov 18, 2022

This change fixes an integer underflow that happens in the commit accounting, here is the sequence of events that will lead to it.

Under use_large_page_p, we created a new heap segment, we passed SEGMENT_INITIAL_COMMIT (which is just a page) to virtual_commit, so committed_by_oh[0] is increased by SEGMENT_INITIAL_COMMIT.

Later, the heap_segment_committed value for the segment is changed to size, which is the full region size.

Eventually, we return_free_region, which will subtract committed_by_oh[0] by the full region size, which will lead to a subtraction integer underflow.

This will result in a huge number, and that will fail future virtual_commit calls, leading to unexpected OOM.

The fix simply makes sure we add the full region size to the committed_by_oh[0] to begin with.

@cshung cshung requested a review from mrsharm November 18, 2022 01:24
@cshung cshung self-assigned this Nov 18, 2022
@ghost
Copy link

ghost commented Nov 18, 2022

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

Issue Details

This change fixes an integer underflow that happens in the commit accounting, here is the sequence of events that will lead to it.

Under use_large_page_p, we created a new heap segment, we passed SEGMENT_INITIAL_COMMIT (which is just a page) to virtual_commit, so committed_by_oh[0] is increased by SEGMENT_INITIAL_COMMIT.

Later, the heap_segment_committed value for the segment is changed to size, which is the full region size.

Eventually, we return_free_region, which will subtract committed_by_oh[0] by the full region size, which will lead to a subtraction integer underflow.

This will result in a huge number, and that will fail future virtual_commit calls, leading to unexpected OOM.

The fix simply makes sure we add the full region size to the committed_by_oh[0] to begin with.

Author: cshung
Assignees: cshung
Labels:

area-GC-coreclr

Milestone: -

@cshung cshung force-pushed the public/fix-commit-accounting branch from df7676d to aec1429 Compare November 18, 2022 01:31
@cshung cshung force-pushed the public/fix-commit-accounting branch from aec1429 to a86c1ac Compare November 18, 2022 02:07
@cshung cshung merged commit 793676e into dotnet:main Nov 18, 2022
@cshung cshung deleted the public/fix-commit-accounting branch November 18, 2022 16:08
@cshung
Copy link
Contributor Author

cshung commented Nov 18, 2022

/backport to release/7.0

@github-actions
Copy link
Contributor

Started backporting to release/7.0: https://github.com/dotnet/runtime/actions/runs/3498854226

@ghost ghost locked as resolved and limited conversation to collaborators Dec 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants