Fix ICE caused by seeking past i64::MAX
#3689
Conversation
src/shims/unix/fs.rs
Outdated
| SeekFrom::Start(u64::try_from(offset).unwrap()) | ||
| // Be consistent with standard library. | ||
| #[allow(clippy::cast_sign_loss)] | ||
| SeekFrom::Start(i64::try_from(offset).unwrap() as u64) |
There was a problem hiding this comment.
We're implementing POSIX lseek here, it doesn't matter what the Rust standard library does.
The real question is what does POSIX say should happen when offset is negative in SEEK_SET mode?
tests/pass/issues/issue-miri-3680.rs
Outdated
|
|
||
| let result = catch_unwind(|| { | ||
| let mut f = std::fs::File::create(&path).unwrap(); | ||
| f.seek(std::io::SeekFrom::Start(i64::MAX as u64 + 1)).unwrap(); |
There was a problem hiding this comment.
Looks like on a Windows host, we are getting "unsupported io error". Curious that this doesn't happen on Linux hosts, I guess they generate different io::ErrorKind for this case?
There was a problem hiding this comment.
There's no named constant for error 131, so on windows this becomes ErrorKind::Other and then we can't map it to a target error.
I think if the offset is negative we probably don't want to call file_descriptor.seek at all, and instead return EINVAL directly ourselves.
There was a problem hiding this comment.
Indeed. So I guess test should only be run for unix host. No idea how to configure this, but @only-host-linux is good enough?
Sounds good.
There was a problem hiding this comment.
No it should run on all hosts, Miri should just behave consistently.
There was a problem hiding this comment.
Oh, I got it. We only need to know Miri can interpret this operation successfully without ICE so the stderr output doesn't matter.
How can I configure the test header for this? I tried @rustc-env: MIRI_SKIP_UI_CHECKS=1 and it still compares output.
There was a problem hiding this comment.
No stderr output should be checked.
If you do this...
I think if the offset is negative we probably don't want to call file_descriptor.seek at all, and instead return EINVAL directly ourselves.
... then I think all the troubles with the test will disappear.
There was a problem hiding this comment.
Understood now. No need to panic on purpose thus no stderr is emitted.
There was a problem hiding this comment.
No, that's not what I meant. The error code will be the same across all targets if you implement what I quoted above, so the stderr will be the same.
But checking the returned error instead of printing it is also fine, and avoids having to spawn a thread or use catch_unwind, so it's probably better.
tests/pass/issues/issue-miri-3680.rs
Outdated
| let result = f.seek(std::io::SeekFrom::Start(i64::MAX as u64 + 1)); | ||
|
|
||
| // It should be error due to negative offset. | ||
| assert!(result.is_err()); |
There was a problem hiding this comment.
You can even check that it returned ErrorKind::InvalidInput.
There was a problem hiding this comment.
But for windows, it's ErrorKind::Other. Should I write platform-specific check?
There was a problem hiding this comment.
Sorry, I misunderstood the codebase. I thought miri/src/shims/unix/fs.rs was platform-specific code and would be used for unix host only.
There was a problem hiding this comment.
It is used for Unix targets only :)
|
Looks good, thanks! @bors r+ |
|
☀️ Test successful - checks-actions |
Make Miri behave the same as standard library on file seeking offset.
Fixes #3680.