Skip to content

ioctl request arg size for Android aarch64 is wrong? #1036

Open
@lilydjwg

Description

@lilydjwg

I was trying to call libc::ioctl but I got type errors with Android aarch64 target. It works fine with x86_64. The second argument request has different size in these targets: c_ulong for x86_64 and c_int for Android aarch64.

I believe it should be c_ulong for Android aarch64 too, because it works (I'm using FFI now, and I get correct data out), and my request 0xC020660B (FS_IOC_FIEMAP) doesn't fit into a c_int.

Activity

alexcrichton

alexcrichton commented on Jul 9, 2018

@alexcrichton
Member

We are verifying the definition is correct on Android aarch64, so I think this may be an oddity of C!

drrlvn

drrlvn commented on Sep 6, 2018

@drrlvn

I'm having this same issue but with x86_64-unknown-linux-musl where request is i32 as opposed to u64. What do you mean an oddity of C? Is this not a bug in libc?

lilydjwg

lilydjwg commented on Sep 6, 2018

@lilydjwg
Author

I've digged into this a little bit deeper. Technically unless the request is larger than 32 bits, this type difference in C results in the same kernel behaviour (the assembly I got was different but the data passed to kernel was the same).

Android has different header files than glibc's one. Maybe it's the same situation for musl.

upsuper

upsuper commented on Dec 13, 2019

@upsuper

@alexcrichton If you look at definition of ioctl in Android, for example this one, you would see

__BEGIN_DECLS

/**
 * [ioctl(2)](http://man7.org/linux/man-pages/man2/ioctl.2.html) operates on device files.
 */
int ioctl(int __fd, int __request, ...);

/*
 * Work around unsigned -> signed conversion warnings: many common ioctl
 * constants are unsigned.
 *
 * Since this workaround introduces an overload to ioctl, it's possible that it
 * will break existing code that takes the address of ioctl. If such a breakage
 * occurs, you can work around it by either:
 * - specifying a concrete, correct type for ioctl (whether it be through a cast
 *   in `(int (*)(int, int, ...))ioctl`, creating a temporary variable with the
 *   type of the ioctl you prefer, ...), or
 * - defining BIONIC_IOCTL_NO_SIGNEDNESS_OVERLOAD, which will make the
 *   overloading go away.
 */
#if !defined(BIONIC_IOCTL_NO_SIGNEDNESS_OVERLOAD)
/* enable_if(1) just exists to break overloading ties. */
int ioctl(int __fd, unsigned __request, ...) __overloadable __enable_if(1, "") __RENAME(ioctl);
#endif

__END_DECLS

You can see that it explicitly calls out the situation that many common ioctl constants are unsigned, and can't fit into int.

The __overloadable here is a macro defined to be __attribute__((overloadable)), which is an attribute provided by clang and gcc to support overloading in C.

Given the situation that many constants are actually unsigned, and Android header specifically tries to handle that, I think this issue probably shouldn't be closed. I'm not sure how can we solve this, as Rust doesn't support overloading, but this issue is valid and should be discussed.

reopened this on Dec 13, 2019
gnzlbg

gnzlbg commented on Dec 13, 2019

@gnzlbg
Contributor

@upsuper I'm not sure if I understanding the issue properly, but I have one question. Those two "overloads", do they have the same mangled name? If not, then I think we should just be providing two different APIs, with two different type signatures, each pointing at a different overload.

upsuper

upsuper commented on Dec 13, 2019

@upsuper

IIUC the two overloads share the same symbol. It is explicitly done via __RENAME(ioctl) (which expands to __asm__("ioctl")) meaning its name is also ioctl.

gnzlbg

gnzlbg commented on Dec 13, 2019

@gnzlbg
Contributor

I see. The only reasonable way I can think of, of exposing this safely from rust, is to provide two libc APIs, both pointing to the same symbol, but with different type signatures.

upsuper

upsuper commented on Dec 13, 2019

@upsuper

That sounds painful for downstream users, as they may need to use different functions for different platforms. Or are you proposing adding new APIs with consistent signature across platforms?

CmdrMoozy

CmdrMoozy commented on May 29, 2021

@CmdrMoozy

Just adding a +1 since it seems this hasn't got any attention lately. It also affects aarch64-unknown-linux-musl, not just the Android target.

To add some more context:

This is a weird mismatch, but it turns out to "just work" because the conversion is well defined by the standard and does the (I suppose) intuitive thing (https://stackoverflow.com/questions/46519568/conversion-of-function-parameter-from-signed-int-to-unsigned-int).

Note that maybe surprisingly, sizeof(int) (and likewise sizeof(unsigned int)) is 32 bits on aarch64.

So I think the libc crate is actually doing the right thing. Note that in C when you pass a long int into a function which takes an int, it just implicitly truncates the high bits.

So, I think that no ioctl parameters should actually use > 32 bits, and if they did, they would just get implicitly truncated anyway?

If everything I've said is right, then passing in <value> as libc::c_int should work correctly on all platforms.

Ah, but on x86_64-unknown-linux-gnu the libc crate says it takes an unsigned long. This might match what glibc or something does, but it doesn't match how the kernel actually defines the syscall, so that seems weird. So, maybe it's hopeless and we need to conditionally compile to select the type.

added a commit that references this issue on Jun 13, 2021
added this to the 1.0 milestone on Aug 29, 2024
added
E-mediumE-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate.
and removed on Aug 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    E-mediumE-medium Call for participation: Medium difficulty. Experience needed to fix: Intermediate.O-androidO-arm

    Type

    No type

    Projects

    No projects

    Relationships

    None yet

      Development

      No branches or pull requests

        Participants

        @drrlvn@alexcrichton@upsuper@lilydjwg@gnzlbg

        Issue actions

          ioctl request arg size for Android aarch64 is wrong? · Issue #1036 · rust-lang/libc