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

MemoryMap does not fail the task if munmap fails #13966

Closed
ghost opened this issue May 6, 2014 · 6 comments
Closed

MemoryMap does not fail the task if munmap fails #13966

ghost opened this issue May 6, 2014 · 6 comments

Comments

@ghost
Copy link

ghost commented May 6, 2014

os.rs:

#[cfg(unix)]
impl Drop for MemoryMap {
    /// Unmap the mapping. Fails the task if `munmap` fails.
    fn drop(&mut self) {
        if self.len == 0 { /* workaround for dummy_stack */ return; }

        unsafe {
            // FIXME: what to do if this fails?
            let _ = libc::munmap(self.data as *c_void, self.len as libc::size_t);
        }
    }
}

Same for Windows. Crypto needs this guarantee to protect sensitive data.

@lilyball
Copy link
Contributor

lilyball commented May 6, 2014

Does it really need this, or does it just need an explicit way to destroy the MemoryMap and return any error? I'm in favor of the latter. If you don't have security concerns, you don't want Drop to fail even if the munmap() does. If you do have security concerns, you can explicitly destroy the MemoryMap and handle any error as you see fit.

@thestinger
Copy link
Contributor

If munmap fails, I think the process should abort, not fail. The only normal failure reason (as in not from something like seccomp) is EINVAL, which is a logic error and indicates that there's horrible memory unsafety going on.

@lilyball
Copy link
Contributor

lilyball commented May 6, 2014

Reading the manpage, looks like @thestinger is right. munmap() failing signifies something serious is wrong.

@geraldstanje
Copy link

when will std::os::MemoryMap be stable?
i opened a new issue for those who used it before: #25538

@alexcrichton
Copy link
Member

As @klutzy pointed out in #25538 this has since been removed, so I'm going to close this, but it's definitely something that needs to be considered if re-adding!

@alexcrichton
Copy link
Member

@geraldstanje it's likely that this sort of functionality would start out as an external crate on crates.io before migrating into the standard library.

bors added a commit to rust-lang-ci/rust that referenced this issue Feb 13, 2023
Don't compute layout if `TargetDataLayout` is not available
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

No branches or pull requests

5 participants