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

Avoid integer overflow on ARM machine with 8G of memory #73271

Merged
merged 2 commits into from
Aug 3, 2022

Conversation

cshung
Copy link
Member

@cshung cshung commented Aug 2, 2022

Fixes #65466

On Raspberry Pi 4 model d03114, there is 8G of physical memory.

If we install the ARM dotnet SDK and run an application there, GetGCMemoryInfo.TotalAvailableMemoryBytes would return a negative number, and that is because we are hitting an integer overflow with the multiplication in this change.

Note that sysconf returns a long and that is a signed 32-bit number on ARM.

I confirmed that the negative value no longer repros on that machine, and it reports a number close to 8G.

@cshung cshung added this to the 7.0.0 milestone Aug 2, 2022
@cshung cshung requested a review from janvorli August 2, 2022 22:07
@cshung cshung self-assigned this Aug 2, 2022
@ghost
Copy link

ghost commented Aug 2, 2022

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

Issue Details

Fixes #65466

On Raspberry Pi 4 model d03114, there is 8G of physical memory.

If we install the ARM dotnet SDK and run an application there, GetGCMemoryInfo.TotalAvailableMemoryBytes would return a negative number, and that is because we are hitting an integer overflow with the multiplication in this change.

Note that sysconf returns a long and that is a signed 32-bit number on ARM.

I confirmed that the negative value no longer repros on that machine, and it reports a number close to 8G.

Author: cshung
Assignees: cshung
Labels:

area-GC-coreclr

Milestone: 7.0.0

Copy link
Member

@janvorli janvorli left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@jkotas
Copy link
Member

jkotas commented Aug 2, 2022

Can you fix the same issue in the standalone GC PAL:

return pages * pageSize;

@Maoni0
Copy link
Member

Maoni0 commented Aug 2, 2022

wouldn't be better if we just used unsigned exclusively here?

uint64_t physical_memory;
// Get the Physical memory size
physical_memory = ((uint64_t)sysconf( _SC_PHYS_PAGES )) * ((uint64_t) sysconf( _SC_PAGE_SIZE ));

@jkotas
Copy link
Member

jkotas commented Aug 3, 2022

Test failure tracked by #73299

@jkotas jkotas merged commit 7d7229c into dotnet:main Aug 3, 2022
@cshung cshung deleted the public/fix-arm-memory branch August 3, 2022 16:18
@ghost ghost locked as resolved and limited conversation to collaborators Sep 2, 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.

GetGCMemoryInfo.TotalAvailableMemoryBytes returns negative value on ARM32
4 participants