Skip to content

fix: use Padding in stat padding fields - #5497

Draft
xtqqczze wants to merge 1 commit into
rust-lang:libc-0.2from
xtqqczze:stat-padding
Draft

fix: use Padding in stat padding fields#5497
xtqqczze wants to merge 1 commit into
rust-lang:libc-0.2from
xtqqczze:stat-padding

Conversation

@xtqqczze

@xtqqczze xtqqczze commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Use Padding in stat and stat64 for fields like st_spare, __pad, __unused

@tgross35

tgross35 commented Sep 7, 2026

Copy link
Copy Markdown
Member

I think this may overlap with #5444

pub st_ctime_nsec: c_long,
pub st_size: off_t,
pub st_blocks: i64,
pub __old_st_blksize: u32,

@dybucc dybucc Sep 7, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This change is not part of #5444.

View changes since the review

pub __pad1: c_uint,
pub __st_ino: crate::ino_t,
__pad1: Padding<c_uint>,
__st_ino: crate::ino_t,

@dybucc dybucc Sep 7, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This change is not part of #5444.

View changes since the review

pub st_dev: crate::dev_t,
__pad1: Padding<c_ushort>,
pub __st_ino: crate::ino_t,
__st_ino: crate::ino_t,

@dybucc dybucc Sep 7, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This change is not part of #5444.

View changes since the review

pub __pad1: c_uint,
pub __st_ino: crate::ino_t,
__pad1: Padding<c_uint>,
__st_ino: crate::ino_t,

@dybucc dybucc Sep 7, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This change is not part of #5444.

View changes since the review

Comment thread src/unix/nto/mod.rs Outdated
Comment on lines +114 to +116
__old_st_mtime: crate::_Time32t,
__old_st_atime: crate::_Time32t,
__old_st_ctime: crate::_Time32t,

@dybucc dybucc Sep 7, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This change is not part of #5444.

View changes since the review

@dybucc

dybucc commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

I just looked through the patch, and left some comments where there were
changes not included in #5444. They are minimal, but some of them affect
previously public fields. I can include them in #5444, but that is not
for me to decide.

@xtqqczze

xtqqczze commented Sep 7, 2026

Copy link
Copy Markdown
Contributor Author

@dybucc I don't think you should include the additional changes in #5444. If necessary the changes such as removing pub from __st_ino, __old_st_blksize and __old_st-* could be made in a seperate PR.

@dybucc

dybucc commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

@dybucc I don't think you should include the additional changes in
#5444. If necessary the changes
such as removing pub from __st_ino, __old_st_blksize and
__old_st-* could be made in a seperate PR.

Sure thing.

@xtqqczze

xtqqczze commented Sep 7, 2026

Copy link
Copy Markdown
Contributor Author

I’ve removed the unnecessary field-visibility changes from this PR, so it is now superseded by #5444.

However, I note that #5444 was declined for stable. These changes are necessary to prevent Undefined Behavior, as reported in bytecodealliance/rustix#1668.

@tgross35 Since this PR is already targeted at libc-0.2, could these changes still be considered for inclusion here?

@xtqqczze xtqqczze changed the title fix: use Padding in stat private fields fix: use Padding in stat padding fields Sep 7, 2026

@tgross35 tgross35 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Realizing now this is targeted for 0.2. I won't want to take this change since these are some of the most popular structs in the crate and they happen to have all fields public on some platforms. Just means there's a higher chance of breakage than most other changes like this.

View changes since this review

@rustbot

rustbot commented Sep 8, 2026

Copy link
Copy Markdown
Collaborator

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

@tgross35

tgross35 commented Sep 8, 2026

Copy link
Copy Markdown
Member

However, I note that #5444 was declined for stable. These changes are necessary to prevent Undefined Behavior, as reported in bytecodealliance/rustix#1668.

Also, commented on the other thread but so it's here as well: libc's definitions are correct, it's not our bug. rustix is doing a bad assume_init, among many others, and Miri just happened to flag the problem on stat.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants