Page MenuHomePhabricator

[compiler-rt] Replace forkpty with posix_spawn

Authored by kubamracek on Nov 14 2017, 9:06 AM.



On Darwin, we currently use forkpty to communicate with the "atos" symbolizer. There are several problems that fork or forkpty 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. Since posix_spawn doesn't fork (at least on Darwin), the interceptors are not a problem. Additionally, this also fixes a latent threading problem with ptsname (it's unsafe to use this function in multithreaded programs). Yet another benefit is that we'll handle post-fork failures (e.g. sandbox disallows "exec") gracefully now.

Diff Detail

Event Timeline

kubamracek created this revision.Nov 14 2017, 9:06 AM
dvyukov edited edge metadata.Nov 15 2017, 12:00 AM

I can't say that I fully understand what happens here, but I can rubber stamp LGTM if you wish.

FWIW for linux we have a bunch of checks if we are in symbolizer in interceptors, and do something different if we are.

229 ↗(On Diff #122853)

Don't we want to use internal_open/internal_close here? open/close are intercepted.

vitalybuka resigned from this revision.Dec 5 2017, 4:14 PM
kubamracek updated this revision to Diff 133308.Feb 7 2018, 2:27 PM

Updating patch.

kubamracek set the repository for this revision to rCRT Compiler Runtime.
kubamracek added a subscriber: dcoughlin.
Herald added a subscriber: Restricted Project. · View Herald TranscriptFeb 7 2018, 2:27 PM
kubamracek added inline comments.Feb 7 2018, 2:28 PM
229 ↗(On Diff #122853)

Updated patch. Although on Darwin, there's no difference, plain calls to open/close call the original functions.

dberris accepted this revision.Feb 7 2018, 3:08 PM

LGTM -- nothing looks obviously broken to me here, though you probably want to get one more person to LGTM.

This revision is now accepted and ready to land.Feb 7 2018, 3:08 PM

@george.karpenkov would you mind taking a look?

george.karpenkov accepted this revision.Feb 7 2018, 4:36 PM

LGTM with a few nits, but I'm not 100% sure what's going on either..

216 ↗(On Diff #133308)

I would appreciate a comment on why do you need a pseudo-terminal here.

217 ↗(On Diff #133308)

Is kInvalidFd equal to -1 ?

261 ↗(On Diff #133308)

could the error happen after the file descriptor was set? (e.g. system call fails, but the pointer was still written into?) Might be safer to have multiple cleanup labels to jump to the one where you know it's safe to close the descriptor?

Updated patch.

261 ↗(On Diff #133308)

I don't think this can happen. Both master_fd and slave_fd are only assigned directly in the code above, there's no pointers involved. I think it's pretty safe to say that they're either kInvalidFd or valid fds that need to be closed.

This revision was automatically updated to reflect the committed changes.
kubamracek reopened this revision.Feb 11 2018, 12:47 PM
This revision is now accepted and ready to land.Feb 11 2018, 12:47 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptOct 7 2019, 4:48 AM