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

Return NULL for mmap fail case on Unix #78069

Merged
merged 7 commits into from
Nov 10, 2022

Conversation

wscho77
Copy link
Contributor

@wscho77 wscho77 commented Nov 8, 2022

If mmap failed, "MAP_FAILED" is returned not "NULL".
The windows implememtation of GetRWMapping returns "NULL" for fail case, and the caller function is also checking "NULL".
So, change Unix implementation to return "NULL" for fail case.

If mmap failed, "MAP_FAILED" is returned not "NULL".
The windows implememtation of GetRWMapping returns "NULL" for fail case,
and the caller function is also checking "NULL".

So, change Unix implementation to return "NULL" for fail case.
@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@Wraith2
Copy link
Contributor

Wraith2 commented Nov 8, 2022

This is the second use of nmap that needed changing recently,
#77952 is it worth auditing all other uses?

@wscho77
Copy link
Contributor Author

wscho77 commented Nov 8, 2022

I checked all mmap return value and add missing one.

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! Sigh, I wonder how I could get it wrong at that many places.

src/coreclr/minipal/Unix/doublemapping.cpp Outdated Show resolved Hide resolved
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.

Ah, I've approved it accidentally, we will need to fix the thing I've commented on in around the memset.

@wscho77 wscho77 requested review from janvorli and removed request for MichalStrehovsky November 9, 2022 10:08
@janvorli
Copy link
Member

janvorli commented Nov 9, 2022

Hmm, some CI legs are failing with the newly added fatal error.
Ah, I can see the problem, the condition added in the VMToOSInterface::ReleaseDoubleMappedMemory is reverse.

@runfoapp runfoapp bot mentioned this pull request Nov 9, 2022
@ghost ghost added the needs-author-action An issue or pull request that requires more info or actions from the author. label Nov 9, 2022
@ghost ghost removed the needs-author-action An issue or pull request that requires more info or actions from the author. label Nov 9, 2022
@wscho77
Copy link
Contributor Author

wscho77 commented Nov 9, 2022

Hmm, some CI legs are failing with the newly added fatal error. Ah, I can see the problem, the condition added in the VMToOSInterface::ReleaseDoubleMappedMemory is reverse.

Oops. I fixed condition check. Thank you.

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!

@janvorli janvorli merged commit 39e9c6b into dotnet:main Nov 10, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Dec 10, 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