commit 598f4b3ac1be3c6ef3af613ac4b0694841e165fe
author Elliott Hughes <enh@google.com> Tue Jun 24 16:36:31 2014 +0000
committer Gerrit Code Review <noreply-gerritcodereview@google.com> Tue Jun 24 07:45:29 2014 +0000
tree e6241ec4877e02fedd707ae0a2300e5203219b0c
parent 63fa660b52f1e9717f0a2105d4861a4b48c330b1
parent fa9e16efaf0e885f6044e725eb759ef6de10f7ef

Merge "Fix getpid caching across a clone."

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());