Page MenuHomePhabricator

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

Authored by kubamracek on Jul 24 2020, 5:20 PM.

Details

Summary

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

Diff Detail

Event Timeline

kubamracek created this revision.Jul 24 2020, 5:20 PM
Herald added subscribers: Restricted Project, Charusso, kristof.beyls. · View Herald TranscriptJul 24 2020, 5:20 PM
dvyukov edited reviewers, added: vitalybuka; removed: dvyukov.Jul 24 2020, 9:46 PM

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 accepted this revision.Jul 28 2020, 12:06 AM

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)
This revision is now accepted and ready to land.Jul 28 2020, 12:06 AM
kubamracek added inline comments.Jul 29 2020, 12:21 PM
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 added inline comments.Jul 29 2020, 12:23 PM
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?

This revision was landed with ongoing or failed builds.Jul 30 2020, 9:01 AM
This revision was automatically updated to reflect the committed changes.

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.

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

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

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.

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!!!

...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.  */

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

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.

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.

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?