Page MenuHomePhabricator

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

Authored by cryptoad on Jan 7 2020, 2:40 PM.

Details

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.

Event Timeline

cryptoad created this revision.Jan 7 2020, 2:40 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJan 7 2020, 2:40 PM
Herald added subscribers: Restricted Project, jfb. · View Herald Transcript
cryptoad updated this revision to Diff 236693.Jan 7 2020, 2:42 PM

The wrong version was uploaded.

Harbormaster completed remote builds in B43466: Diff 236693.
hctim added inline comments.Jan 7 2020, 3:23 PM
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.

161

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 added inline comments.Jan 7 2020, 3:50 PM
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.

161

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 added inline comments.Jan 7 2020, 3:56 PM
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 added inline comments.Jan 7 2020, 4:05 PM
compiler-rt/lib/scudo/standalone/wrappers_c.inc
158

Yep, missed this too...

161

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

cryptoad updated this revision to Diff 236834.Jan 8 2020, 8:20 AM
cryptoad marked 9 inline comments as done.

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

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.

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.

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
161

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.

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.

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.

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 updated this revision to Diff 236894.Jan 8 2020, 1:00 PM

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 planned changes to this revision.Jan 8 2020, 1:30 PM

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

cryptoad abandoned this revision.Jan 9 2020, 9:02 AM

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