Dropping vfork support regressed performance when posix_spawn not available

[adamgundry]

#279 replaced vfork calls with fork, but fork can perform substantially worse when the process has a large memory footprint.

Ideally we would use posix_spawn instead, but (when setting the cwd) this requires posix_spawn_file_actions_addchdir. There seems to be an inconsistency as the configure script looks for posix_spawn_file_actions_addchdir but do_spawn_posix tests for the _np version. Presumably that should be fixed, though it isn't clear whether glibc has added support for posix_spawn_file_actions_addchdir (without _np) yet; perhaps we should test for both the portable and non-portable versions?

Since there may be other cases where posix_spawn cannot be used, I wonder if we should retain vfork support as well (preferring posix_spawn if possible, and falling back on fork if vfork is not available).

cc @bgamari

[bgamari]

I'm rather reluctant to reintroduce vfork support for a few reasons:

1. In practice, it is quite tricky to use correctly since you need to guarantee that the forked child does not touch any memory of the forking parent before exec'ing. While in principle runProcess is a fairly simple piece of code which can be easily audited, it calls various bits of libc which are much harder to say anything about.
2. It is easy to end up with privilege escalation issues when vfork is used in a multithreaded program which uses vfork to spawn a child with reduced privileges.
3. POSIX.1-2008 has formally removed any specification of its behavior, making it a bit hard to depend upon

Ultimately, I wouldn't necessarily be opposed to reintroducing support assuming that we explicitly disable its use when setuid or setgid are requested. This would be sufficient to side-step (1) and (2), which should make it fairly safe (assuming the semantics given to it by POSIX.1-2001).

[adamgundry]

I don't claim to have a great amount of expertise here, so I'm happy to trust your judgement. I know there are cases in which posix_spawn isn't used so we have to fall back to [v]fork, but I don't know how common those are (at least once #303 resolves the issue with posix_spawn_file_actions_addchdir). If they are sufficiently rare, perhaps it is indeed fine to stay away from vfork.

[bgamari]

The problem ultimately is that there is a large amount (complete?) overlap between the cases that posix_spawn can't handle and the cases that vfork can't (safely) handle. Consequently, it's not clear to me that we really gain much beyond maintenance burden by reintroducing vfork support.

[adamgundry]

Unfortunately I just tested this again using process-1.6.19.0, and it seems that code setting cwd still ends up using fork rather than posix_spawn. I think I see why and have a patch incoming.

Here's a test program that exhibits the issue:

import System.Process
import System.IO

mkProcess :: IO (Maybe Handle, Maybe Handle, Maybe Handle, ProcessHandle)
mkProcess =
    createProcess CreateProcess {
          cmdspec = RawCommand "/bin/true" []
        , cwd = Just "."
        , env = Just []
        , std_in  = CreatePipe
        , std_out = CreatePipe
        , std_err = CreatePipe
        , close_fds = False
        , create_group = False
        , delegate_ctlc = False
        , detach_console = False
        , create_new_console = False
        , new_session = False
        , child_group = Nothing
        , child_user = Nothing
        , use_process_jobs = False
        }

main :: IO ()
main = do
    (createdInH, createdOutH, createdErrorH, pHandle) <- mkProcess
    pure ()

Running this under strace gives

clone(child_stack=NULL, flags=CLONE_CHILD_CLEARTID|CLONE_CHILD_SETTID|SIGCHLD, child_tidptr=0x7febb63e1a10) = 740508

whereas a fixed version gives

clone3({flags=CLONE_VM|CLONE_VFORK, exit_signal=SIGCHLD, stack=0x7d84a9c4e000, stack_size=0x9000}, 88) = 747494