[PATCH] D72364: [scudo][standalone] Modify malloc_{enable,disable} wrt fork

[Kostya Kortchinsky via Phabricator via llvm-commits]

cryptoad updated this revision to Diff 236894.
cryptoad added a comment.

Submitting this new design for consideration. I am working on adding
some tests but at least the code idea can get some feedback.

The point now is to do, as suggested by eugenis@ something like:
`atwork(disable, enable, enable)`

With the previous mutexes, a disable/fork/enable from other thread
would have deadlocked due to the mutex being held by disable in fork
so enable wouldn't fire.

I changed that to use a pthread conditional variable loop which should
allow to not deadlock in this situation (I am working on a test that
would catch that).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72364/new/

https://reviews.llvm.org/D72364

Files:
  compiler-rt/lib/scudo/standalone/tests/wrappers_c_test.cpp
  compiler-rt/lib/scudo/standalone/wrappers_c.inc


Index: compiler-rt/lib/scudo/standalone/wrappers_c.inc
===================================================================
--- compiler-rt/lib/scudo/standalone/wrappers_c.inc
+++ compiler-rt/lib/scudo/standalone/wrappers_c.inc
@@ -141,11 +141,37 @@
   return 0;
 }
 
+static pthread_once_t SCUDO_PREFIX(OnceControl) = PTHREAD_ONCE_INIT;
+static pthread_mutex_t SCUDO_PREFIX(Mutex) = PTHREAD_MUTEX_INITIALIZER;
+static pthread_cond_t SCUDO_PREFIX(Conditional) = PTHREAD_COND_INITIALIZER;
+static bool SCUDO_PREFIX(Disabled) = false;
+
+INTERFACE WEAK void SCUDO_PREFIX(malloc_enable)() {
+  pthread_mutex_lock(&SCUDO_PREFIX(Mutex));
+  SCUDO_ALLOCATOR.enable();
+  SCUDO_PREFIX(Disabled) = false;
+  pthread_cond_broadcast(&SCUDO_PREFIX(Conditional));
+  pthread_mutex_unlock(&SCUDO_PREFIX(Mutex));
+}
+
+static void SCUDO_PREFIX(setupAtFork)();
+
 INTERFACE WEAK void SCUDO_PREFIX(malloc_disable)() {
+  pthread_mutex_lock(&SCUDO_PREFIX(Mutex));
+  while (SCUDO_PREFIX(Disabled))
+    pthread_cond_wait(&SCUDO_PREFIX(Conditional), &SCUDO_PREFIX(Mutex));
+  CHECK_EQ(pthread_once(&SCUDO_PREFIX(OnceControl), SCUDO_PREFIX(setupAtFork)),
+           0);
   SCUDO_ALLOCATOR.disable();
+  SCUDO_PREFIX(Disabled) = true;
+  pthread_mutex_unlock(&SCUDO_PREFIX(Mutex));
 }
 
-INTERFACE WEAK void SCUDO_PREFIX(malloc_enable)() { SCUDO_ALLOCATOR.enable(); }
+// Execute malloc_enable for a fork child.
+static void SCUDO_PREFIX(setupAtFork)() {
+  pthread_atfork(SCUDO_PREFIX(malloc_disable), SCUDO_PREFIX(malloc_enable),
+                 SCUDO_PREFIX(malloc_enable));
+}
 
 INTERFACE WEAK int SCUDO_PREFIX(mallopt)(int param, UNUSED int value) {
   if (param == M_DECAY_TIME) {
Index: compiler-rt/lib/scudo/standalone/tests/wrappers_c_test.cpp
===================================================================
--- compiler-rt/lib/scudo/standalone/tests/wrappers_c_test.cpp
+++ compiler-rt/lib/scudo/standalone/tests/wrappers_c_test.cpp
@@ -310,4 +310,32 @@
   fclose(F);
   EXPECT_EQ(strncmp(Buffer, "<malloc version=\"scudo-", 23), 0);
 }
+
+TEST(ScudoWrappersCTest, MallocFork) {
+  void *P;
+  pid_t Pid = fork();
+  EXPECT_GE(Pid, 0);
+  if (Pid == 0) {
+    P = malloc(Size);
+    EXPECT_NE(P, nullptr);
+    memset(P, 0x42, Size);
+    free(P);
+    _exit(0);
+  }
+  waitpid(Pid, nullptr, 0);
+  P = malloc(Size);
+  EXPECT_NE(P, nullptr);
+  memset(P, 0x42, Size);
+  free(P);
+
+  // fork should stall if the allocator has been disabled.
+  EXPECT_DEATH(
+      {
+        malloc_disable();
+        alarm(1);
+        Pid = fork();
+        EXPECT_GE(Pid, 0);
+      },
+      "");
+}
 #endif


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D72364.236894.patch
Type: text/x-patch
Size: 2597 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20200108/a2f3874d/attachment.bin>