Use throw_unsup_format! instead of returning ENOTSUP in the mmap shim

[marc0246]

I noticed this while trying to use mmap with PROT_NONE, which resulted in this error message:

Os { code: 95, kind: Uncategorized, message: "<unknown errnum in strerror_r: 95>" }

cc @saethlin

[saethlin]

@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.

[RalfJung]

The program could set the last error itself and then print it, instead of relying on mmap?

[saethlin]

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:

miri/src/shims/unix/mem.rs

Lines 83 to 84 in e1099c6

	this.set_last_error(this.eval_libc("ENOTSUP"))?;
	return Ok(this.eval_libc("MAP_FAILED"));

[marc0246]

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.

[RalfJung]

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.)

[RalfJung]

@rustbot author

[marc0246]

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

[RalfJung]

Looks good, thanks!

@bors r+

[bors]

📌 Commit 424e5ed has been approved by RalfJung

It is now in the queue for this repository.

[bors]

⌛ Testing commit 424e5ed with merge 53481c4...

[bors]

☀️ Test successful - checks-actions
Approved by: RalfJung
Pushing 53481c4 to master...