Don't assume PTHREAD_CREATE_JOINABLE is 0 on all systems

[fjricci]

Summary

Lsan was using PTHREAD_CREATE_JOINABLE/PTHREAD_CREATE_DETACHED
as truthy values, which works on Linux, where the values are 0 and 1,
but this fails on OS X, where the values are 1 and 2.

Set PTHREAD_CREATE_DETACHED to the correct value for a given system.

[alekseyshl]

lib/lsan/lsan_interceptors.cc

54

Why do we have to define them? why not use the proper definition in phtread.h?

269

int detached = PTHREAD_CREATE_JOINABLE; to be consistent with the rest of your change.

[fjricci]

lib/lsan/lsan_interceptors.cc

54

We don't want to include any system headers in this file.

269

This review established that we don't want to define PTHREAD_CREATE_JOINABLE (that value will get over-written by pthread_attr_getdetachstate anyway): D10606

[alekseyshl]

lib/lsan/lsan_interceptors.cc

54

Oh, we already have those defined in tsan... Let's move the definition to saniter_linux.h and sanitizer_mac.h, define them as enum (yes, I'd still vote for defining PTHREAD_CREATE_JOINABLE, I don't see the upside of not defining it) and reuse them in both tsan and lsan.

269

pthread_attr_getdetachstate returns int and can fail, we're ignoring the error and go with a default value. I admit, it does not change the result of detached == PTHREAD_CREATE_DETACHED check, but does not feel like a sound practice.

[fjricci]

Address comments

[alekseyshl]

lib/sanitizer_common/sanitizer_linux.h

60

I'd prefer them to be an anonymous enum:

enum {

PTHREAD_CREATE_JOINABLE = 0,
PTHREAD_CREATE_DETACHED = 1

}

Preprocessor is evil.

[fjricci]

So I think we may have to return to the original method of defining the value in the source files. sanitizer_mac.h is included in files that also include pthread.h, which causes a multiple definition error. We can't use an ifndef, because the pthreads header isn't necessarily included first. If we want to keep these in sanitizer_mac and sanitizer_linux, we'll need to rename them to something that won't conflict with the pthreads name. Thoughts?

[fjricci]

Update to avoid conflicts with definitions from pthreads header

It doesn't look like we have any existing uses of the const-type constants
in platform-specific headers currently though, so I'm not sure that this is
better than the original solution of defining inside the source file.

[kubamracek]

Can we use the original names instead of kCreateJoinable and kCreateDetached? The name "kCreateDetached" suggests that it's some arbitrary sanitizer-internal constant, but that's not true. It's a platform-defined value that we must follow.

[fjricci]

In D31883#724975, @kubamracek wrote:

Can we use the original names instead of kCreateJoinable and kCreateDetached? The name "kCreateDetached" suggests that it's some arbitrary sanitizer-internal constant, but that's not true. It's a platform-defined value that we must follow.

The problem with the original name is that we hit conflicts in files that include both 'sanitizer_linux.h' and 'pthread.h', due to a multiple definition.

[fjricci]

In D31883#724977, @fjricci wrote:

In D31883#724975, @kubamracek wrote:

Can we use the original names instead of kCreateJoinable and kCreateDetached? The name "kCreateDetached" suggests that it's some arbitrary sanitizer-internal constant, but that's not true. It's a platform-defined value that we must follow.

The problem with the original name is that we hit conflicts in files that include both 'sanitizer_linux.h' and 'pthread.h', due to a multiple definition.

Well, not technically a multiple definition, but after the preprocessor replaces PTHREAD_CREATE_JOINABLE, you end up with a statement like '#define 0 = 0` in whichever header is included second, which is an error.

[alekseyshl]

Hmm, what about this then, define bool IsThreadAttached wrapper function in sanitizer_common/sanitizer_{linux|mac}, the same way we do for GetTid(). Definitions from pthread.h are available there, feels like a cleaner solution.

[fjricci]

Add wrapper function for checking thread detached state

[fjricci]

Fix typo

[alekseyshl]

lib/lsan/lsan_interceptors.cc

286

What about !SANITIZER_POSIX case? Who's going to provide this function?

[fjricci]

lib/lsan/lsan_interceptors.cc

286

Lots of other things in this function will also break in the !SANITIZER_POSIX case - pthread_attr_getdetachstate, pthread_create, etc. I don't think this file is currently designed to work at all in non-posix cases.

[alekseyshl]

Other than tsan_interceptors.cc question, lgtm.

lib/lsan/lsan_interceptors.cc

286

Let's remove #if SANITIZER_POSIX around include then, it's misleading.

What about tsan_interceptors.cc? Is it also posix only?

[fjricci]

lib/lsan/lsan_interceptors.cc

286

Yeah, tsan is also posix only, and there is definitely quite a bit of posix-specific code in that file. Interestingly, they do use the SANITIZER_POSIX guard on the header inclusion in that file, perhaps for future-proofing?

[alekseyshl]

lib/lsan/lsan_interceptors.cc

286

I don't know why. It's trivial to add back and in case of future changes, the failure is more explicit when there's no ifdef around the include.

[fjricci]

lib/lsan/lsan_interceptors.cc

286

Ok, sounds good. I'll remove it from the tsan file as well then, for consistency.

lib/lsan/lsan_interceptors.cc

 
 #include "interception/interception.h"
 #include "sanitizer_common/sanitizer_allocator.h"
 #include "sanitizer_common/sanitizer_atomic.h"
 #include "sanitizer_common/sanitizer_common.h"
 #include "sanitizer_common/sanitizer_flags.h"
 #include "sanitizer_common/sanitizer_internal_defs.h"
 #include "sanitizer_common/sanitizer_linux.h"
+#include "sanitizer_common/sanitizer_mac.h"
 #include "sanitizer_common/sanitizer_platform_interceptors.h"
 #include "sanitizer_common/sanitizer_platform_limits_posix.h"
 #include "sanitizer_common/sanitizer_tls_get_addr.h"
 #include "lsan.h"
 #include "lsan_allocator.h"
 #include "lsan_common.h"
 #include "lsan_thread.h"
 
namespace std {
   struct nothrow_t;
 }
 
 #if !SANITIZER_MAC
 INTERCEPTOR(void*, malloc, uptr size) {
   ENSURE_LSAN_INITED;
   GET_STACK_TRACE_MALLOC;
   return lsan_malloc(size, stack);
 }
 
 INTERCEPTOR(void, free, void *p) {
   ENSURE_LSAN_INITED;
   lsan_free(p);
 }
 
 INTERCEPTOR(void*, calloc, uptr nmemb, uptr size) {
   if (lsan_init_is_running) {
INTERCEPTOR(int, pthread_create, void *th, void *attr,
   ENSURE_LSAN_INITED;
   EnsureMainThreadIDIsCorrect();
   __sanitizer_pthread_attr_t myattr;
   if (!attr) {
     pthread_attr_init(&myattr);
     attr = &myattr;
   }
   AdjustStackSize(attr);
-  int detached = 0;
+  int detached = kCreateJoinable;
   pthread_attr_getdetachstate(attr, &detached);
   ThreadParam p;
   p.callback = callback;
   p.param = param;
   atomic_store(&p.tid, 0, memory_order_relaxed);
   int res;
   {
     // Ignore all allocations made by pthread_create: thread stack/TLS may be
     // stored by pthread for future reuse even after thread destruction, and
     // the linked list it's stored in doesn't even hold valid pointers to the
     // objects, the latter are calculated by obscure pointer arithmetic.
     ScopedInterceptorDisabler disabler;
     res = REAL(pthread_create)(th, attr, __lsan_thread_start_func, &p);
   }
   if (res == 0) {
-    int tid = ThreadCreate(GetCurrentThread(), *(uptr *)th, detached);
+    int tid = ThreadCreate(GetCurrentThread(), *(uptr *)th,
+                           detached == kCreateDetached);
     CHECK_NE(tid, 0);
     atomic_store(&p.tid, tid, memory_order_release);
     while (atomic_load(&p.tid, memory_order_acquire) != 0)
       internal_sched_yield();
   }
   if (attr == &myattr)
     pthread_attr_destroy(&myattr);
   return res;

lib/sanitizer_common/sanitizer_linux.h

 #if defined(__x86_64__) || defined(__mips__) || defined(__aarch64__) \
   || defined(__powerpc64__) || defined(__s390__) || defined(__i386__) \
   || defined(__arm__)
 uptr internal_clone(int (*fn)(void *), void *child_stack, int flags, void *arg,
                     int *parent_tidptr, void *newtls, int *child_tidptr);
 #endif
 #endif  // SANITIZER_LINUX
 
+static const int kCreateJoinable = 0;
+static const int kCreateDetached = 1;
+
 // This class reads thread IDs from /proc/<pid>/task using only syscalls.
 class ThreadLister {
  public:
   explicit ThreadLister(int pid);
   ~ThreadLister();
   // GetNextTID returns -1 if the list of threads is exhausted, or if there has
   // been an error.
   int GetNextTID();

lib/sanitizer_common/sanitizer_mac.h

 };
 
 MacosVersion GetMacosVersion();
 
 char **GetEnviron();
 
 }  // namespace __sanitizer
 
+static const int kCreateJoinable = 1;
+static const int kCreateDetached = 2;
+
 extern "C" {
 static char __crashreporter_info_buff__[__sanitizer::kErrorMessageBufferSize] =
   {};
 static const char *__crashreporter_info__ __attribute__((__used__)) =
   &__crashreporter_info_buff__[0];
 asm(".desc ___crashreporter_info__, 0x10");
 } // extern "C"
 

lib/tsan/rtl/tsan_interceptors.cc

 //
 // FIXME: move as many interceptors as possible into
 // sanitizer_common/sanitizer_common_interceptors.inc
 //===----------------------------------------------------------------------===//
 
 #include "sanitizer_common/sanitizer_atomic.h"
 #include "sanitizer_common/sanitizer_libc.h"
 #include "sanitizer_common/sanitizer_linux.h"
+#include "sanitizer_common/sanitizer_mac.h"
 #include "sanitizer_common/sanitizer_platform_limits_posix.h"
 #include "sanitizer_common/sanitizer_placement_new.h"
 #include "sanitizer_common/sanitizer_stacktrace.h"
 #include "sanitizer_common/sanitizer_tls_get_addr.h"
 #include "interception/interception.h"
 #include "tsan_interceptors.h"
 #include "tsan_interface.h"
 #include "tsan_platform.h"

 #define stderr __stderrp
 #endif
 
 #if SANITIZER_ANDROID
 #define __errno_location __errno
 #define mallopt(a, b)
 #endif
 
-#if SANITIZER_LINUX || SANITIZER_FREEBSD
-#define PTHREAD_CREATE_DETACHED 1
-#elif SANITIZER_MAC
-#define PTHREAD_CREATE_DETACHED 2
-#endif
-
-
 #ifdef __mips__
 const int kSigCount = 129;
 #else
 const int kSigCount = 65;
 #endif
 
 struct my_siginfo_t {
   // The size is determined by looking at sizeof of real siginfo_t on linux.
if (flags()->die_after_fork) {
           "die_after_fork=0, but you are on your own\n", internal_getpid());
     }
   }
   __sanitizer_pthread_attr_t myattr;
   if (attr == 0) {
     pthread_attr_init(&myattr);
     attr = &myattr;
   }
-  int detached = 0;
+  int detached = kCreateJoinable;
   REAL(pthread_attr_getdetachstate)(attr, &detached);
   AdjustStackSize(attr);
 
   ThreadParam p;
   p.callback = callback;
   p.param = param;
   atomic_store(&p.tid, 0, memory_order_relaxed);
   int res = -1;
   {
     // Otherwise we see false positives in pthread stack manipulation.
     ScopedIgnoreInterceptors ignore;
     ThreadIgnoreBegin(thr, pc);
     res = REAL(pthread_create)(th, attr, __tsan_thread_start_func, &p);
     ThreadIgnoreEnd(thr, pc);
   }
   if (res == 0) {
     int tid = ThreadCreate(thr, pc, *(uptr*)th,
-                           detached == PTHREAD_CREATE_DETACHED);
+                           detached == kCreateDetached);
     CHECK_NE(tid, 0);
     // Synchronization on p.tid serves two purposes:
     // 1. ThreadCreate must finish before the new thread starts.
     //    Otherwise the new thread can call pthread_detach, but the pthread_t
     //    identifier is not yet registered in ThreadRegistry by ThreadCreate.
     // 2. ThreadStart must finish before this thread continues.
     //    Otherwise, this thread can call pthread_detach and reset thr->sync
     //    before the new thread got a chance to acquire from it in ThreadStart.