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

ioctl write_int doesn't support passing long integers #824

Closed
jethrogb opened this issue Jan 3, 2018 · 9 comments
Closed

ioctl write_int doesn't support passing long integers #824

jethrogb opened this issue Jan 3, 2018 · 9 comments
Labels

Comments

@jethrogb
Copy link

jethrogb commented Jan 3, 2018

The Linux kernel API always passes the ioctl parameter as an unsigned long. However, the ioctl! macro always uses c_int. It should probably use c_long/c_ulong/usize instead, in order to pass all possible values.

@Susurrus
Copy link
Contributor

Susurrus commented Jan 8, 2018

Where is this specified for Linux? And if this is correct, then why wouldn't c_ulong be the most appropriate choice out of those 3 options? Only one should be used, but you didn't clarify which is the best option in your statement.

@Susurrus Susurrus added the A-bug label Jan 20, 2018
@Susurrus
Copy link
Contributor

@jethrogb I don't understand enough to fix this, so I need some more information from you. Specifically a test case that you know is failing currently.

Linux should actually be using a c_ulong type for this according to this code in src/sys/ioctl/platform/linux.rs:

#[cfg(not(any(target_os = "android", target_env = "musl")))]
#[doc(hidden)]
pub type ioctl_num_type = ::libc::c_ulong;

@Susurrus
Copy link
Contributor

Sorry, actually looking at the code I do see the size of a c_int is used in generating the ioctl request code, so maybe that's the error you're indicating? I'm going to wait for more details from you before digging into this further.

@jethrogb
Copy link
Author

ioctl_num_type is the type used for the ioctl number, not the parameter. It might be ok to reuse it for the parameter? Not sure if it's consistent across platforms.

The problem is with this line https://github.com/Susurrus/nix/blob/7eb8dd1ccf5ec8281076e9aa8f1598e0e8bcbd68/src/sys/ioctl/mod.rs#L544

@Susurrus
Copy link
Contributor

I still don't see any documentation suggesting what youre saying here. The passed integer must be equal in size to a pointer type because of the way ioctl is defined. We'll need to sort out what's appropriate for each platform, and we'll need docs for that. Can you link to any?

@jethrogb
Copy link
Author

jethrogb commented Jan 21, 2018

https://github.com/torvalds/linux/blob/5d38f04/include/linux/fs.h#L1702-L1703

That, by the way, shows that the current ioctl_num_type of c_ulong for non-Android non-musl Linux is incorrect.

@Susurrus
Copy link
Contributor

I couldn't quickly find the declaration of ioctl on Android to find the ioctl_num_type, but I'd assume it's c_int. And musl shouldn't matter since this is a Linux kernel API which is independent of the libc being used. So I agree with you there. I wonder why it was ever like this to begin with?

I'll fix this and add also specify the parameter as c_ulong as well in #833.

On BSD it looks like it's consistently a c_ulong, so at least that should be correct.

@Susurrus
Copy link
Contributor

And the fix to this might not be too easy because:

~/libc $ rg fn\ ioctl
src/unix/notbsd/linux/s390x.rs
1249:    pub fn ioctl(fd: ::c_int, request: ::c_ulong, ...) -> ::c_int;

src/unix/notbsd/linux/mips/mod.rs
679:    pub fn ioctl(fd: ::c_int, request: ::c_ulong, ...) -> ::c_int;

src/unix/notbsd/linux/musl/mod.rs
303:    pub fn ioctl(fd: ::c_int, request: ::c_int, ...) -> ::c_int;

src/unix/notbsd/linux/other/mod.rs
569:    pub fn ioctl(fd: ::c_int, request: ::c_ulong, ...) -> ::c_int;

So are those 3 c_ulong ioctl declarations wrong then?

Susurrus pushed a commit to Susurrus/nix that referenced this issue Apr 10, 2018
While usually `ioctl()` passes a pointer, the function call has been
overloaded to allow integers to be passed. For some platforms this
is an `int` and on others it's a `ulong`.

Fixes nix-rust#824.
@Susurrus
Copy link
Contributor

Fixed by 5dad660, which is now in master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants