Fix gettid() after clone().
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
diff --git a/libc/bionic/clone.cpp b/libc/bionic/clone.cpp
index 8281ac8..b50a96d 100644
--- a/libc/bionic/clone.cpp
+++ b/libc/bionic/clone.cpp
@@ -75,6 +75,15 @@
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 @@
#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;
}
diff --git a/libc/bionic/gettid.cpp b/libc/bionic/gettid.cpp
index f42e36a..fe25a4d 100644
--- a/libc/bionic/gettid.cpp
+++ b/libc/bionic/gettid.cpp
@@ -31,5 +31,14 @@
#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);
}
diff --git a/tests/unistd_test.cpp b/tests/unistd_test.cpp
index 80ebf6b..660679f 100644
--- a/tests/unistd_test.cpp
+++ b/tests/unistd_test.cpp
@@ -434,7 +434,7 @@
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 @@
}
}
+// 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 @@
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 @@
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 @@
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) {