-
Notifications
You must be signed in to change notification settings - Fork 320
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No it should run on all hosts, Miri should just behave consistently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Understood now. No need to panic on purpose thus no stderr is emitted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can even check that it returned ErrorKind::InvalidInput
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But for windows, it's ErrorKind::Other
. Should I write platform-specific check?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.