OSDN Git Service

Fix gettid() after clone().
authorRobert Sesek <rsesek@google.com>
Tue, 25 Oct 2016 14:29:02 +0000 (10:29 -0400)
committerRobert Sesek <rsesek@google.com>
Fri, 28 Oct 2016 16:14:23 +0000 (12:14 -0400)
The tid is cached in the pthread_internal_t and is properly re-set after fork()
and pthread_create(). But after a plain clone() the value is stale from the
parent.

Test: mmma bionic/tests
Test: bionic-unit-tests-static --gtest_filter=*fork*:*clone*
Test: m checkbuild tests
Test: angler boots

Bug: 32305649
Change-Id: I026d416d1537484cd3e05c8493a35e5ed2acc8ed

libc/bionic/clone.cpp
libc/bionic/gettid.cpp
tests/unistd_test.cpp

index 8281ac8..b50a96d 100644 (file)
@@ -75,6 +75,15 @@ int clone(int (*fn)(void*), void* child_stack, int flags, void* arg, ...) {
   pthread_internal_t* self = __get_thread();
   pid_t parent_pid = self->invalidate_cached_pid();
 
+  // Remmber the caller's tid so that it can be restored in the parent after clone.
+  pid_t caller_tid = self->tid;
+  // Invalidate the tid before the syscall. The value is lazily cached in gettid(),
+  // and it will be updated by fork() and pthread_create(). We don't do this if
+  // we are sharing address space with the child.
+  if (!(flags & (CLONE_VM|CLONE_VFORK))) {
+    self->tid = -1;
+  }
+
   // Actually do the clone.
   int clone_result;
   if (fn != nullptr) {
@@ -87,11 +96,16 @@ int clone(int (*fn)(void*), void* child_stack, int flags, void* arg, ...) {
 #endif
   }
 
-  // We're the parent, so put our known pid back in place.
-  // We leave the child without a cached pid, but:
-  // 1. pthread_create gives its children their own pthread_internal_t with the correct pid.
-  // 2. fork makes a clone system call directly.
-  // If any other cases become important, we could use a double trampoline like __pthread_start.
-  self->set_cached_pid(parent_pid);
+  if (clone_result != 0) {
+    // We're the parent, so put our known pid and tid back in place.
+    // We leave the child without a cached pid and tid, but:
+    // 1. pthread_create gives its children their own pthread_internal_t with the correct pid and tid.
+    // 2. fork uses CLONE_CHILD_SETTID to get the new pid/tid.
+    // 3. The tid is lazily fetched in gettid().
+    // If any other cases become important, we could use a double trampoline like __pthread_start.
+    self->set_cached_pid(parent_pid);
+    self->tid = caller_tid;
+  }
+
   return clone_result;
 }
index f42e36a..fe25a4d 100644 (file)
 #include "pthread_internal.h"
 
 pid_t gettid() {
-  return __get_thread()->tid;
+  pthread_internal_t* self = __get_thread();
+  if (__predict_true(self)) {
+    pid_t tid = self->tid;
+    if (__predict_true(tid != -1)) {
+      return tid;
+    }
+    self->tid = syscall(__NR_gettid);
+    return self->tid;
+  }
+  return syscall(__NR_gettid);
 }
index 80ebf6b..660679f 100644 (file)
@@ -434,7 +434,7 @@ static void TestGetPidCachingWithFork(int (*fork_fn)()) {
   ASSERT_NE(fork_result, -1);
   if (fork_result == 0) {
     // We're the child.
-    AssertGetPidCorrect();
+    ASSERT_NO_FATAL_FAILURE(AssertGetPidCorrect());
     ASSERT_EQ(parent_pid, getppid());
     _exit(123);
   } else {
@@ -444,14 +444,100 @@ static void TestGetPidCachingWithFork(int (*fork_fn)()) {
   }
 }
 
+// gettid() is marked as __attribute_const__, which will have the compiler
+// optimize out multiple calls to gettid in the same function. This wrapper
+// defeats that optimization.
+static __attribute__((__noinline__)) pid_t GetTidForTest() {
+  __asm__("");
+  return gettid();
+}
+
+static void AssertGetTidCorrect() {
+  // The loop is just to make manual testing/debugging with strace easier.
+  pid_t gettid_syscall_result = syscall(__NR_gettid);
+  for (size_t i = 0; i < 128; ++i) {
+    ASSERT_EQ(gettid_syscall_result, GetTidForTest());
+  }
+}
+
+static void TestGetTidCachingWithFork(int (*fork_fn)()) {
+  pid_t parent_tid = GetTidForTest();
+  ASSERT_EQ(syscall(__NR_gettid), parent_tid);
+
+  pid_t fork_result = fork_fn();
+  ASSERT_NE(fork_result, -1);
+  if (fork_result == 0) {
+    // We're the child.
+    EXPECT_EQ(syscall(__NR_getpid), syscall(__NR_gettid));
+    EXPECT_EQ(getpid(), GetTidForTest()) << "real tid is " << syscall(__NR_gettid)
+                                         << ", pid is " << syscall(__NR_getpid);
+    ASSERT_NO_FATAL_FAILURE(AssertGetTidCorrect());
+    _exit(123);
+  } else {
+    // We're the parent.
+    ASSERT_EQ(parent_tid, GetTidForTest());
+    AssertChildExited(fork_result, 123);
+  }
+}
+
 TEST(UNISTD_TEST, getpid_caching_and_fork) {
   TestGetPidCachingWithFork(fork);
 }
 
+TEST(UNISTD_TEST, gettid_caching_and_fork) {
+  TestGetTidCachingWithFork(fork);
+}
+
 TEST(UNISTD_TEST, getpid_caching_and_vfork) {
   TestGetPidCachingWithFork(vfork);
 }
 
+static int CloneLikeFork() {
+  return clone(nullptr, nullptr, SIGCHLD, nullptr);
+}
+
+TEST(UNISTD_TEST, getpid_caching_and_clone_process) {
+  TestGetPidCachingWithFork(CloneLikeFork);
+}
+
+TEST(UNISTD_TEST, gettid_caching_and_clone_process) {
+  TestGetTidCachingWithFork(CloneLikeFork);
+}
+
+static int CloneAndSetTid() {
+  pid_t child_tid = 0;
+  pid_t parent_tid = GetTidForTest();
+
+  int rv = clone(nullptr, nullptr, CLONE_CHILD_SETTID | SIGCHLD, nullptr, nullptr, nullptr, &child_tid);
+  EXPECT_NE(-1, rv);
+
+  if (rv == 0) {
+    // Child.
+    EXPECT_EQ(child_tid, GetTidForTest());
+    EXPECT_NE(child_tid, parent_tid);
+  } else {
+    EXPECT_NE(child_tid, GetTidForTest());
+    EXPECT_NE(child_tid, parent_tid);
+    EXPECT_EQ(GetTidForTest(), parent_tid);
+  }
+
+  return rv;
+}
+
+TEST(UNISTD_TEST, gettid_caching_and_clone_process_settid) {
+  TestGetTidCachingWithFork(CloneAndSetTid);
+}
+
+static int CloneStartRoutine(int (*start_routine)(void*)) {
+  void* child_stack[1024];
+  int clone_result = clone(start_routine, &child_stack[1024], CLONE_NEWNS | SIGCHLD, NULL);
+  if (clone_result == -1 && errno == EPERM && getuid() != 0) {
+    GTEST_LOG_(INFO) << "This test only works if you have permission to CLONE_NEWNS; try running as root.\n";
+    return clone_result;
+  }
+  return clone_result;
+}
+
 static int GetPidCachingCloneStartRoutine(void*) {
   AssertGetPidCorrect();
   return 123;
@@ -461,12 +547,7 @@ TEST(UNISTD_TEST, getpid_caching_and_clone) {
   pid_t parent_pid = getpid();
   ASSERT_EQ(syscall(__NR_getpid), parent_pid);
 
-  void* child_stack[1024];
-  int clone_result = clone(GetPidCachingCloneStartRoutine, &child_stack[1024], CLONE_NEWNS | SIGCHLD, NULL);
-  if (clone_result == -1 && errno == EPERM && getuid() != 0) {
-    GTEST_LOG_(INFO) << "This test only works if you have permission to CLONE_NEWNS; try running as root.\n";
-    return;
-  }
+  int clone_result = CloneStartRoutine(GetPidCachingCloneStartRoutine);
   ASSERT_NE(clone_result, -1);
 
   ASSERT_EQ(parent_pid, getpid());
@@ -474,6 +555,23 @@ TEST(UNISTD_TEST, getpid_caching_and_clone) {
   AssertChildExited(clone_result, 123);
 }
 
+static int GetTidCachingCloneStartRoutine(void*) {
+  AssertGetTidCorrect();
+  return 123;
+}
+
+TEST(UNISTD_TEST, gettid_caching_and_clone) {
+  pid_t parent_tid = GetTidForTest();
+  ASSERT_EQ(syscall(__NR_gettid), parent_tid);
+
+  int clone_result = CloneStartRoutine(GetTidCachingCloneStartRoutine);
+  ASSERT_NE(clone_result, -1);
+
+  ASSERT_EQ(parent_tid, GetTidForTest());
+
+  AssertChildExited(clone_result, 123);
+}
+
 static void* GetPidCachingPthreadStartRoutine(void*) {
   AssertGetPidCorrect();
   return NULL;
@@ -492,6 +590,25 @@ TEST(UNISTD_TEST, getpid_caching_and_pthread_create) {
   ASSERT_EQ(NULL, result);
 }
 
+static void* GetTidCachingPthreadStartRoutine(void*) {
+  AssertGetTidCorrect();
+  uint64_t tid = GetTidForTest();
+  return reinterpret_cast<void*>(tid);
+}
+
+TEST(UNISTD_TEST, gettid_caching_and_pthread_create) {
+  pid_t parent_tid = GetTidForTest();
+
+  pthread_t t;
+  ASSERT_EQ(0, pthread_create(&t, NULL, GetTidCachingPthreadStartRoutine, &parent_tid));
+
+  ASSERT_EQ(parent_tid, GetTidForTest());
+
+  void* result;
+  ASSERT_EQ(0, pthread_join(t, &result));
+  ASSERT_NE(static_cast<uint64_t>(parent_tid), reinterpret_cast<uint64_t>(result));
+}
+
 class UNISTD_DEATHTEST : public BionicDeathTest {};
 
 TEST_F(UNISTD_DEATHTEST, abort) {