Skip to content

Conversation

CraftSpider
Copy link
Contributor

No description provided.

@RalfJung
Copy link
Member

RalfJung commented Jun 3, 2025

There are some CI failures, could you look into that?
@rustbot author

@rustbot
Copy link
Collaborator

rustbot commented Jun 3, 2025

Reminder, once the PR becomes ready for a review, use @rustbot ready.

@rustbot rustbot added the S-waiting-on-author Status: Waiting for the PR author to address review comments label Jun 3, 2025
@CraftSpider
Copy link
Contributor Author

Well, I've just learned that attempting to read from a write-only file on Unix returns EBADF, which Rust maps to Uncategorized. Which surprises me for at least two reasons. But either way, I should be able to fix it by either:

  • Mapping EBADF specifically on Linux
  • Making the test allow this error.

The first though would likely be the first in a long line of this kind of change - several currently-disabled filesystem-related tests appear to rely on the specific OS error returned by functions, but this is a property of the backend, not the emulated target. If it's desired to always match the OS error kinds returned by the target, I'd want to brainstorm a more holistic way to handle the likely large amount of mapping that will require (and in some cases probably be impossible, since not all OSes will provide the same error granularity)

@RalfJung
Copy link
Member

RalfJung commented Jun 7, 2025

EBADF is the error you get when the FD doesn't even exist, so it sounds to me like something else is going on.

But, anyway, why does this test have to involve "reading from a write-only file"? You can mirror the test at

fn test_dup() {
let bytes = b"dup and dup2";
let path = utils::prepare_with_content("miri_test_libc_dup.txt", bytes);
let mut name = path.into_os_string();
name.push("\0");
let name_ptr = name.as_bytes().as_ptr().cast::<libc::c_char>();
unsafe {
let fd = libc::open(name_ptr, libc::O_RDONLY);
let mut first_buf = [0u8; 4];
libc::read(fd, first_buf.as_mut_ptr() as *mut libc::c_void, 4);
assert_eq!(&first_buf, b"dup ");
let new_fd = libc::dup(fd);
let mut second_buf = [0u8; 4];
libc::read(new_fd, second_buf.as_mut_ptr() as *mut libc::c_void, 4);
assert_eq!(&second_buf, b"and ");
let new_fd2 = libc::dup2(fd, 8);
let mut third_buf = [0u8; 4];
libc::read(new_fd2, third_buf.as_mut_ptr() as *mut libc::c_void, 4);
assert_eq!(&third_buf, b"dup2");
}
}

by having two FDs read from the same file and observing that reading one advances the position of the other. (The dup test doesn't even quite do that, it just checks that the position is copied over on dup; #4382 fixes that.)

@CraftSpider
Copy link
Contributor Author

I was hoping to verify that permissions were copied correctly, was the idea. I can just test position matching. That does still leave the underlying issue of the error kind being based on the miri target, not the emulated one. But that's also pre-existing.

Also, EBADF being the error for no read permissions, outside of Miri and with no dup involved, can be seen here: https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=7574e0de8939c17772ca441a6c64e1d8. I checked before concluding that was just the expected error.

@RalfJung
Copy link
Member

RalfJung commented Jun 8, 2025

Also, EBADF being the error for no read permissions, outside of Miri and with no dup involved, can be seen here: https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=7574e0de8939c17772ca441a6c64e1d8. I checked before concluding that was just the expected error.

👍

That does still leave the underlying issue of the error kind being based on the miri target, not the emulated one. But that's also pre-existing.

Yeah, that's not surprising, though maybe we should have an issue for this. It'd take a huge amount of code to match up the error kinds, I don't think it's worth it.

@CraftSpider
Copy link
Contributor Author

Just a note: Didn't forget this, I just bought a house, I'll get to this when I have time again.

@CraftSpider
Copy link
Contributor Author

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Waiting for a review to complete and removed S-waiting-on-author Status: Waiting for the PR author to address review comments labels Aug 27, 2025
@RalfJung
Copy link
Member

This looks great, thanks! Please squash the commits using ./miri squash. Then write @rustbot ready after you force-pushed the squashed PR.

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: Waiting for the PR author to address review comments and removed S-waiting-on-review Status: Waiting for a review to complete labels Aug 28, 2025
@CraftSpider CraftSpider force-pushed the duplicate-handle-test branch from aa18c78 to a16280c Compare August 29, 2025 18:15
@CraftSpider
Copy link
Contributor Author

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Waiting for a review to complete and removed S-waiting-on-author Status: Waiting for the PR author to address review comments labels Aug 29, 2025
@RalfJung RalfJung enabled auto-merge August 29, 2025 18:39
@RalfJung RalfJung added this pull request to the merge queue Aug 29, 2025
Merged via the queue into rust-lang:master with commit 95e40cd Aug 29, 2025
15 checks passed
@rustbot rustbot removed the S-waiting-on-review Status: Waiting for a review to complete label Aug 29, 2025
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

Successfully merging this pull request may close these issues.

3 participants