commit fa9e16efaf0e885f6044e725eb759ef6de10f7ef
author Elliott Hughes <enh@google.com> Mon Jun 23 17:49:45 2014 -0700
committer Elliott Hughes <enh@google.com> Mon Jun 23 17:49:45 2014 -0700
tree b2788038c17cde5a1d53c13a51d6a16868bd229f
parent 0d7415fb225573c37ff9e880957cd2044dd658ce

Fix getpid caching across a clone.

If you make clone, fork, or vfork system calls directly, you're still
on your own, but we now do the right thing for the clone wrapper.
With this implementation, children lose the getpid caching, but we've
no reason to think that that covers any significant use cases.

Bug: 15387103
Change-Id: Icfab6b63c708fea830960742ec92aeba8ce7680d

libc/bionic/clone.cpp

diff --git a/libc/bionic/clone.cpp b/libc/bionic/clone.cpp
index 001e245..0a0fdd5 100644
--- a/libc/bionic/clone.cpp
+++ b/libc/bionic/clone.cpp
@@ -31,6 +31,8 @@
 #include <stdlib.h>
 #include <stdarg.h>
 
+#include "pthread_internal.h"
+
 extern "C" pid_t __bionic_clone(uint32_t flags, void* child_stack, int* parent_tid, void* tls, int* child_tid, int (*fn)(void*), void* arg);
 extern "C" __noreturn void __exit(int status);
 
@@ -64,5 +66,18 @@
   child_stack_addr &= ~0xf;
   child_stack = reinterpret_cast<void*>(child_stack_addr);
 
-  return __bionic_clone(flags, child_stack, parent_tid, new_tls, child_tid, fn, arg);
+  // Remember the parent pid and invalidate the cached value while we clone.
+  pthread_internal_t* self = __get_thread();
+  pid_t parent_pid = self->invalidate_cached_pid();
+
+  // Actually do the clone.
+  int clone_result = __bionic_clone(flags, child_stack, parent_tid, new_tls, child_tid, fn, arg);
+
+  // 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);
+  return clone_result;
 }

tests/unistd_test.cpp

diff --git a/tests/unistd_test.cpp b/tests/unistd_test.cpp
index 95e63b3..58c9ad9 100644
--- a/tests/unistd_test.cpp
+++ b/tests/unistd_test.cpp
@@ -381,6 +381,14 @@
   TestFsyncFunction(fsync);
 }
 
+static void AssertGetPidCorrect() {
+  // The loop is just to make manual testing/debugging with strace easier.
+  pid_t getpid_syscall_result = syscall(__NR_getpid);
+  for (size_t i = 0; i < 128; ++i) {
+    ASSERT_EQ(getpid_syscall_result, getpid());
+  }
+}
+
 TEST(unistd, getpid_caching_and_fork) {
   pid_t parent_pid = getpid();
   ASSERT_EQ(syscall(__NR_getpid), parent_pid);
@@ -389,7 +397,7 @@
   ASSERT_NE(fork_result, -1);
   if (fork_result == 0) {
     // We're the child.
-    ASSERT_EQ(syscall(__NR_getpid), getpid());
+    AssertGetPidCorrect();
     ASSERT_EQ(parent_pid, getppid());
     _exit(123);
   } else {
@@ -403,12 +411,29 @@
   }
 }
 
-static void GetPidCachingHelperHelper() {
-  ASSERT_EQ(syscall(__NR_getpid), getpid());
+static int GetPidCachingCloneStartRoutine(void*) {
+  AssertGetPidCorrect();
+  return 123;
 }
 
-static void* GetPidCachingHelper(void*) {
-  GetPidCachingHelperHelper(); // Can't assert in a non-void function.
+TEST(unistd, 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);
+  ASSERT_NE(clone_result, -1);
+
+  ASSERT_EQ(parent_pid, getpid());
+
+  int status;
+  ASSERT_EQ(clone_result, waitpid(clone_result, &status, 0));
+  ASSERT_TRUE(WIFEXITED(status));
+  ASSERT_EQ(123, WEXITSTATUS(status));
+}
+
+static void* GetPidCachingPthreadStartRoutine(void*) {
+  AssertGetPidCorrect();
   return NULL;
 }
 
@@ -416,7 +441,7 @@
   pid_t parent_pid = getpid();
 
   pthread_t t;
-  ASSERT_EQ(0, pthread_create(&t, NULL, GetPidCachingHelper, NULL));
+  ASSERT_EQ(0, pthread_create(&t, NULL, GetPidCachingPthreadStartRoutine, NULL));
 
   ASSERT_EQ(parent_pid, getpid());