Skip to content
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

Update ReadDir::next in std::sys::pal::unix::fs to use &raw const (*p).field instead of p.byte_offset().cast() #134678

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 14 additions & 22 deletions library/std/src/sys/pal/unix/fs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -713,7 +713,7 @@ impl Iterator for ReadDir {
// thread safety for readdir() as long an individual DIR* is not accessed
// concurrently, which is sufficient for Rust.
super::os::set_errno(0);
let entry_ptr = readdir64(self.inner.dirp.0);
let entry_ptr: *const dirent64 = readdir64(self.inner.dirp.0);
if entry_ptr.is_null() {
// We either encountered an error, or reached the end. Either way,
// the next call to next() should return None.
Expand All @@ -739,44 +739,36 @@ impl Iterator for ReadDir {
// contents were "simply" partially initialized data.
//
// Like for uninitialized contents, converting entry_ptr to `&dirent64`
// would not be legal. However, unique to dirent64 is that we don't even
// get to use `&raw const (*entry_ptr).d_name` because that operation
// requires the full extent of *entry_ptr to be in bounds of the same
// allocation, which is not necessarily the case here.
//
// Instead we must access fields individually through their offsets.
macro_rules! offset_ptr {
($entry_ptr:expr, $field:ident) => {{
const OFFSET: isize = mem::offset_of!(dirent64, $field) as isize;
if true {
// Cast to the same type determined by the else branch.
$entry_ptr.byte_offset(OFFSET).cast::<_>()
} else {
#[allow(deref_nullptr)]
{
&raw const (*ptr::null::<dirent64>()).$field
}
}
// would not be legal. However, we can use `&raw const (*entry_ptr).d_name`
// to refer the fields individually, because that operation is equivalent
// to `byte_offset` and thus does not require the full extent of `*entry_ptr`
// to be in bounds of the same allocation, only the offset of the field
// being referenced.
macro_rules! entry_field_ptr {
($field:ident) => {{
// To make sure we aren't silently doing any Deref coercion.
const _: usize = mem::offset_of!(dirent64, $field);
&raw const (*entry_ptr).$field
}};
}

// d_name is guaranteed to be null-terminated.
let name = CStr::from_ptr(offset_ptr!(entry_ptr, d_name).cast());
let name = CStr::from_ptr(entry_field_ptr!(d_name).cast());
let name_bytes = name.to_bytes();
if name_bytes == b"." || name_bytes == b".." {
continue;
}

#[cfg(not(target_os = "vita"))]
let entry = dirent64_min {
d_ino: *offset_ptr!(entry_ptr, d_ino) as u64,
d_ino: *entry_field_ptr!(d_ino) as u64,
#[cfg(not(any(
target_os = "solaris",
target_os = "illumos",
target_os = "aix",
target_os = "nto",
)))]
d_type: *offset_ptr!(entry_ptr, d_type) as u8,
d_type: *entry_field_ptr!(d_type) as u8,
Comment on lines -772 to +771
Copy link
Contributor

@tgross35 tgross35 Dec 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know why the as casts are there, no. dirent64 is imported as aliases from various different platforms, so maybe the as casts were added just to make sure it worked (or at least compiled) on any weird platforms where the fields were different type?

Removing the as casts doesn't make any platforms I've tried with ./x.py check library/std complain though1, so maybe they could be removed?

Footnotes

  1. x86_64-unknown-linux-gnu, aarch64-unknown-linux-gnu, aarch64-unknown-freebsd, arm-linux-androideabi, armv7-linux-androideabi, i686-linux-android

};

#[cfg(target_os = "vita")]
Expand Down
Loading