Page MenuHomePhabricator

[sanitizer_common] Replace forkpty with posix_spawn on Darwin
ClosedPublic

Authored by yln on Jul 24 2019, 4:11 PM.

Details

Summary

On Darwin, we currently use forkpty to communicate with the "atos"
symbolizer. There are several problems that fork[pty] has, e.g. that
after fork, interceptors are still active and this sometimes causes
crashes or hangs. This is especially problematic for TSan, which uses
interceptors for OS-provided locks and mutexes, and even Libc functions
use those.

This patch replaces forkpty with posix_spawn on Darwin. Since
posix_spawn doesn't fork (at least on Darwin), the interceptors are not
a problem. Another benefit is that we'll handle post-fork failures (e.g.
sandbox disallows "exec") gracefully now.

Related revisions and previous attempts that were blocked by or had to
be revered due to test failures:
https://reviews.llvm.org/D48451
https://reviews.llvm.org/D40032

Diff Detail

Repository
rL LLVM

Event Timeline

yln created this revision.Jul 24 2019, 4:11 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJul 24 2019, 4:11 PM
Herald added subscribers: llvm-commits, Restricted Project, kubamracek. · View Herald Transcript
yln added a reviewer: alex.Jul 24 2019, 4:49 PM

Thanks! This looks like a great step in the right direction. An overall question first: Should we use posix_spawn for *all platforms*? IIRC, unifying the launch paths was discussed on mailing lists before and sounds desirable.

alex added a comment.Tue, Jul 30, 6:52 PM

I had a patch to use posix_spawn on all posix platforms, but hit some issues I didn't quite have the time to debug all the way through. It's a good idea in my view.

vitalybuka added inline comments.Fri, Aug 2, 12:21 PM
compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_mac.cc
53 ↗(On Diff #211630)

Do we have use_posix_spawn = false on mac anywhere?

yln marked an inline comment as done.Fri, Aug 2, 3:54 PM

Thanks! This looks like a great step in the right direction. An overall question first: Should we use posix_spawn for *all platforms*? IIRC, unifying the launch paths was discussed on mailing lists before and sounds desirable.

I agree that this should be the end goal. For this patch, I would prefer to keep it Darwin-only to reduce the scope, especially since multiple people already attempted this before.
Also, I wouldn't be able to debug this locally for other platforms. How about doing this as a first step (of course I want to avoid doing anything that would prevent adopting this on other platforms) and going from there?

compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_mac.cc
53 ↗(On Diff #211630)

We have LLVMSymbolizerProcess which doesn't specify it and therefore gets the default false.

kubamracek accepted this revision.Wed, Aug 14, 10:49 AM

LGTM, even if it's just for Darwin now.

I do think, though, that LLVMSymbolizerProcess should also use posix_spawn *on Darwin*. Can we get that (as a separate change)?

compiler-rt/lib/sanitizer_common/sanitizer_mac.cc
257 ↗(On Diff #211630)

There's a bunch of different reasons why we can fail to spawn. Should we Report() to log the actual failed API?

This revision is now accepted and ready to land.Wed, Aug 14, 10:49 AM
yln updated this revision to Diff 215283.Wed, Aug 14, 4:36 PM

Fixed failing test closed-fds.cpp: external symbolizer should continue to work
even when client program implements any of std{in,out,err}. Implemented similiar
functionality as CreateTwoHighNumberedPipes for internal_spawn.

This revision was automatically updated to reflect the committed changes.
yln added a comment.Thu, Aug 15, 10:29 AM

I do think, though, that LLVMSymbolizerProcess should also use posix_spawn *on Darwin*. Can we get that (as a separate change)?

Changed in:
https://github.com/llvm/llvm-project/commit/bb99437f6d45af7bbc88e07b8e4dbc1528802508