This is an archive of the discontinued LLVM Phabricator instance.

[Sanitizer][ASAN][MSAN] Fix infinite recursion on FreeBSD
ClosedPublic

Authored by arichardson on Jul 24 2019, 9:47 AM.

Details

Summary

MSAN was broken on FreeBSD by https://reviews.llvm.org/D55703: after this
change accesses to the key variable call tls_get_addr, which is
intercepted. The interceptor then calls GetCurrentThread which calls
MsanTSDGet which again calls
tls_get_addr, etc...
Using the default implementation in the SANITIZER_FREEBSD case fixes MSAN
for me.

I then applied the same change to ASAN (introduced in https://reviews.llvm.org/D55596)
but that did not work yet. In the ASAN case, we get infinite recursion
again during initialization, this time because calling pthread_key_create() early on
results in infinite recursion. pthread_key_create() calls sysctlbyname()
which is intercepted but COMMON_INTERCEPTOR_NOTHING_IS_INITIALIZED returns
true, so the interceptor calls internal_sysctlbyname() which then ends up
calling the interceptor again. I fixed this issue by using dlsym() to get
the libc version of sysctlbyname() instead.

This fixes https://llvm.org/PR40761

Event Timeline

arichardson created this revision.Jul 24 2019, 9:47 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJul 24 2019, 9:47 AM
Herald added subscribers: llvm-commits, Restricted Project, jfb and 2 others. · View Herald Transcript

Is any test was broken?

dim added a comment.Jul 24 2019, 11:11 PM

Is any test was broken?

As described in https://bugs.llvm.org/show_bug.cgi?id=40761, almost all sanitizer tests crash with DEADLYSIGNAL due to the infinite recursion.

arichardson edited the summary of this revision. (Show Details)Jul 25 2019, 1:01 AM

Remove freebsd XFAIL from two tests that pass with this change

MaskRay added inline comments.
compiler-rt/lib/sanitizer_common/sanitizer_linux.cc
780

Change it to using and place it inside the function.

786

When is dlfunc(RTLD_DEFAULT, "sysctlbyname") used?

devnexen added inline comments.Jul 26 2019, 2:16 AM
compiler-rt/lib/sanitizer_common/sanitizer_linux.cc
786

I wonder the same do you have a situation when it occurs ?

arichardson marked an inline comment as done.Jul 26 2019, 2:42 AM
arichardson added inline comments.
compiler-rt/lib/sanitizer_common/sanitizer_linux.cc
786

Indeed that does seem unlikely (I guess it could theoretically happen statically linking the runtime into libc?).
I copied this code from the sanitizer_netbsd.cc file and assumed there might be a case where the RTLD_DEFAULT case is needed.
But it does seem like more things would break if this is ever needed so I'll remove it.

  • Address review feedback
  • Remove XFAIL: freebsd from another test that now passes

Fine by me but prefers someone else giving the greenlight.

MaskRay added inline comments.Jul 26 2019, 3:00 AM
compiler-rt/lib/sanitizer_common/sanitizer_linux.cc
780

If the signature of internal_sysctlbyname is the same as sysctlbyname, I believe the below will be simpler:

static void *real;
if (!real)
  real = dlsym(RTLD_NEXT, "sysctlbyname");
return (decltype(internal_sysctlbyname)*(real))(sname, oldp, oldlenp, newp, newlen);

I wonder if FreeBSD specific dlfunc is still relevant nowadays...

FreeBSD dlsym(3): "in the C standard, conversions between data and function pointer types are undefined"

Yet, C11 J.5.7 Function pointer casts (Annex J. I guess all working C compilers support this..):

A pointer to an object or to void may be cast to a pointer to a function, allowing data to be invoked as a function (6.5.4).

C++11 [expr.reinterpret.cast]

Converting a function pointer to an object pointer type or vice versa is conditionally-supported.

arichardson marked an inline comment as done.Jul 26 2019, 3:16 AM
arichardson added inline comments.
compiler-rt/lib/sanitizer_common/sanitizer_linux.cc
780

Theoretically, function pointers could be a different size from data pointers (e.g. a pair of pointers for code+got, or even something smaller if you have separate code and data address spaces). But since so much C code assumes that you can store them in a void* I'm not sure any relevant architecture actually uses different sizes.

I quite like dlfunc() because using it explicitly states that you are looking up a function and not a data symbol. FreeBSD rtld doesn't actually differntiate it right now, but I was considering changing it to return NULL if the lookup yields a non-function symbol.

However, using dlfunc() is only fine because this is in a FreeBSD specific block so if other operating systems want to use this function it would need to change. I don't mind changing it to use dlsym() if you think that may be the case in the future.

arichardson marked an inline comment as done.Jul 26 2019, 3:19 AM
arichardson added inline comments.
compiler-rt/lib/sanitizer_common/sanitizer_linux.cc
780

Unfortunately uptr* is not the same as size_t*. I'm not sure why the signature is different but changing the signature would require updating the other implementations, too.

Use the more portable dlsym() instead of dlfunc()

dim added inline comments.Jul 28 2019, 2:58 AM
compiler-rt/lib/sanitizer_common/sanitizer_linux.cc
786

Note that I got an 80 column -Werror warning about this line. Please move the (size_t) newlen part to the next line.

git-clang-format

arichardson marked an inline comment as done.

Remove mention of FreeBSD from comment

krytarowski added inline comments.Jul 28 2019, 3:03 PM
compiler-rt/lib/sanitizer_common/sanitizer_linux.cc
780

size_t is popularly replaced with uptr that in practice is the same underlying type.

arichardson added a reviewer: Restricted Project.Jul 29 2019, 3:15 AM
arichardson marked an inline comment as done.
arichardson added inline comments.
compiler-rt/lib/sanitizer_common/sanitizer_linux.cc
780

I believe I got an error without the cast. Maybe for i386 size_t is int and uptr long? Unrelated to this review, the system that I work on (CHERI on FreeBSD) has 64-bit size_t and 128-bit uinptr_t so they are definitely not always compatible.

Also these casts were already required before my patch.

krytarowski added inline comments.Jul 30 2019, 1:02 PM
compiler-rt/lib/sanitizer_common/sanitizer_linux.cc
780

128-bit uinptr_t? 128bit pointers?

If there are 128 bit pointers then there are probably bigger problems than this cast here.

arichardson marked an inline comment as done.Jul 30 2019, 1:16 PM
arichardson added inline comments.
compiler-rt/lib/sanitizer_common/sanitizer_linux.cc
780

Yes exactly (see http://www.chericpu.com for more details). But as I said this is unrelated to this review.

This change is just about making ASAN work again on FreeBSD (x86).
It's been broken for all of 8.0 and it would be very nice to have it working again before 9.0 is shipped.

MaskRay added inline comments.Jul 30 2019, 10:49 PM
compiler-rt/lib/sanitizer_common/sanitizer_linux.cc
780

I think you still miss the example I gave above.

using syctlbyname_ptr should not be needed and should not be placed outside the function (no other functions will use it).

arichardson marked 2 inline comments as done.

Used decltype now. Should be a bit shorter than the using declaration.
I still need the casts, otherwise the build fails when building the i368 RTSanitizerCommon.i386.dir/sanitizer_linux.cc.o:

/exports/users/alr48/sources/upstream-llvm-project/compiler-rt/lib/sanitizer_common/sanitizer_linux.cc:786:57: error: cannot initialize a parameter of type 'size_t *' (aka 'unsigned int *') with an lvalue of type '__sanitizer::uptr *' (aka 'unsigned long *')

Move the cast to dlsym() return value rather than the function pointer call

MaskRay accepted this revision.Jul 31 2019, 7:35 AM
This revision is now accepted and ready to land.Jul 31 2019, 7:35 AM
This revision was automatically updated to reflect the committed changes.