-
Notifications
You must be signed in to change notification settings - Fork 1.1k
android: use proper types for dirent.d_ino, dirent.d_off, stat.st_mode and stat64.st_mode #4216
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
base: main
Are you sure you want to change the base?
android: use proper types for dirent.d_ino, dirent.d_off, stat.st_mode and stat64.st_mode #4216
Conversation
and stat64.st_mode This bug was noticed in termux/termux-packages#22609
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @tgross35 (or someone else) some time within the next two weeks. Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (
|
Thanks for the detailed sources. Is the following delta accurate?
These issues seem reasonably severe so I am surprised it has not been noticed before (@maurer do you know of anything?). I guess the other fields at least keep the relevant fields here correctly aligned?
For reference, the command can't be in backticks :) @rustbot label stable-nominated |
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.
Fixing up mode_t
/ino_t
might be worth doing, but the changes you're making to these structs will actively break things on certain platforms, as I've enumerated.
You can't just assume that dirent
/stat
& co will have the same layout they have in other environments.
You can't reset ino_t
to be u64
because it got represented as u64
in dirent
- it's used in other places, e.g. FTSENT
where it must have its original value. While libc
doesn't bind this struct, other crates do, and they use libc
's ino_t
type assuming that it matches the C variant.
@@ -30,7 +32,7 @@ s! { | |||
pub st_dev: c_ulonglong, | |||
__pad0: [c_uchar; 4], | |||
__st_ino: crate::ino_t, | |||
pub st_mode: c_uint, | |||
pub st_mode: crate::mode_t, |
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.
This is in fact an unsigned int
and not a mode_t
.
Note that stat
and stat64
are actually the same on Android.
@@ -52,7 +54,7 @@ s! { | |||
pub st_dev: c_ulonglong, | |||
__pad0: [c_uchar; 4], | |||
__st_ino: crate::ino_t, | |||
pub st_mode: c_uint, | |||
pub st_mode: crate::mode_t, |
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.
This is supposed to be unsigned int
, not mode_t
.
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.
I made this mode_t to allow code using rust libc to just use the time mode_t to store the type like on other *nix platforms instead of having to deal with Android in other way.
@@ -528,16 +526,16 @@ s_no_extra_traits! { | |||
} | |||
|
|||
pub struct dirent { | |||
pub d_ino: u64, | |||
pub d_off: i64, | |||
pub d_ino: crate::ino_t, |
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.
This is not correct, ino_t
is only used when LP64
is enabled (int is 32-bit, long int is 64-bit, pointers are 64-bit).
In all other cases (mostly 32-bit CPUs), this is an explicit u64
, not an ino_t
.
We could split dirent
by architecture if we really wanted to use ino_t
here, but on LP64 platforms, ino_t
happens to be u64
which is why this was working.
pub d_ino: u64, | ||
pub d_off: i64, | ||
pub d_ino: crate::ino_t, | ||
pub d_off: crate::off_t, |
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.
This either needs to remain unmodified or become off64_t
- it is unconditionally 64-bits on android
pub d_reclen: c_ushort, | ||
pub d_type: c_uchar, | ||
pub d_name: [c_char; 256], | ||
} | ||
|
||
pub struct dirent64 { | ||
pub d_ino: u64, | ||
pub d_off: i64, | ||
pub d_ino: crate::ino64_t, |
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.
Same comment applies here - dirent64
and dirent
are the same struct on Android.
Oh yeah, I didn't look into other uses of ino_t in FTSENT. I don't think I am capable of taking this PR further. This is just way too complicated for me. Feel free to take over this |
@rustbot author There is no rush for anything here, so great if you get a chance to update this and no worries if not. It may also be possible to split this into smaller PRs, that is an option as well if you are able. |
- Fixes termux#24741 - Cherry-pick fish-shell/fish-shell@bbf678e to replace `libc-src-unix-linux_like-android-mod.rs.diff` (since `libandroid-spawn` does not currently work with `fish`) - `libc-src-unix-linux_like-android.diff` is **causing** termux#24741 because of the problems explained by **maurer** in rust-lang/libc#4216 (comment) - I have rewritten `libc-src-unix-linux_like-android.diff` in **my own alternative way of solving [the errors](termux#22609 (comment) - My personal belief is that these errors should be solved by **patching `fish`**, _not_ by patching the **`libc` cargo crate**. - The reason I believe that is because **`fish` is using the type `libc::ino_t` to represent a value for storing the `d_ino` member of a `dirent` struct, which **cannot work on 32-bit Android** because 32-bit Android's `d_ino` is **not** an `ino_t`. [It is a `uint64_t`](https://cs.android.com/android/platform/superproject/main/+/main:bionic/libc/include/dirent.h;l=64?q=d_ino&ss=android%2Fplatform%2Fsuperproject%2Fmain:bionic%2Flibc%2Finclude%2F&start=1). - Therefore, the logical conclusion is that any successful port of `fish` to 32-bit Android **must explicitly store the value of a `dirent` `d_ino` as a `u64` anywhere it appears in Rust code.** - The same concept applies to `st_mode` - `st_mode` of `stat` on 32-bit Android is **not** a `mode_t`. [It is an `unsigned int`](https://cs.android.com/android/platform/superproject/main/+/main:bionic/libc/include/sys/stat.h;l=87), and [on Android, `unsigned int` is a 32-bit integer](https://cs.android.com/android/platform/superproject/main/+/main:bionic/libc/include/stdint.h;l=42;drc=61197364367c9e404c7da6900658f1b16c42d0da;bpv=0;bpt=1?q=stdint.h&ss=android%2Fplatform%2Fsuperproject%2Fmain). Therefore, any successful port of `fish` to 32-bit Android must explicitly store the value of a `stat` `st_mode` as `u32`. - In C, `S_IFMT` is a pure typeless literal preprocessor definition, which is [`00170000` in Android](https://cs.android.com/android/kernel/superproject/+/common-android-mainline:common/include/uapi/linux/stat.h;l=9?q=S_IFMT), preprocessor definitions do not really exist in Rust so there is unfortunately no correct alternative to a hardcoded integer literal for this on 32-bit Android - I previously read rust-lang/libc#4216 in January 2025, however, I unfortunately did not have the experience necessary at the time to fully understand how to solve this specific problem, so at the time, I did not change the version written by thunder-coding because I did not know how to make a better way to bypass the errors - However, in April 2025, I [ported Steel Bank Common Lisp to 64-bit Android-x86](https://github.com/robertkirkman/termux-packages/blob/b54fa7f61e3c9acdfdc13b9680b9ff023d69323d/packages/sbcl/fix-stat-st_nlink-x86.patch). In the process of doing that, I became more experienced with Android's `stat.h` and `dirent.h` files, and I have been able to use that experience to successfully create what appears to be a correct solution to termux#24741 The original errors, repeated here for convenint reference by others: ``` error[E0308]: mismatched types --> src/wutil/dir_iter.rs:114:50 | 114 | self.typ.set(stat_mode_to_entry_type(s.st_mode)); | ----------------------- ^^^^^^^^^ expected `u16`, found `u32` | | | arguments to this function are incorrect | note: function defined here --> src/wutil/dir_iter.rs:151:4 | 151 | fn stat_mode_to_entry_type(m: libc::mode_t) -> Option<DirEntryType> { | ^^^^^^^^^^^^^^^^^^^^^^^ --------------- help: you can convert a `u32` to a `u16` and panic if the converted value doesn't fit | 114 | self.typ.set(stat_mode_to_entry_type(s.st_mode.try_into().unwrap())); | ++++++++++++++++++++ error[E0308]: mismatched types --> src/wutil/dir_iter.rs:292:32 | 292 | self.entry.inode = dent.d_ino; | ---------------- ^^^^^^^^^^ expected `u32`, found `u64` | | | expected due to the type of this binding ```
Unfortunately, it turned out that this PR did not successfully work at run-time in a real-world 32-bit Android situation: My personal belief is that in this case, the original errors (in the Fish project) should actually be considered the responsibility of the Fish side of the code, and that the correct location to apply patches is in the Fish code, so if, for example, the Fish project does not officially intend to support Android, then termux-packages should apply downstream patches to the Fish-specific code, not to the rust-lang/libc cargo crate. |
- Fixes termux#24741 - Cherry-pick fish-shell/fish-shell@bbf678e to replace `libc-src-unix-linux_like-android-mod.rs.diff` (since `libandroid-spawn` does not currently work with `fish`) - `libc-src-unix-linux_like-android.diff` is **causing** termux#24741 because of the problems explained by **maurer** in rust-lang/libc#4216 (comment) - I have rewritten `libc-src-unix-linux_like-android.diff` in **my own alternative way of solving [the errors](termux#22609 (comment) - My personal belief is that these errors should be solved by **patching `fish`**, _not_ by patching the **`libc` cargo crate**. - The reason I believe that is because **`fish` is using the type `libc::ino_t` to represent a value for storing the `d_ino` member of a `dirent` struct, which **cannot work on 32-bit Android** because 32-bit Android's `d_ino` is **not** an `ino_t`. [It is a `uint64_t`](https://cs.android.com/android/platform/superproject/main/+/main:bionic/libc/include/dirent.h;l=64?q=d_ino&ss=android%2Fplatform%2Fsuperproject%2Fmain:bionic%2Flibc%2Finclude%2F&start=1). - Therefore, the logical conclusion is that any successful port of `fish` to 32-bit Android **must explicitly store the value of a `dirent` `d_ino` as a `u64` anywhere it appears in Rust code.** - The same concept applies to `st_mode` - `st_mode` of `stat` on 32-bit Android is **not** a `mode_t`. [It is an `unsigned int`](https://cs.android.com/android/platform/superproject/main/+/main:bionic/libc/include/sys/stat.h;l=87), and [on Android, `unsigned int` is a 32-bit integer](https://cs.android.com/android/platform/superproject/main/+/main:bionic/libc/include/stdint.h;l=42;drc=61197364367c9e404c7da6900658f1b16c42d0da;bpv=0;bpt=1?q=stdint.h&ss=android%2Fplatform%2Fsuperproject%2Fmain). Therefore, any successful port of `fish` to 32-bit Android must explicitly store the value of a `stat` `st_mode` as `u32`. - In C, `S_IFMT` is a pure typeless literal preprocessor definition, which is [`00170000` in Android](https://cs.android.com/android/kernel/superproject/+/common-android-mainline:common/include/uapi/linux/stat.h;l=9?q=S_IFMT), preprocessor definitions do not really exist in Rust so there is unfortunately no correct alternative to a hardcoded integer literal for this on 32-bit Android - I previously read rust-lang/libc#4216 in January 2025, however, I unfortunately did not have the experience necessary at the time to fully understand how to solve this specific problem, so at the time, I did not change the version written by thunder-coding because I did not know how to make a better way to bypass the errors - However, in April 2025, I [ported Steel Bank Common Lisp to 64-bit Android-x86](https://github.com/robertkirkman/termux-packages/blob/b54fa7f61e3c9acdfdc13b9680b9ff023d69323d/packages/sbcl/fix-stat-st_nlink-x86.patch). In the process of doing that, I became more experienced with Android's `stat.h` and `dirent.h` files, and I have been able to use that experience to successfully create what appears to be a correct solution to termux#24741 The original errors, repeated here for convenint reference by others: ``` error[E0308]: mismatched types --> src/wutil/dir_iter.rs:114:50 | 114 | self.typ.set(stat_mode_to_entry_type(s.st_mode)); | ----------------------- ^^^^^^^^^ expected `u16`, found `u32` | | | arguments to this function are incorrect | note: function defined here --> src/wutil/dir_iter.rs:151:4 | 151 | fn stat_mode_to_entry_type(m: libc::mode_t) -> Option<DirEntryType> { | ^^^^^^^^^^^^^^^^^^^^^^^ --------------- help: you can convert a `u32` to a `u16` and panic if the converted value doesn't fit | 114 | self.typ.set(stat_mode_to_entry_type(s.st_mode.try_into().unwrap())); | ++++++++++++++++++++ error[E0308]: mismatched types --> src/wutil/dir_iter.rs:292:32 | 292 | self.entry.inode = dent.d_ino; | ---------------- ^^^^^^^^^^ expected `u32`, found `u64` | | | expected due to the type of this binding ```
- Fixes #24741 - Cherry-pick fish-shell/fish-shell@bbf678e to replace `libc-src-unix-linux_like-android-mod.rs.diff` (since `libandroid-spawn` does not currently work with `fish`) - `libc-src-unix-linux_like-android.diff` is **causing** #24741 because of the problems explained by **maurer** in rust-lang/libc#4216 (comment) - I have rewritten `libc-src-unix-linux_like-android.diff` in **my own alternative way of solving [the errors](#22609 (comment) - My personal belief is that these errors should be solved by **patching `fish`**, _not_ by patching the **`libc` cargo crate**. - The reason I believe that is because **`fish` is using the type `libc::ino_t` to represent a value for storing the `d_ino` member of a `dirent` struct, which **cannot work on 32-bit Android** because 32-bit Android's `d_ino` is **not** an `ino_t`. [It is a `uint64_t`](https://cs.android.com/android/platform/superproject/main/+/main:bionic/libc/include/dirent.h;l=64?q=d_ino&ss=android%2Fplatform%2Fsuperproject%2Fmain:bionic%2Flibc%2Finclude%2F&start=1). - Therefore, the logical conclusion is that any successful port of `fish` to 32-bit Android **must explicitly store the value of a `dirent` `d_ino` as a `u64` anywhere it appears in Rust code.** - The same concept applies to `st_mode` - `st_mode` of `stat` on 32-bit Android is **not** a `mode_t`. [It is an `unsigned int`](https://cs.android.com/android/platform/superproject/main/+/main:bionic/libc/include/sys/stat.h;l=87), and [on Android, `unsigned int` is a 32-bit integer](https://cs.android.com/android/platform/superproject/main/+/main:bionic/libc/include/stdint.h;l=42;drc=61197364367c9e404c7da6900658f1b16c42d0da;bpv=0;bpt=1?q=stdint.h&ss=android%2Fplatform%2Fsuperproject%2Fmain). Therefore, any successful port of `fish` to 32-bit Android must explicitly store the value of a `stat` `st_mode` as `u32`. - In C, `S_IFMT` is a pure typeless literal preprocessor definition, which is [`00170000` in Android](https://cs.android.com/android/kernel/superproject/+/common-android-mainline:common/include/uapi/linux/stat.h;l=9?q=S_IFMT), preprocessor definitions do not really exist in Rust so there is unfortunately no correct alternative to a hardcoded integer literal for this on 32-bit Android - I previously read rust-lang/libc#4216 in January 2025, however, I unfortunately did not have the experience necessary at the time to fully understand how to solve this specific problem, so at the time, I did not change the version written by thunder-coding because I did not know how to make a better way to bypass the errors - However, in April 2025, I [ported Steel Bank Common Lisp to 64-bit Android-x86](https://github.com/robertkirkman/termux-packages/blob/b54fa7f61e3c9acdfdc13b9680b9ff023d69323d/packages/sbcl/fix-stat-st_nlink-x86.patch). In the process of doing that, I became more experienced with Android's `stat.h` and `dirent.h` files, and I have been able to use that experience to successfully create what appears to be a correct solution to #24741 The original errors, repeated here for convenint reference by others: ``` error[E0308]: mismatched types --> src/wutil/dir_iter.rs:114:50 | 114 | self.typ.set(stat_mode_to_entry_type(s.st_mode)); | ----------------------- ^^^^^^^^^ expected `u16`, found `u32` | | | arguments to this function are incorrect | note: function defined here --> src/wutil/dir_iter.rs:151:4 | 151 | fn stat_mode_to_entry_type(m: libc::mode_t) -> Option<DirEntryType> { | ^^^^^^^^^^^^^^^^^^^^^^^ --------------- help: you can convert a `u32` to a `u16` and panic if the converted value doesn't fit | 114 | self.typ.set(stat_mode_to_entry_type(s.st_mode.try_into().unwrap())); | ++++++++++++++++++++ error[E0308]: mismatched types --> src/wutil/dir_iter.rs:292:32 | 292 | self.entry.inode = dent.d_ino; | ---------------- ^^^^^^^^^^ expected `u32`, found `u64` | | | expected due to the type of this binding ```
- Fixes termux/termux-packages#24741 - Cherry-pick fish-shell/fish-shell@bbf678e to replace `libc-src-unix-linux_like-android-mod.rs.diff` (since `libandroid-spawn` does not currently work with `fish`) - `libc-src-unix-linux_like-android.diff` is **causing** termux/termux-packages#24741 because of the problems explained by **maurer** in rust-lang/libc#4216 (comment) - I have rewritten `libc-src-unix-linux_like-android.diff` in **my own alternative way of solving [the errors](termux/termux-packages#22609 (comment) - My personal belief is that these errors should be solved by **patching `fish`**, _not_ by patching the **`libc` cargo crate**. - The reason I believe that is because **`fish` is using the type `libc::ino_t` to represent a value for storing the `d_ino` member of a `dirent` struct, which **cannot work on 32-bit Android** because 32-bit Android's `d_ino` is **not** an `ino_t`. [It is a `uint64_t`](https://cs.android.com/android/platform/superproject/main/+/main:bionic/libc/include/dirent.h;l=64?q=d_ino&ss=android%2Fplatform%2Fsuperproject%2Fmain:bionic%2Flibc%2Finclude%2F&start=1). - Therefore, the logical conclusion is that any successful port of `fish` to 32-bit Android **must explicitly store the value of a `dirent` `d_ino` as a `u64` anywhere it appears in Rust code.** - The same concept applies to `st_mode` - `st_mode` of `stat` on 32-bit Android is **not** a `mode_t`. [It is an `unsigned int`](https://cs.android.com/android/platform/superproject/main/+/main:bionic/libc/include/sys/stat.h;l=87), and [on Android, `unsigned int` is a 32-bit integer](https://cs.android.com/android/platform/superproject/main/+/main:bionic/libc/include/stdint.h;l=42;drc=61197364367c9e404c7da6900658f1b16c42d0da;bpv=0;bpt=1?q=stdint.h&ss=android%2Fplatform%2Fsuperproject%2Fmain). Therefore, any successful port of `fish` to 32-bit Android must explicitly store the value of a `stat` `st_mode` as `u32`. - In C, `S_IFMT` is a pure typeless literal preprocessor definition, which is [`00170000` in Android](https://cs.android.com/android/kernel/superproject/+/common-android-mainline:common/include/uapi/linux/stat.h;l=9?q=S_IFMT), preprocessor definitions do not really exist in Rust so there is unfortunately no correct alternative to a hardcoded integer literal for this on 32-bit Android - I previously read rust-lang/libc#4216 in January 2025, however, I unfortunately did not have the experience necessary at the time to fully understand how to solve this specific problem, so at the time, I did not change the version written by thunder-coding because I did not know how to make a better way to bypass the errors - However, in April 2025, I [ported Steel Bank Common Lisp to 64-bit Android-x86](https://github.com/robertkirkman/termux-packages/blob/b54fa7f61e3c9acdfdc13b9680b9ff023d69323d/packages/sbcl/fix-stat-st_nlink-x86.patch). In the process of doing that, I became more experienced with Android's `stat.h` and `dirent.h` files, and I have been able to use that experience to successfully create what appears to be a correct solution to termux/termux-packages#24741 The original errors, repeated here for convenint reference by others: ``` error[E0308]: mismatched types --> src/wutil/dir_iter.rs:114:50 | 114 | self.typ.set(stat_mode_to_entry_type(s.st_mode)); | ----------------------- ^^^^^^^^^ expected `u16`, found `u32` | | | arguments to this function are incorrect | note: function defined here --> src/wutil/dir_iter.rs:151:4 | 151 | fn stat_mode_to_entry_type(m: libc::mode_t) -> Option<DirEntryType> { | ^^^^^^^^^^^^^^^^^^^^^^^ --------------- help: you can convert a `u32` to a `u16` and panic if the converted value doesn't fit | 114 | self.typ.set(stat_mode_to_entry_type(s.st_mode.try_into().unwrap())); | ++++++++++++++++++++ error[E0308]: mismatched types --> src/wutil/dir_iter.rs:292:32 | 292 | self.entry.inode = dent.d_ino; | ---------------- ^^^^^^^^^^ expected `u32`, found `u64` | | | expected due to the type of this binding ```
This bug was noticed in termux/termux-packages#22609
Description
Fixes types for
dirent.d_ino
,dirent.d_off
,stat.st_mode
andstat64.st_mode
on Android.The hardcoded types are incorrect and this PR changes them to how they are declared for Linux. Also fixes the incorrect type errors in termux/termux-packages#22609, where I found the bug (The builds are still failing due to missing
posix_spawn*
support here, but that's a different issue.)This patch should likely also be backported to 0.2.x release, I have the patchset already applied on my other branch. Will link another PR soon.It seems that this patch applies cleanly to libc-0.2 branch, I was backporting to an older release earlier.
@rustbot label stable-nominated
should be it according to CONTRIBUTING.mdSources
mode_t
:mode_t
is defined asunsigned int
on 64-bit android platforms:https://android.googlesource.com/platform/prebuilts/ndk/+/dev/platform/sysroot/usr/include/sys/types.h#64
https://android.googlesource.com/platform/prebuilts/ndk/+/dev/platform/sysroot/usr/include/asm-generic/posix_types.h#18
and is defined as
unsigned short
on 32-bit platforms:https://android.googlesource.com/platform/prebuilts/ndk/+/dev/platform/sysroot/usr/include/i686-linux-android/asm/posix_types_32.h#9
https://android.googlesource.com/platform/prebuilts/ndk/+/dev/platform/sysroot/usr/include/arm-linux-androideabi/asm/posix_types.h#9
off_t
:https://android.googlesource.com/platform/bionic/+/main/docs/32-bit-abi.md mentions that
off_t
isi32
on 32-bit andi64
on 64-bitino_t
:https://android.googlesource.com/platform/prebuilts/ndk/+/dev/platform/sysroot/usr/include/dirent.h#64
https://android.googlesource.com/platform/prebuilts/ndk/+/dev/platform/sysroot/usr/include/sys/types.h#70
https://android.googlesource.com/platform/prebuilts/ndk/+/dev/platform/sysroot/usr/include/asm-generic/posix_types.h#14
Due to "historic accident", although
ino_t
isunsigned long
on all platforms, the actual type used isunsigned long
on 64-bit anduint64_t
on 32-bit. I think we should just declareino_t
to be of typeunsigned long
on 64-bit anduint64_t
on 32-bit as that's how code using these types is supposed to be used.Checklist
libc-test/semver
have been updated*LAST
or*MAX
areincluded (see #3131)
cd libc-test && cargo test --target mytarget
);especially relevant for platforms that may not be checked in CI
I tried building the tests locally, it seems like the tests are already broken for Android. But I was able to fix some of the issues in bump(main/fish): 4.0.0 termux/termux-packages#22609