This is an archive of the discontinued LLVM Phabricator instance.

[compiler-rt] Fix the prototype of ioctl interceptor
ClosedPublic

Authored by kubamracek on Jan 17 2015, 7:58 PM.

Details

Reviewers
eugenis
Summary

The interceptor of ioctl is using a non-standard prototype:

INTERCEPTOR(int, ioctl, int d, unsigned request, void *arg)

At least on OS X, the request argument should be unsigned long and not just unsigned, and also instead of the last argument (arg), the function should be accepting a variable number of arguments, so the prototype should be (per https://developer.apple.com/library/mac/documentation/Darwin/Reference/ManPages/man2/ioctl.2.html):

int ioctl(int fildes, unsigned long request, ...);

It looks to me that this prototype is valid for Linux as well (http://man7.org/linux/man-pages/man2/ioctl.2.html), but I've also found other documents that have request as int and not long (http://linux.die.net/man/2/ioctl). I'm not sure if we need an platform-specific #if.

I have actually seen a memory corruption and a crash because of that, but I'm unable to create a reproducible test case for it.

Diff Detail

Event Timeline

kubamracek updated this revision to Diff 18352.Jan 17 2015, 7:58 PM
kubamracek retitled this revision from to [compiler-rt] Fix the prototype of ioctl interceptor.
kubamracek updated this object.
kubamracek edited the test plan for this revision. (Show Details)
kubamracek added subscribers: Unknown Object (MLST), samsonov, glider, kcc.

The short version: we should just assume that the prototype is int ioctl(int fildes, unsigned long request, ...).

The longer version:

Seventh Edition Unix (circa 1979) used int, many UNIX derived operating systems behave like this.
2.9 BSD (circa 1984) used int, unchanged from V7.
2.10 BSD (circa 1986) changed this to unsigned long, many BSD derived operating systems behave like this.

SUSv3 specifies that the request parameter have type int but not all UNIX-like operating systems conform.

HP-UX, Solaris, IRIX, ULTRIX and AIX all use int.
Linux, FreeBSD, NetBSD, OpenBSD and Darwin all use unsigned long.

man 2 ioctl contains the following prototype:

int ioctl(int d, int request, ...);

Perhaps this is the source of the mistake, because <sys/ioctl.h> has the correct one:

extern int ioctl (int __fd, unsigned long int __request, ...) __THROW;

I think you also need to fix sanitizer_common_interceptors_ioctl.inc

glider added a subscriber: eugenis.Jan 20 2015, 2:55 AM
eugenis added inline comments.Jan 20 2015, 3:20 AM
lib/sanitizer_common/sanitizer_common_interceptors.inc
1028

I agree with "unsigned long request", but is there any benefit in the _unconditional_ va_arg stuff? We don't reliably know if there is an argument to a given ioctl or not, and then we pass this (possible garbage) value to REAL(ioctl) in any case.

Updating sanitizer_common_interceptors_ioctl.inc with unsigned -> unsigned long as well.

I agree with "unsigned long request", but is there any benefit in the _unconditional_ va_arg stuff? We don't reliably know if there is an argument to a given ioctl or not, and then we pass this (possible garbage) value to REAL(ioctl) in any case.

I don't think we have a way to tell whether that argument is used or not. The docs (e.g. http://man7.org/linux/man-pages/man2/ioctl.2.html) also suggest that it's always used:

The third argument is an untyped pointer to memory. It's traditionally char
*argp (from the days before void * was valid C), and will be so named
for this discussion.

In D7038#111635, @kubabrecka wrote:

Updating sanitizer_common_interceptors_ioctl.inc with unsigned -> unsigned long as well.

I agree with "unsigned long request", but is there any benefit in the _unconditional_ va_arg stuff? We don't reliably know if there is an argument to a given ioctl or not, and then we pass this (possible garbage) value to REAL(ioctl) in any case.

I don't think we have a way to tell whether that argument is used or not. The docs (e.g. http://man7.org/linux/man-pages/man2/ioctl.2.html) also suggest that it's always used:

Exactly. Then why go through va_arg()?

lib/sanitizer_common/sanitizer_common_interceptors_ioctl.inc
16 ↗(On Diff #18543)

This costs 4 bytes multiplied by the number of known ioctls. We know that all values of req fit in "unsigned" - they are compile-time constants.

I agree with "unsigned long request", but is there any benefit in the _unconditional_ va_arg stuff? We don't reliably know if there is an argument to a given ioctl or not, and then we pass this (possible garbage) value to REAL(ioctl) in any case.

I don't think we have a way to tell whether that argument is used or not. The docs (e.g. http://man7.org/linux/man-pages/man2/ioctl.2.html) also suggest that it's always used:

Exactly. Then why go through va_arg()?

Are you suggesting not to use the variadic prototype of the function, i.e.

int ioctl(int fildes, unsigned long request, void *arg);

? Because that's exactly what I think is the cause of the memory corruptions that I saw. Even when we *know* there's exactly one var-argument every time, the following function headers are not equal on all platforms:

int ioctl(int fildes, unsigned long request, void *arg);
int ioctl(int fildes, unsigned long request, ...);

This costs 4 bytes multiplied by the number of known ioctls. We know that all values of req fit in "unsigned" - they are compile-time constants.

Right, I guess we can just fix the interceptor prototype, but store the request values as unsigned. I'll update the patch again.

Updating patch to fix the prototype, but keep using unsigned internally to save space.

eugenis accepted this revision.Jan 23 2015, 12:56 AM
eugenis added a reviewer: eugenis.
In D7038#112364, @kubabrecka wrote:

Are you suggesting not to use the variadic prototype of the function, i.e.

int ioctl(int fildes, unsigned long request, void *arg);

? Because that's exactly what I think is the cause of the memory corruptions that I saw. Even when we *know* there's exactly one var-argument every time, the following function headers are not equal on all platforms:

int ioctl(int fildes, unsigned long request, void *arg);
int ioctl(int fildes, unsigned long request, ...);

Ok, I guess.
Btw, what platforms do you have in mind where the old code (w/o va_arg, but with "unsigned long request") would not work correctly?

This revision is now accepted and ready to land.Jan 23 2015, 12:56 AM
kubamracek closed this revision.Jan 23 2015, 11:19 AM

Landed in r226926.