Closed marc0246 closed 1 month ago
@marc0246 Can you add a test that verifies that a program which encounters ENOTSUP
can correctly print last_os_error
?
The precipitating program is this:
let ptr = unsafe {
libc::mmap(
std::ptr::null_mut(),
4096,
libc::PROT_NONE,
libc::MAP_PRIVATE | libc::MAP_ANONYMOUS,
-1,
0,
)
};
if ptr == libc::MAP_FAILED {
Err::<(), _>(std::io::Error::last_os_error()).unwrap();
}
Which before this PR prints:
thread 'main' panicked at src/main.rs:1:265:
called `Result::unwrap()` on an `Err` value: Os { code: 95, kind: Uncategorized, message: "<unknown errnum in strerror_r: 95>" }
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
note: in Miri, you may have to set `-Zmiri-env-forward=RUST_BACKTRACE` for the environment variable to have an effect
This scenario is close to a viable test case, the only complaint I'd have about this as a test is that it relies on the details of our mmap
implementation.
The program could set the last error itself and then print it, instead of relying on mmap
?
Do you think I should close the PR against std then?
libs decided not to FCP a colliding conversion from errno values to ErrorKind
so I think we shouldn't suggest adding one. (wishing I hadn't trusted my memory sigh I thought they had)
About this PR as a whole, we definitely need a better message for this mmap
misuse overall. Ralf said:
Though usually what we do in that case is just
throw_unsup_format!
, which can give a lot more context and so be much more informative. Is there a reason mmap doesn't do that?
There is no good reason. I waffled a lot while implementing the mmap
shim about whether we should return an error or throw_unsup_format!
. The core tension here is that if we return an error, callers can use some kind of fallback behavior. If we use throw_unsup_format!
, we halt the interpreter and the caller needs to add a cfg(miri)
to their code to make progress.
I suspect that given your experience and given that I've now been corrected that the standard library does not use ENOTSUP
anywhere, it seems dubious to return ENOTSUP
for these calls. I think now I'd prefer to throw_unsup_format!
with a descriptive message instead. We should probably provide a different string for use of MAP_FIXED
and use of protections other than PROT_READ | PROT_WRITE
. Here's the relevant code: https://github.com/rust-lang/miri/blob/e1099c653869d18d2590bc3366b74b96a70997b1/src/shims/unix/mem.rs#L83-L84
That sounds good to me. I for one will definitely agree that a recovery in this situation is doubtful, and the error has rather confused me.
Okay, so let's bring mmap in sync with all other Miri shims then by defaulting to throw_unsup_format.
@marc0246 can you update the PR to do that instead? (I don't think it needs a test, we don't usually test the "unsupported" paths.)
@rustbot author
This is what the UI looks like now:
error: unsupported operation: Miri does not support calls to mmap with protections other than PROT_READ|PROT_WRITE
--> src/lib.rs:215:17
|
215 | / libc::mmap(
216 | | ptr::null_mut(),
217 | | size,
218 | | libc::PROT_NONE,
... |
221 | | 0,
222 | | )
| |_________________^ Miri does not support calls to mmap with protections other than PROT_READ|PROT_WRITE
|
= help: this is likely not a bug in the program; it indicates that the program performed an operation that Miri does not support
@rustbot ready
Looks good, thanks!
@bors r+
:pushpin: Commit 424e5ed8fe3ec409723127209b19e962d9c293fe has been approved by RalfJung
It is now in the queue for this repository.
:hourglass: Testing commit 424e5ed8fe3ec409723127209b19e962d9c293fe with merge 53481c468db62d3c305499dcab213eacef6979eb...
:sunny: Test successful - checks-actions Approved by: RalfJung Pushing 53481c468db62d3c305499dcab213eacef6979eb to master...
I noticed this while trying to use
mmap
withPROT_NONE
, which resulted in this error message:cc @saethlin