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

[cryptoad]

Summary

In the current state of things, fork'ing a process while the allocator
is disabled yields to a child that can't use allocation primitives
until the allocator has been enabled again.

While this appears like a sound behavior to me, it broke some Android
expectations.

With this patch we change the behavior to unlocking the allocator in
a fork child via a pthread_atfork handler that is installed in
malloc_disable to fit Android's expectations.

[cryptoad]

The wrong version was uploaded.

[hctim]

compiler-rt/lib/scudo/standalone/tests/wrappers_c_test.cpp

314

Maybe add a test with a fork() where malloc isn't disabled?

compiler-rt/lib/scudo/standalone/wrappers_c.inc

158

instead of unconditionally calling malloc_enable on fork, can you guard it behind an if Disabled? Otherwise you call unlock() on an already-unlocked mutex in the allocators.

165

Just to clarify, this is non-racy because malloc_enable and malloc_disable are always called in a single-threaded context? libmemunreachable I know PTRACE_INTERRUPT's the other threads, but unsure about other uses of malloc_enable.

[cryptoad]

compiler-rt/lib/scudo/standalone/tests/wrappers_c_test.cpp

314

Ack

compiler-rt/lib/scudo/standalone/wrappers_c.inc

158

I am guarding Disabled with the mutex, so it makes it easier to call malloc_enable as it does check for Disabled within the mutex.
Unless I am misunderstanding the comment.

165

I don't think this can be racey since we are protecting the blocks with the mutex. Concurrent call to enable & disable should be resolved appropriately, as well as concurrent calls to the same API since we check for Disabled at the beginning of the block.

[cryptoad]

compiler-rt/lib/scudo/standalone/tests/wrappers_c_test.cpp

324

I think I am missing an exit here.
Haven't touched fork in ages so this might be sketchy.

[hctim]

compiler-rt/lib/scudo/standalone/wrappers_c.inc

158

Yep, missed this too...

165

Oops, early morning code reviews are a bad idea. You're right.

[cryptoad]

Updating test to exercise both disable and no-disable pre-fork.
Added an _exit in fork child.

[eugenis]

Could we do this exact same thing in Android? I find this behavior (auto-enabling the allocator after fork) unexpected, and it makes the interface even more confusing then it is now.

[cryptoad]

In D72364#1810436, @eugenis wrote:

Could we do this exact same thing in Android? I find this behavior (auto-enabling the allocator after fork) unexpected, and it makes the interface even more confusing then it is now.

We discussed it with cferris@ and the two behaviors that were deemed acceptable were:

• enable in child post fork
• block fork

A test is being added with https://android-review.googlesource.com/c/platform/system/memory/libmemunreachable/+/1202475.

I went with enable in child post fork because it was straightforward but I can go the other way as well.

[eugenis]

I see. I think the other approach could be safer. Grab all the locks before fork and release them after fork. This will also allow user applications to use malloc after fork, which ex. glibc allocator supports.

compiler-rt/lib/scudo/standalone/wrappers_c.inc

165

This will deadlock is malloc_disable or malloc_enable is called concurrently with fork().
The child process will have SCUDO_PREFIX(Mutex) locked with no way to release it.

[cryptoad]

In D72364#1810596, @eugenis wrote:

I see. I think the other approach could be safer. Grab all the locks before fork and release them after fork. This will also allow user applications to use malloc after fork, which ex. glibc allocator supports.

I think the second approach is more keen to "if allocator is disabled when we call fork, wait until it's enabled", as opposed to disabling it pre-fork.

[eugenis]

In D72364#1810619, @cryptoad wrote:

In D72364#1810596, @eugenis wrote:

I see. I think the other approach could be safer. Grab all the locks before fork and release them after fork. This will also allow user applications to use malloc after fork, which ex. glibc allocator supports.

I think the second approach is more keen to "if allocator is disabled when we call fork, wait until it's enabled", as opposed to disabling it pre-fork.

