Intercepting arguments with va_args will not pass in arguments correctly

[cjappl]

rtcheck/src/lib_rt_check.cpp

Line 539 in 706ea6d

auto result = REAL(open)(path, oflag, args);

Hi @drowaudio!

Was just poking around your repo, and noticed a bug that I just fixed in RADSan also here, wanted to file an issue so you don't get weird reports later on.

The fix can be seen in this PR:
https://github.com/realtime-sanitizer/llvm-project/pull/23/files

You must unpack the va_arg first, then pass it in to the REAL call. The three in our code that had this problem were:

1. open
2. openat
3. fcntl

but be wary of any of these syscalls that accept variable args.

Cheers!

[drowaudio]

Thanks for the heads up @cjappl , I'll get that fixed.
Quick question though, what if open etc. are called without that extra parameter? Aren't the ellipsis arguments optional and depend on the preceding arguments as to what they actually contain?

Or have I misunderstood the docs in regard to these functions?

Cheers!

[cjappl]

(before I begin, we had a few more updates to both open, openat and fcntl, make sure you check latest realtime-sanitizer/radsan branch and get the latest from there, as it's the most up to date)

Nope, you're totally correct, they do depend on the arguments passed in. However, the code change that we did follows the interceptor in tsan, where regardless of the arguments they unpack the last argument:

TSAN_INTERCEPTOR(int, open, const char *name, int oflag, ...) {
  va_list ap;
  va_start(ap, oflag);
  mode_t mode = va_arg(ap, int);
  va_end(ap);
  SCOPED_TSAN_INTERCEPTOR(open, name, oflag, mode);
  READ_STRING(thr, pc, name, 0);
  int fd = REAL(open)(name, oflag, mode);
  if (fd >= 0)
    FdFileCreate(thr, pc, fd);
  return fd;
}

In open, this last argument only exists if the O_CREAT flag is passed in. Apparently TSAN is confident that the last argument is ignored, unless in the O_CREAT case, where we have unpacked the right argument (which seems reasonable enough)

However, I was looking for this in the docs of va_arg (https://en.cppreference.com/w/c/variadic/va_arg) and it says:

If va_arg is called when there are no more arguments in ap, the behavior is undefined.

So it seems like TSAN is depending on a little undefined behavior here, which makes me uneasy. They must be very confident that any """deadly UB""" must not be triggered if the argument isn't ever touched.

This is easy enough to code around in the open case, imagine (psuedocode):

INTERCEPTOR(open ... )
{
    if `O_CREAT` {
        va_list ap;
        va_start(ap, oflag);
       mode_t mode = va_arg(ap, int);
       va_end(ap);
       int fd = REAL(open)(name, oflag, mode);
    }
    else
         int fd = REAL(open)(name, oflag);

...

}

However, fcntl is hell if you do this. Not only are there >20 different commands, which take at least 3 different types as their final argument, but these commands and types are compiler and OS dependent. This results in a giant switch statement with some cases conditionally omitted either at compile or runtime...

The workaround we used for this was similar to open, but used the same approach that linux does. Throw the argument into something "big enough to accommodate anything" and assume that fcntl deals with it properly with a cast:

INTERCEPTOR(int, fcntl, int filedes, int cmd, ...) {
  radsan::expectNotRealtime("fcntl");

  va_list args;
  va_start(args, cmd);

  // Following precedent here. The linux source (fcntl.c, do_fcntl) accepts the
  // final argument in a variable that will hold the largest of the possible
  // argument types (pointers and ints are typical in fcntl) It is then assumed
  // that the implementation of fcntl will cast it properly depending on cmd.
  //
  // This is also similar to what is done in
  // sanitizer_common/sanitizer_common_syscalls.inc
  const unsigned long arg = va_arg(args, unsigned long);
  int result = REAL(fcntl)(filedes, cmd, arg);

  va_end(args);

  return result;
}

Again, not the code that helps you sleep at night the best. This code does pass various unit tests that use different arg types, and no arg types, and resolved 3 of our most longstanding bugs (which you have in rtcheck right now, by nature of the code being the same as our old version), but I'm not in love with the solution.

If you have any better suggestions we are all ears. This development all just happened in the last two days, so this code is fresh.

Let me know if you need more context or help, happy to!

Related bugs that were fixed in our latest versions of our interceptors:
realtime-sanitizer/rtsan#31
realtime-sanitizer/rtsan#29
realtime-sanitizer/llvm-project#17 (see the foo around the flakey tests)

[drowaudio]

Ok, thanks. I've copied what you're doing for now. It seems sensible.
Cheers!