Linux base::LaunchApp allocates memory in the cloned child via std::function ctor

RESOLVED FIXED in Firefox 63

Status

()

defect
P1
normal
RESOLVED FIXED
10 months ago
9 months ago

People

(Reporter: glandium, Assigned: jld)

Tracking

unspecified
mozilla63
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox62 wontfix, firefox63 fixed)

Details

Attachments

(1 attachment)

Reporter

Description

10 months ago
I'm not entirely sure why this hasn't been a massive cause of problems in the past 6 months. But here we are. Somehow, compiling with clang makes the problems happen *very* reliably. https://treeherder.mozilla.org/#/jobs?repo=try&revision=101fb8d5b1d692e43b6a83209b825b291702e268

The problem is that between the fake-fork and the execv in LaunchApp, the line that does CloseSuperfluousFds(shuffle.MapsToFunc()); allocates memory.

Guess what happens when another thread was in the middle of allocating memory when the clone() system call happens? An allocator lock is held. Guess what happens when the created process wants to allocate memory? It's stuck waiting for that lock.

The way this is normally handled is that the allocator is setting pthread atfork handlers to run before and after fork(), in parent and child processes, to lock everything before fork, and unlock everything after to ensure that no lock is left in the wrong state. And without that happening, well, we're doomed.

Whatever is done to fix this should probably be backported up to esr60.
CloseSuperfluousFds itself should be safe (see comments in the func and bug 622992), so I'm guessing this was the FD shuffling that was changed in bug 1456911?
Yes, this is probably bug 1456911 — there's a std::function constructor.  This should be fixable by using std::ref on a stack-allocated closure.
Assignee: nobody → jld
Priority: -- → P1
pthread_atfork not working in the fake fork is a known “feature”.  (Android's fork() had the same one, although that might not be the case anymore in versions we support.)  There was a glibc bug about exposing a nonstandard fork_with_flags but it didn't go anywhere.  I'll see about making the comments in LaunchApp more explicit about this.
Summary: Sandbox "fork" doesn't run pthread atfork handlers, leading to dead locks → Linux base::LaunchApp allocates memory in the cloned child via std::function ctor
Blocks: 1456911
No longer blocks: 1401062
I disassembled a local build and I don't see the allocation; it appears to be writing the closure contents to the stack and passing a stack-based pointer into CloseSuperfluousFds.

I'm using clang-6.0 on Debian unstable; addr2line resolves that part of the code to header files from the libstdc++-8-dev package, so I tried again with libc++, but still no allocation.  Maybe some older Clang / LLVM / stdlib doesn't optimize this?

In any case, it's still a bug; this instance of the constructor may allocate.  The instance for std::reference_wrapper must not allocate (https://en.cppreference.com/w/cpp/utility/functional/function/function, and [func.wrap.func.con] in the standard at least requires that it can't throw bad_alloc).
Component: Security: Process Sandboxing → IPC
Reporter

Comment 5

10 months ago
pthread_atfork didn't exist in older versions of Android, but we've had our own for a long while in mozglue (and we wrap fork).

While it may be possible to get away with this right now by ensuring we don't allocate between fork and exec, further down the road, project fission is going to require content processes to be fork()ed and not executed, and we'll have to address the fact that atfork handlers have to run somehow.
Reporter

Comment 6