Right, but it looks like it will have the same effect.
Attempting to disable an already disabled allocator will block until the allocator is enabled.
I.e. pthread_atfork(disable, enable, enable) should be sufficient.

[cryptoad]

In D72364#1810649, @eugenis wrote:

Right, but it looks like it will have the same effect.
Attempting to disable an already disabled allocator will block until the allocator is enabled.
I.e. pthread_atfork(disable, enable, enable) should be sufficient.

Ack, thanks!

[cryptoad]

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).

[cryptoad]

I realized that this is now no longer correct as the atfork handler is only installed in a disable.

[cryptoad]

Abandoning this one, this will come back in another form.

compiler-rt/lib/scudo/standalone/tests/wrappers_c_test.cpp

TEST(ScudoWrappersCTest, MallocInfo) {
   FILE *F = fmemopen(Buffer, sizeof(Buffer), "w+");
   EXPECT_NE(F, nullptr);
   errno = 0;
   EXPECT_EQ(malloc_info(0, F), 0);
   EXPECT_EQ(errno, 0);
   fclose(F);
   EXPECT_EQ(strncmp(Buffer, "<malloc version=\"scudo-", 23), 0);
 }
+
+// If a fork occurs while the allocator was disabled, we want to ensure that the
+// child can use allocation primitives without enabling the allocator first.
+TEST(ScudoWrappersCTest, MallocPostFork) {
+  pid_t Pid;
+  malloc_disable();
+  Pid = fork();
+  EXPECT_GE(Pid, 0);
+  if (Pid == 0) {
+    void *P = malloc(Size);
+    EXPECT_NE(P, nullptr);
+    free(P);
+  }
+  waitpid(Pid, nullptr, 0);
+  malloc_enable();
+  void *P = malloc(Size);
+  EXPECT_NE(P, nullptr);
+  free(P);
+}
 #endif

compiler-rt/lib/scudo/standalone/wrappers_c.inc

 
 INTERFACE WEAK int SCUDO_PREFIX(malloc_iterate)(
     uintptr_t base, size_t size,
     void (*callback)(uintptr_t base, size_t size, void *arg), void *arg) {
   SCUDO_ALLOCATOR.iterateOverChunks(base, size, callback, arg);
   return 0;
 }
 
+static pthread_once_t OnceControl = PTHREAD_ONCE_INIT;
+static scudo::atomic_u8 Disabled;
+
+// TODO(kostyak): malloc_{enable,disable} are not necessarily thread safe. But
+//                since they are used for heap debugging, we deem it good enough
+//                for now. We will revisity that later.
+
+INTERFACE WEAK void SCUDO_PREFIX(malloc_enable)() {
+  if (!scudo::atomic_load(&Disabled, scudo::memory_order_acquire))
+    return;
+  SCUDO_ALLOCATOR.enable();
+  scudo::atomic_store(&Disabled, 0U, scudo::memory_order_release);
+}
+
+// Execute malloc_enable for a fork child.
+static void setupAtFork() {
+  pthread_atfork(nullptr, nullptr, SCUDO_PREFIX(malloc_enable));
+}
+
 INTERFACE WEAK void SCUDO_PREFIX(malloc_disable)() {
+  if (scudo::atomic_load(&Disabled, scudo::memory_order_acquire))
+    return;
+  CHECK_EQ(pthread_once(&OnceControl, setupAtFork), 0);
   SCUDO_ALLOCATOR.disable();
+  scudo::atomic_store(&Disabled, 1U, scudo::memory_order_release);
 }
 
-INTERFACE WEAK void SCUDO_PREFIX(malloc_enable)() { SCUDO_ALLOCATOR.enable(); }
-
 INTERFACE WEAK int SCUDO_PREFIX(mallopt)(int param, UNUSED int value) {
   if (param == M_DECAY_TIME) {
     // TODO(kostyak): set release_to_os_interval_ms accordingly.
     return 1;
   } else if (param == M_PURGE) {
     SCUDO_ALLOCATOR.releaseToOS();
     return 1;
   }