[compiler-rt] Fix the prototype of ioctl interceptor

[kubamracek]

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.

[majnemer]

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.

[glider]

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

[eugenis]

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.

[kubamracek]

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.

[eugenis]

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.

[kubamracek]

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.

[kubamracek]

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

[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?

[kubamracek]

Landed in r226926.

lib/sanitizer_common/sanitizer_common_interceptors.inc

   COMMON_INTERCEPT_FUNCTION(__isoc99_fprintf);   \
   COMMON_INTERCEPT_FUNCTION(__isoc99_vprintf);   \
   COMMON_INTERCEPT_FUNCTION(__isoc99_vsprintf);  \
   COMMON_INTERCEPT_FUNCTION(__isoc99_vsnprintf); \
   COMMON_INTERCEPT_FUNCTION(__isoc99_vfprintf);
 #else
 #define INIT_ISOC99_PRINTF
 #endif
 
 #if SANITIZER_INTERCEPT_IOCTL
 #include "sanitizer_common_interceptors_ioctl.inc"
-INTERCEPTOR(int, ioctl, int d, unsigned request, void *arg) {
+INTERCEPTOR(int, ioctl, int d, unsigned long request, ...) {
   void *ctx;
+  va_list ap;
+  va_start(ap, request);
+  void *arg = va_arg(ap, void *);
+  va_end(ap);
   COMMON_INTERCEPTOR_ENTER(ctx, ioctl, d, request, arg);
 
   CHECK(ioctl_initialized);
 
   // Note: TSan does not use common flags, and they are zero-initialized.
   // This effectively disables ioctl handling in TSan.
   if (!common_flags()->handle_ioctl) return REAL(ioctl)(d, request, arg);
 
+  // Although request is unsigned long, the rest of the interceptor uses it
+  // as just "unsigned" to save space, because we know that all values fit in
+  // "unsigned" - they are compile-time constants.
+
   const ioctl_desc *desc = ioctl_lookup(request);
   ioctl_desc decoded_desc;
   if (!desc) {
     VPrintf(2, "Decoding unknown ioctl 0x%x\n", request);
     if (!ioctl_decode(request, &decoded_desc))
       Printf("WARNING: failed decoding unknown ioctl 0x%x\n", request);
     else
       desc = &decoded_desc;