10 months ago
(In reply to Jed Davis [:jld] (⏰UTC-6) from comment #4)
> I disassembled a local build and I don't see the allocation; it appears to
> be writing the closure contents to the stack and passing a stack-based
> pointer into CloseSuperfluousFds.
> 
> I'm using clang-6.0 on Debian unstable; addr2line resolves that part of the
> code to header files from the libstdc++-8-dev package, so I tried again with
> libc++, but still no allocation.  Maybe some older Clang / LLVM / stdlib
> doesn't optimize this?

The disassembly for the try build linked previously has a call to moz_xmalloc in LaunchApp. And so do, by the way, nightlies.
Reporter

Comment 7

10 months ago
Ah, actually, the moz_xmalloc is before the fork on nightlies. And there's an additional one between the fork and the execve in the clang build.
Reporter

Updated

10 months ago
Blocks: 1480631
The fork server will be single-threaded, so it should be safe from anything that the atfork handlers were trying to prevent.  (We may need to interpose pthread_create and/or filter known problems out of LD_PRELOAD, but it *will* be single-threaded.)

It doesn't seem too hard to interpose pthread_atfork and emulate it if we really need to, but that's beyond the scope of this bug.


(Also, on the topic of getting away with things, touching pthread mutexes in a pthread_atfork child handler invokes undefined behavior: http://pubs.opengroup.org/onlinepubs/9699919799/functions/pthread_atfork.html#tag_16_402_07 )

Comment 10

10 months ago
mozreview-review
Comment on attachment 8997257 [details]
Bug 1480401 - Avoid heap-allocated closures in async signal safe part of LaunchApp.

https://reviewboard.mozilla.org/r/261122/#review268230

r=me, I guess, but I don't know about the below...

::: ipc/chromium/src/base/process_util_linux.cc:61
(Diff revision 1)
> +    // Constructing a std::function from a reference_wrapper won't allocate.
> +    CloseSuperfluousFds(std::ref(fdIsUsed));

That is, uh, quite an assumption.  Is that justified by the standard?
Attachment #8997257 - Flags: review?(nfroyd) → review+
(In reply to Nathan Froyd [:froydnj] from comment #10)
> Comment on attachment 8997257 [details]
> Bug 1480401 - Avoid heap-allocated closures in async signal safe part of
> LaunchApp.
> 
> https://reviewboard.mozilla.org/r/261122/#review268230
> 
> r=me, I guess, but I don't know about the below...
> 
> ::: ipc/chromium/src/base/process_util_linux.cc:61
> (Diff revision 1)
> > +    // Constructing a std::function from a reference_wrapper won't allocate.
> > +    CloseSuperfluousFds(std::ref(fdIsUsed));
> 
> That is, uh, quite an assumption.  Is that justified by the standard?

See comment #4; the standard doesn't seem to want to specify things like “won't call new/malloc”, understandably, but it does say that it can't throw bad_alloc.  (For values of “the standard” that are drafts before and after C++11, because the actual standards are paywalled.)

If that's not convincing enough I can convert this to C-style function pointers and casting from void*, but if I don't have to I'd rather not.
Is my justification in comment #11 convincing enough to land this?
Flags: needinfo?(nfroyd)
Yes, that seems fine.  Thanks for the explanation!
Flags: needinfo?(nfroyd)

Comment 14

10 months ago
Pushed by jedavis@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1e65391c7583
Avoid heap-allocated closures in async signal safe part of LaunchApp. r=froydnj

Comment 15

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1e65391c7583
Status: NEW → RESOLVED
Last Resolved: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Reporter

Comment 16

10 months ago
The large amount of failure that happened on landing of bug 1480631 suggests this is either not fixed or something else added some allocations between fake-fork and exec.
See Also: → 1481978
Is this something you'd consider uplifting to 62 beta or is the fix not solid yet?
Flags: needinfo?(jld)
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #17)
> Is this something you'd consider uplifting to 62 beta or is the fix not
> solid yet?

It's not too useful without the patch for bug 1481978, which just landed.  (There were two instances of the same mistake.)

As for uplifting them both: I could try, but it doesn't appear to affect Mozilla's own builds (it depends on the C++ toolchain, and both glandium (comment #7) and I have disassembled Nightlies before this landed to confirm that they weren't affected), and I suspect that “it may affect some downstream Linux distributions” wouldn't be a strong enough justification this close to release.
Flags: needinfo?(jld)
You need to log in before you can comment on or make changes to this bug.