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.

Sign up for GitHub

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

Jump to bottom

Add mman::mprotect #991

Merged
merged 1 commit into from
Jan 23, 2019
Merged

Add mman::mprotect #991

merged 1 commit into from
Jan 23, 2019

Conversation

acfoltzer
Copy link
Contributor

It doesn't look like there's an existing test suite for mmap and friends, so this is regrettably untested.

@asomers
Copy link
Member

asomers commented Dec 5, 2018

Would you be willing to write a testsuite for mmap?

@acfoltzer
Copy link
Contributor Author

I don't think I'm the right person to do that; I don't know the APIs very deeply.

@acfoltzer
Copy link
Contributor Author

Is there anything else I need to do so that this can be merged? It keeps picking up conflicts in the changelog...

@asomers
Copy link
Member

asomers commented Dec 19, 2018

I would feel much better about the PR if there were some tests. Even a simple doc test would be good. Right now it's not obvious how to use it. Also, unsafe functions' documentation blocks should say why they're unsafe, and what the user must do to use them safely.

@acfoltzer
Copy link
Contributor Author

Got it; I was following the pattern of the rest of the definitions in that module, which don't have those things; I'll get a doctest merged shortly.

@acfoltzer
Copy link
Contributor Author

Well, it looks like my test is only gonna pass on Linux, due to differences in how mmap works there. This is part of why I was hesitant to add any tests for mprotect while mmap itself doesn't have any; it's a big, complicated API and I am not equipped to portably test it.

asomers
asomers requested changes Dec 19, 2018
View reviewed changes
src/sys/mman.rs Show resolved Hide resolved
src/sys/mman.rs Outdated Show resolved Hide resolved
src/sys/mman.rs Outdated
///
/// let mut slice: &mut [u8] = unsafe {
/// let mem = mmap(ptr::null_mut(), 1024, ProtFlags::PROT_NONE,
/// MapFlags::MAP_ANONYMOUS | MapFlags::MAP_PRIVATE, -1, 0).unwrap();
Copy link
Member

Choose a reason for hiding this comment

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

MAP_ANONYMOUS is a nonstandard flag, so it doesn't make a great example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've only ever used mmap with MAP_ANONYMOUS, but from what I can tell by the POSIX manpage, it would need a file descriptor if that flag isn't used. This has to either be backed by a real file or a typed memory object, the API for which nix doesn't support. This is starting to feel way too complicated for a 3 line wrapper function, and would be more appropriate for a larger mman test suite.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It looks like MAP_ANON at least passes on all the CI platforms, even though it's also not in POSIX. Good enough?

src/sys/mman.rs Outdated Show resolved Hide resolved
@acfoltzer acfoltzer force-pushed the mprotect branch 2 times, most recently from 6218aca to 6965172 Compare December 19, 2018 22:37
asomers
asomers requested changes Jan 4, 2019
View reviewed changes
Copy link
Member

@asomers asomers left a comment

Choose a reason for hiding this comment

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

You'll need to rebase this change before bors will agree to merge it. Our CI setup recently changed.

src/sys/mman.rs Outdated
/// See [`mprotect(3)`](http://pubs.opengroup.org/onlinepubs/9699919799/functions/mprotect.html) for
/// details.
///
/// # Safety Calls to `mprotect` are inherently unsafe, as changes to memory protections can lead to
Copy link
Member

Choose a reason for hiding this comment

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

You need a newline after "Safety". Also, is there any advice you can offer on how the user can use this function safely?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Newline fixed; Emacs got overly enthusiastic about reflowing that paragraph.

As for advice, anything I write would be just rewriting the linked manpage. It's such a broad API with so many potential implications that I think it would serve folks best to just use that as a resource.

@asomers
Copy link
Member

asomers commented Jan 7, 2019

Can you please squash your two commits into one? We can't do that ourselves because of bors.

@acfoltzer acfoltzer force-pushed the mprotect branch 2 times, most recently from b289f39 to 0d20c37 Compare January 7, 2019 19:24
@acfoltzer
Copy link
Contributor Author

Squashed and rebased on the current master; I think this CI failure is unrelated to this PR though

@acfoltzer
Copy link
Contributor Author

@asomers is this ready to merge now?

@asomers
Copy link
Member

asomers commented Jan 22, 2019

Uh, could you remove that odd blank line in the CHANGELOG?

@acfoltzer
Copy link
Contributor Author

Got it; of course the online editor first tried to make a merge conflict out of it :(

asomers
asomers approved these changes Jan 22, 2019
View reviewed changes
Copy link
Member

@asomers asomers left a comment

Choose a reason for hiding this comment

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

bors r+

bors bot added a commit that referenced this pull request Jan 22, 2019
@bors @acfoltzer
Merge #991
148871e 
991: Add `mman::mprotect` r=asomers a=acfoltzer

It doesn't look like there's an existing test suite for `mmap` and friends, so this is regrettably untested.

Co-authored-by: Adam C. Foltzer <acfoltzer@acfoltzer.net>
@bors
Copy link
Contributor

bors bot commented Jan 23, 2019

Build succeeded

@bors bors bot merged commit 94dbad2 into nix-rust:master Jan 23, 2019
@acfoltzer acfoltzer deleted the mprotect branch January 6, 2021 00:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@asomers asomers asomers approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@acfoltzer @asomers