[tsan] Fix the open and open64 interceptors to have correct declarations (variadic functions)

[kubamracek]

Summary

Not matching the (real) variadic declaration makes the interceptor take garbage inputs on AArch64.

[MaskRay]

Not matching the (real) variadic declaration makes the interceptor take garbage inputs on AArch64.

Is this Darwin specific or does it apply to aarch64-linux as well?

compiler-rt/test/tsan/Darwin/variadic-open.cpp

1 ↗

(On Diff #280630)

Is this Darwin specific or does it apply to aarch64-linux as well?

[vitalybuka]

Could you please move it one dir up with suggested changes?

Or maybe just drop "CHECK: permissions ="

compiler-rt/test/tsan/Darwin/variadic-open.cpp

12 ↗

(On Diff #280630)

somehow on linux even if file has 644 and argument is 644
fstat below returns 640

23 ↗

(On Diff #280630)

[kubamracek]

compiler-rt/test/tsan/Darwin/variadic-open.cpp

1 ↗

(On Diff #280630)

I believe this is Darwin specific. See https://developer.apple.com/library/archive/documentation/Xcode/Conceptual/iPhoneOSABIReference/Articles/ARM64FunctionCallingConventions.html which explains that all variadic arguments are always passed on the stack.

[kubamracek]

compiler-rt/test/tsan/Darwin/variadic-open.cpp

12 ↗

(On Diff #280630)

Is that just a umask effect? Would a call to umask(0) help here?

[kubamracek]

This fails on Linux/PPC64: http://lab.llvm.org:8011/builders/clang-ppc64be-linux/builds/52026/steps/ninja%20check%201/logs/FAIL%3A%20ThreadSanitizer-powerpc64%3A%3A%20variadic-open.cpp

I'm going to move the test back to the Darwin/ folder, as I don't have a good way to debug this.

@vitalybuka If you have suggestions how to make this reliably work on Linux, I'll be happy to add/move the test again.

[kubamracek]

Actually given the symptom, I'm thinking this could be a real problem with the variadic interceptor:

1: Hello world.
2: permissions = 07777

Would anyone with access to a Linux/PPC64 machine be willing to take a look if the open() syscall is receiving the right value (0600 from the testcase)?

[kubamracek]

CC @simoatze, @wschmidt:

Would anyone with access to a Linux/PPC64 machine be willing to take a look if the open() syscall is receiving the right value (0600 from the testcase)?

[nemanjai]

In D84570#2185572, @kubamracek wrote:

CC @simoatze, @wschmidt:

Would anyone with access to a Linux/PPC64 machine be willing to take a look if the open() syscall is receiving the right value (0600 from the testcase)?

I can provide you access to a PPC64LE machine or I can check the details myself if you can tell me exactly what to do and how.

[kubamracek]

I can provide you access to a PPC64LE machine or I can check the details myself if you can tell me exactly what to do and how.

What I would like to look into is: Build and run the testcase from this patch (variadic-open.cpp) *without* TSan, check that the resulting file permissions are 0600. Then build llvm+compiler-rt on PPC64, and build and run the testcase with the just-built clang *with* TSan on, and that should result with the file permissions reported as 07777? Then try to revert the code change from this patch (from tsan_interceptors_posix.cpp) and re-build and re-run the testcase *with* TSan. Does it now work correctly (report 0600)?

Let me know if think sounds too time-consuming and I can certainly do that myself with a remote access to a machine :)

Thanks!!!

[kubamracek]

...and could you also see if perhaps this change helps:

diff --git a/compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp b/compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp
--- a/compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp
+++ b/compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp
@@ -137,7 +137,11 @@ const int PTHREAD_BARRIER_SERIAL_THREAD = -1;
 #endif
 const int MAP_FIXED = 0x10;
 typedef long long_t;
+#if SANITIZER_MAC || SANITIZER_FREEBSD || SANITIZER_NETBSD
 typedef __sanitizer::u16 mode_t;
+#else
+typedef __sanitizer::u32 mode_t;
+#endif
 
 // From /usr/include/unistd.h
 # define F_ULOCK 0      /* Unlock a previously locked region.  */

[nemanjai]

I'll send you the results in a few minutes. Sorry about the delay - holiday here in Canada.

[nemanjai]

It would appear that this failure is not really related to this patch at all. Seems to fail with -O0/-O1 regardless of which patch is applied.
https://pastebin.com/1Mvz5KDz

Please let me know if there's something else I can help with.

[nemanjai]

In D84570#2190757, @nemanjai wrote:

It would appear that this failure is not really related to this patch at all. Seems to fail with -O0/-O1 regardless of which patch is applied.
https://pastebin.com/1Mvz5KDz

Please let me know if there's something else I can help with.

At a very quick glance, the two versions call different interceptors.
The -O1 version calls __interceptor_fstat and the -O2 version calls __interceptor___fxstat.
I am not sure if that reveals anything further, but that seems to be the major difference between the two.

[kubamracek]

If I read the output right, it seems that even before this change, running with TSan messes up the permissions argument. I think this may be some latent bug in glibc that is only triggered in unusual environments (like TSan). I found this comment at https://lkml.org/lkml/2014/10/30/812:

... uses a conditional va_arg() to get
the third parameter *despite* not even being a variadic function (not
varargs, not stdarg). So it's not even portable or correct *anyway*,

Which sounds relevant. Just a guess, but maybe the varargs code in glibc is technically wrong, but under normal conditions, it just ends up being lucky and the right value ends up at the right register/stackslot?

compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp

 #include "tsan_interceptors.h"
 #include "tsan_interface.h"
 #include "tsan_platform.h"
 #include "tsan_suppressions.h"
 #include "tsan_rtl.h"
 #include "tsan_mman.h"
 #include "tsan_fd.h"
 
+#include <stdarg.h>
+
 using namespace __tsan;
 
 #if SANITIZER_FREEBSD || SANITIZER_MAC
 #define stdout __stdoutp
 #define stderr __stderrp
 #endif
 
 #if SANITIZER_NETBSD

 void *const MAP_FAILED = (void*)-1;
 #if SANITIZER_NETBSD
 const int PTHREAD_BARRIER_SERIAL_THREAD = 1234567;
 #elif !SANITIZER_MAC
 const int PTHREAD_BARRIER_SERIAL_THREAD = -1;
 #endif
 const int MAP_FIXED = 0x10;
 typedef long long_t;
+typedef __sanitizer::u16 mode_t;
 
 // From /usr/include/unistd.h
 # define F_ULOCK 0      /* Unlock a previously locked region.  */
 # define F_LOCK  1      /* Lock a region for exclusive use.  */
 # define F_TLOCK 2      /* Test and lock a region for exclusive use.  */
 # define F_TEST  3      /* Test a region for other processes locks.  */
 
 #if SANITIZER_FREEBSD || SANITIZER_MAC || SANITIZER_NETBSD
if (fd > 0)
     FdAccess(thr, pc, fd);
   return REAL(__fxstat64)(0, fd, buf);
 }
 #define TSAN_MAYBE_INTERCEPT_FSTAT64 TSAN_INTERCEPT(fstat64)
 #else
 #define TSAN_MAYBE_INTERCEPT_FSTAT64
 #endif
 
-TSAN_INTERCEPTOR(int, open, const char *name, int flags, int mode) {
-  SCOPED_TSAN_INTERCEPTOR(open, name, flags, mode);
+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, flags, mode);
+  int fd = REAL(open)(name, oflag, mode);
   if (fd >= 0)
     FdFileCreate(thr, pc, fd);
   return fd;
 }
 
 #if SANITIZER_LINUX
-TSAN_INTERCEPTOR(int, open64, const char *name, int flags, int mode) {
-  SCOPED_TSAN_INTERCEPTOR(open64, name, flags, mode);
+TSAN_INTERCEPTOR(int, open64, 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(open64, name, oflag, mode);
   READ_STRING(thr, pc, name, 0);
-  int fd = REAL(open64)(name, flags, mode);
+  int fd = REAL(open64)(name, oflag, mode);
   if (fd >= 0)
     FdFileCreate(thr, pc, fd);
   return fd;
 }
 #define TSAN_MAYBE_INTERCEPT_OPEN64 TSAN_INTERCEPT(open64)
 #else
 #define TSAN_MAYBE_INTERCEPT_OPEN64
 #endif

compiler-rt/test/tsan/variadic-open.cpp

+// RUN: %clangxx_tsan -O1 %s -o %t && %run %t %t.tmp 2>&1 | FileCheck %s
+#include <stdio.h>
+#include <assert.h>
+#include <fcntl.h>
+#include <unistd.h>
+#include <sys/stat.h>
+
+int main(int argc, char *argv[]) {
+  fprintf(stderr, "Hello world.\n");
+  assert(argv[1]);
+  unlink(argv[1]);
+  int fd = open(argv[1], O_RDWR | O_CREAT, 0600);
+  assert(fd != -1);
+  struct stat info;
+  int result = fstat(fd, &info);
+  fprintf(stderr, "permissions = 0%o\n", info.st_mode & ~S_IFMT);
+  assert(result == 0);
+  close(fd);
+  fprintf(stderr, "Done.\n");
+}
+
+// CHECK: Hello world.
+// CHECK: permissions = 0600
+// CHECK: Done.