From bba80dcd80709ad6e4c4cc8097fa72acc961c413 Mon Sep 17 00:00:00 2001
From: Mitch Phillips <mitchp@google.com>
Date: Tue, 11 Feb 2020 14:42:14 -0800
Subject: [PATCH] [GWP-ASan] Fix non-reentrant libc_globals init behaviour.

The WriteProtected mutator for __libc_globals isn't reentrant.
Previously we were calling __libc_globals.mutate() inside of GWP-ASan's
libc initialisation, which is called inside the __libc_globals.mutate().
This causes problems with malloc_debug and other malloc shims, as they
fail to install when GWP-ASan is sampling their processes.

Bug: 135634846
Test: atest bionic
Change-Id: Iae51faa8d78677eeab6204b6ab4f3ae1b7517ba5
---
 libc/bionic/gwp_asan_wrappers.cpp     | 24 +++++++++++-------------
 libc/bionic/gwp_asan_wrappers.h       |  7 ++++---
 libc/bionic/malloc_common.cpp         |  4 +++-
 libc/bionic/malloc_common_dynamic.cpp |  6 ++++--
 4 files changed, 22 insertions(+), 19 deletions(-)

diff --git a/libc/bionic/gwp_asan_wrappers.cpp b/libc/bionic/gwp_asan_wrappers.cpp
index 680c5e7a9..1feb871b2 100644
--- a/libc/bionic/gwp_asan_wrappers.cpp
+++ b/libc/bionic/gwp_asan_wrappers.cpp
@@ -195,7 +195,7 @@ bool ShouldGwpAsanSampleProcess() {
   return false;
 }
 
-bool MaybeInitGwpAsanFromLibc() {
+bool MaybeInitGwpAsanFromLibc(libc_globals* globals) {
   // Never initialize the Zygote here. A Zygote chosen for sampling would also
   // have all of its children sampled. Instead, the Zygote child will choose
   // whether it samples or not just after the Zygote forks. For
@@ -205,14 +205,14 @@ bool MaybeInitGwpAsanFromLibc() {
   if (progname && strncmp(progname, "app_process", 11) == 0) {
     return false;
   }
-  return MaybeInitGwpAsan(false);
+  return MaybeInitGwpAsan(globals);
 }
 
 static bool GwpAsanInitialized = false;
 
 // Maybe initializes GWP-ASan. Called by android_mallopt() and libc's
 // initialisation. This should always be called in a single-threaded context.
-bool MaybeInitGwpAsan(bool force_init) {
+bool MaybeInitGwpAsan(libc_globals* globals, bool force_init) {
   if (GwpAsanInitialized) {
     error_log("GWP-ASan was already initialized for this process.");
     return false;
@@ -239,17 +239,15 @@ bool MaybeInitGwpAsan(bool force_init) {
 
   // GWP-ASan's initialization is always called in a single-threaded context, so
   // we can initialize lock-free.
-  __libc_globals.mutate([](libc_globals* globals) {
-    // Set GWP-ASan as the malloc dispatch table.
-    globals->malloc_dispatch_table = gwp_asan_dispatch;
-    atomic_store(&globals->default_dispatch_table, &gwp_asan_dispatch);
+  // Set GWP-ASan as the malloc dispatch table.
+  globals->malloc_dispatch_table = gwp_asan_dispatch;
+  atomic_store(&globals->default_dispatch_table, &gwp_asan_dispatch);
 
-    // If malloc_limit isn't installed, we can skip the default_dispatch_table
-    // lookup.
-    if (GetDispatchTable() == nullptr) {
-      atomic_store(&globals->current_dispatch_table, &gwp_asan_dispatch);
-    }
-  });
+  // If malloc_limit isn't installed, we can skip the default_dispatch_table
+  // lookup.
+  if (GetDispatchTable() == nullptr) {
+    atomic_store(&globals->current_dispatch_table, &gwp_asan_dispatch);
+  }
 
 #ifndef LIBC_STATIC
   SetGlobalFunctions(gwp_asan_gfunctions);
diff --git a/libc/bionic/gwp_asan_wrappers.h b/libc/bionic/gwp_asan_wrappers.h
index 9bbc593f1..fd9c547f1 100644
--- a/libc/bionic/gwp_asan_wrappers.h
+++ b/libc/bionic/gwp_asan_wrappers.h
@@ -28,11 +28,12 @@
 
 #pragma once
 
-#include <stddef.h>
+#include <private/bionic_globals.h>
 #include <private/bionic_malloc_dispatch.h>
+#include <stddef.h>
 
 // Hooks for libc to possibly install GWP-ASan.
-bool MaybeInitGwpAsanFromLibc();
+bool MaybeInitGwpAsanFromLibc(libc_globals* globals);
 
 // Maybe initialize GWP-ASan. Set force_init to true to bypass process sampling.
-bool MaybeInitGwpAsan(bool force_init = false);
+bool MaybeInitGwpAsan(libc_globals* globals, bool force_init = false);
diff --git a/libc/bionic/malloc_common.cpp b/libc/bionic/malloc_common.cpp
index da68c80ce..ed5537f5f 100644
--- a/libc/bionic/malloc_common.cpp
+++ b/libc/bionic/malloc_common.cpp
@@ -322,7 +322,9 @@ extern "C" bool android_mallopt(int opcode, void* arg, size_t arg_size) {
       errno = EINVAL;
       return false;
     }
-    return MaybeInitGwpAsan(*reinterpret_cast<bool*>(arg));
+    __libc_globals.mutate([&](libc_globals* globals) {
+      return MaybeInitGwpAsan(globals, *reinterpret_cast<bool*>(arg));
+    });
   }
   errno = ENOTSUP;
   return false;
diff --git a/libc/bionic/malloc_common_dynamic.cpp b/libc/bionic/malloc_common_dynamic.cpp
index 79d25212e..ad28cb37d 100644
--- a/libc/bionic/malloc_common_dynamic.cpp
+++ b/libc/bionic/malloc_common_dynamic.cpp
@@ -379,7 +379,7 @@ static void MallocInitImpl(libc_globals* globals) {
   char prop[PROP_VALUE_MAX];
   char* options = prop;
 
-  MaybeInitGwpAsanFromLibc();
+  MaybeInitGwpAsanFromLibc(globals);
 
   // Prefer malloc debug since it existed first and is a more complete
   // malloc interceptor than the hooks.
@@ -529,7 +529,9 @@ extern "C" bool android_mallopt(int opcode, void* arg, size_t arg_size) {
       errno = EINVAL;
       return false;
     }
-    return MaybeInitGwpAsan(*reinterpret_cast<bool*>(arg));
+    __libc_globals.mutate([&](libc_globals* globals) {
+      return MaybeInitGwpAsan(globals, *reinterpret_cast<bool*>(arg));
+    });
   }
   // Try heapprofd's mallopt, as it handles options not covered here.
   return HeapprofdMallopt(opcode, arg, arg_size);