Skip to content
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

Fix a subtraction overflow in get_free_pages. #1261

Merged
merged 1 commit into from
Jan 14, 2025

Conversation

wks
Copy link
Collaborator

@wks wks commented Jan 14, 2025

The used pages can also be greater than the total pages for the same reason as those in computing get_available_pages, and it can also happen if the VM binding disabled GC, in which case we may over-allocate without triggering GC. When it overflows, get_free_pages will cause subtraction overflow, and will panic in debug build.

We switch to saturating_sub so that it will return 0 if overflow happens. It still makes sense. 0 means there is no free pages because we are over-allocating beyond the current heap size set by the GC trigger.

The used pages can also be greater than the total pages for the same
reason as those in computing `get_available_pages`, and it can also
happen if the VM binding disabled GC, in which case we may over-allocate
without triggering GC.  When it overflows, `get_free_pages` will cause
subtraction overflow, and will panic in debug build.

We switch to `saturating_sub` so that it will return 0 if overflow
happens.  It still makes sense.  0 means there is no free pages because
we are over-allocating beyond the current heap size set by the GC
trigger.
@wks wks requested a review from qinsoon January 14, 2025 06:55
@wks
Copy link
Collaborator Author

wks commented Jan 14, 2025

Rust 1.84 added a new lint unnecessary_map_or, and the style check failed for that reason. I'll fix that separately.

Copy link
Member

@qinsoon qinsoon left a comment

Choose a reason for hiding this comment

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

LGTM

@wks wks added this pull request to the merge queue Jan 14, 2025
Merged via the queue into mmtk:master with commit 541298f Jan 14, 2025
30 of 33 checks passed
@wks wks deleted the fix/free-pages-overflow branch January 14, 2025 14:04
peterzhu2118 added a commit to ruby/mmtk that referenced this pull request Jan 15, 2025
mmtk/mmtk-core#1261 fixes an issue where the following
script causes a Rust panic:

    GC.disable
    10_000.times { Object.new }
    puts GC.stat
matzbot pushed a commit to ruby/ruby that referenced this pull request Jan 15, 2025
mmtk/mmtk-core#1261 fixes an issue where the following
script causes a Rust panic:

    GC.disable
    10_000.times { Object.new }
    puts GC.stat

ruby/mmtk@6191ee994a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants