Support fstat on non-file-backed FDs#4812
Support fstat on non-file-backed FDs#4812enthropy7 wants to merge 2 commits intorust-lang:masterfrom
Conversation
Implement fstat support for sockets, pipes, eventfd, epoll, and stdin/stdout/stderr file descriptors. Fixes scalar size mismatch error by using to_uint with scalar's own size instead of to_u32(). Fixes rust-lang#4753
|
Thank you for contributing to Miri! A reviewer will take a look at your PR, typically within a week or two. |
|
| let buf = this.deref_pointer(buf_op)?; | ||
|
|
||
| // Extract mode value. write_int will handle size conversion when writing to st_mode field. | ||
| let mode = metadata.mode.to_uint(metadata.mode.size())?.try_into().unwrap_or(0u32); |
There was a problem hiding this comment.
This basically bypasses the sanity check that made has the right type. Why is this needed?
| fd_name: &str, | ||
| ) -> InterpResult<'tcx, Result<FileMetadata, IoError>> { | ||
| // Determine the file type and mode based on the FD name | ||
| let (mode_name, size) = match fd_name { |
There was a problem hiding this comment.
The name is entirely a diagnostic tool. It should never be matched on in the program itself -- this will silently break if we change the string or add a typo. It also will silently do the wrong thing when new FD types are added.
Instead, the metadata method on those FD types should be adjusted to return suitable metadata.
| // Convert Scalars to u32 using to_uint with their own size, then combine with bitwise OR. | ||
| let mode_base_u32 = mode_base.to_uint(mode_base.size())?.try_into().unwrap_or(0u32); | ||
| let permissions_u32 = if mode_name == "S_IFSOCK" || mode_name == "S_IFIFO" { | ||
| ecx.eval_libc("S_IRUSR") |
There was a problem hiding this comment.
All these look like they could just use eval_libc_u32.
| // Check that all fields are initialized (at least accessible) | ||
| let _st_size = stat.st_size; | ||
| let _st_nlink = stat.st_nlink; | ||
| let _st_blksize = stat.st_blksize; | ||
| let _st_blocks = stat.st_blocks; | ||
| let _st_ino = stat.st_ino; | ||
| let _st_dev = stat.st_dev; | ||
| let _st_uid = stat.st_uid; | ||
| let _st_gid = stat.st_gid; | ||
| let _st_rdev = stat.st_rdev; | ||
| let _st_atime = stat.st_atime; | ||
| let _st_mtime = stat.st_mtime; | ||
| let _st_ctime = stat.st_ctime; | ||
| let _st_atime_nsec = stat.st_atime_nsec; | ||
| let _st_mtime_nsec = stat.st_mtime_nsec; | ||
| let _st_ctime_nsec = stat.st_ctime_nsec; |
There was a problem hiding this comment.
Please move this into a function instead of repeating the code 3 times.
| // stdout is typically a character device (S_IFCHR) or a regular file | ||
| // We just check that it's not an error | ||
| let file_type = stat.st_mode & libc::S_IFMT; | ||
| assert!( | ||
| file_type == libc::S_IFCHR || file_type == libc::S_IFREG, | ||
| "stdout should be S_IFCHR or S_IFREG, got {:#o}", | ||
| file_type | ||
| ); |
There was a problem hiding this comment.
In this test we know there has been no output redirection to a file. So it can never be a regular file here, can it?
| // Get current user/group IDs from a temporary file or current directory | ||
| let (dev, uid, gid) = match std::fs::metadata("/tmp") | ||
| .or_else(|_| std::fs::metadata(".")) | ||
| { | ||
| Ok(temp_meta) => { | ||
| (temp_meta.dev(), temp_meta.uid(), temp_meta.gid()) | ||
| } |
There was a problem hiding this comment.
This seems like a bad idea -- '/tmp and . have nothing to do with this FD so the values can easily be nonsense, and the overall behavior can be very confusing.
| // Add default permissions (0666 for sockets/pipes, 0600 for others). | ||
| // Convert Scalars to u32 using to_uint with their own size, then combine with bitwise OR. | ||
| let mode_base_u32 = mode_base.to_uint(mode_base.size())?.try_into().unwrap_or(0u32); | ||
| let permissions_u32 = if mode_name == "S_IFSOCK" || mode_name == "S_IFIFO" { |
There was a problem hiding this comment.
From what I can see we currently entirely ignore the permission part in FileMetadata::from_meta. So seems like we could also get away with ignoring it for non-file-FDs for now?
rustbot
added
S-waiting-on-author
|
Reminder, once the PR becomes ready for a review, use |
|
Hi, thanks for your answer! i’m still here, just a little busy with my work. i will answer to your comments as soon as i can and optimise my PR better. I think it would took around 3 days for me to fix this |
3 participants
Implement fstat support for sockets, pipes, eventfd, epoll, and stdin/stdout/stderr file descriptors. Fixes scalar size mismatch error by using to_uint with scalar's own size instead of to_u32().
Fixes #4753