Use posix_spawn on macOS 12

[tov]

vfork is deprecated on macOS 12. This PR uses posix_spawn on macOS 12 instead. (The standard posix_spawn does insufficient error reporting for our API, but the Mac posix_spawn's errors are more informative.)

[aalekseyev]

I don't know if it makes sense in this case, especially since code has already been written, but:
we generally try to put logic on the OCaml side if we can.
In unix_spawn we couldn't do that (while staying sane) since forking interacts poorly with OCaml runtime.
I'm thinking posix_spawn doesn't share that problem, so we could move much of this code to OCaml, instead creating C bindings for the individual posix_spawnattr_* functions.

[tov]

Interesting. Yeah, I see how that could be good. On the other hand, I could extend the blocking section around posix_spawn to include all the related posix_spawn* calls, if you think it's helpful to release the runtime system for longer.

[aalekseyev]

Extending the blocking section is also a liability because you can't access the OCaml heap (or raise exceptions) from the blocking section. For that reason I'm not exactly excited about extending it. (although if some of those functions are known to be blocking/slow then that may be worth it)

[aalekseyev]

We're dupping fds in the parent now, which I think resets close-on-exec, which means that any processes spawned concurrently (through our library or not) will inherit these fds, which seems sad.

I can't think of any way to do it race-free (dup3 doesn't seem to help because it requires us to know a free fd), but at least maybe we should set close-on-exec after dup?
(reading the doc of posix_spawn, it seems it should be able to deal with close-on-exec fds?)

[aalekseyev]

Crazy idea how to do it race-free (sans error handling):

int dup_close_on_exec(int fd) {
  int new_fd = open("/", O_CLOEXEC);
  dup3(fd, new_fd, O_CLOEXEC);
  return new_fd;
}

[tov]

Hmmm. Not that crazy. What do you make of this?:

/* Same as [dup] but 1) ensures the result isn't a standard FD, 2) applies
   [flags] (via dup3) to the new FD, and 3) on error, if [culprit] is non-null,
   assigns to [*culprit] a string literal naming the function that failed.
*/
static int safe_dup(int fd, int flags, char const **culprit)
{
  int new_fd = flags? open("/", O_CLOEXEC) : dup(fd);

  if (new_fd < 0) {
    if (culprit) {
      *culprit = flags? "open" : "dup";
    }
  }

  else if (new_fd < 3) {
    int next = safe_dup(fd, flags, culprit);
    close(new_fd);
    new_fd = next;
  }

  else if (flags) {
    new_fd = dup3(fd, new_fd, flags);
    if (new_fd < 0 && culprit) {
      *culprit = "dup3";
    }
  }

  return new_fd;
}

This might be easier in OCaml, too.

[aalekseyev]

I'm not sure this function benefitted from solving every problem at the same time. I would probably still keep at least the retry loop in a separate function. Anyway, I found that F_DUPFD_CLOEXEC is a thing (a flag you can pass to fcntl). Maybe that just works, and we don't need the ugly open/dup3 trick?

[aalekseyev]

Overall (other than inline comments):

• I think this cpp file is growing a bit too large. If you can think of a way to split it that'd be great.
• I don't understand your comment about error handling: how can posix_spawn possibly return good/better error messages if the only thing it returns is an int?
• I'm curious about performance. Have you tried measuring it against a fork-based version?

[aalekseyev]

By the way, does this function just work in linux? (at least when reviewing it I saw nothing that isn't documented in linux man pages)

[tov]

• I think this cpp file is growing a bit too large. If you can think of a way to split it that'd be great.

Yeah, I can do that.

• I don't understand your comment about error handling: how can posix_spawn possibly return good/better error messages if the only thing it returns is an int?

Linux's posix_spawn sets errno to EINVAL for all errors, whereas mac OS posix_spawn can also set errno to E2BIG, EACCES, EFAULT, etc., depending on what goes wrong.

• I'm curious about performance. Have you tried measuring it against a fork-based version?

Not yet, no.

By the way, does this function just work in linux? (at least when reviewing it I saw nothing that isn't documented in linux man pages)

Linux posix_spawn doesn't have posix_spawn_file_actions_addchdir_np/posix_spawn_file_actions_addfchdir_np.

[tov]

I'm also wondering about user-visible API questions. The current code in this PR doesn't respect the unix_backend argument when posix_spawn is enabled. Presumably, client code should be able to choose between the fork, vfork, and posix_spawn backends where those are supported. But on Linux, whether posix_spawn is supported depends on whether a non-inherited cwd argument is given to Spawn.spawn. So, we could:

1. Add Posix_spawn as a third Unix backend, and default to it on newer mac OS when SPAWN_USE_FORK isn't set.
2. Continue ignoring the unix_backend argument on Windows.
3. When Posix_spawn is passed as the backend on Linux, and cwd is not Inherit, either:
   • ignore the requested backend and fall back to vfork (consistent with Windows behavior), or
   • return ENOSYS.

I was trying to keep this API as simple as a conservative extension as possible, because without knowing about use cases, it's hard to think about how else it might be used.

[tov]

It looks like posix_spawn is kinda broken on Linux (my uid box), in that it 1) isn't checking whether the program exists before forking, and 2) doesn't seem to be setting the child's sigmask. So posix_spawn may not be suitable on (some) Linux for implementing this API.

[aalekseyev]

Oh well, it's unfortunate that it doesn't work well on Linux. I think we just shouldn't enable posix_spawn on linux at all, then. (if we add a new backend, it should probably either always fail on Linux or fallback to vfork (either is acceptable, I think))

[tov]

Yeah, I just discovered F_DUPFD_CLOEXEC as well, because MacOS doesn't have dup3. And apparently fcntl(F_DUPFD_CLOEXEC, ...) is better anyway, assuming we don't need to support Linux < 2.6. J

…

On Fri, Jul 29, 2022 at 12:19 PM Arseniy Alekseyev ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/spawn_stubs.c <#47 (comment)>: > + e_arg = Field(v_cwd, 0); + goto cleanup; + } + break; + case FD: + e_error = posix_spawn_file_actions_addfchdir_np(&actions, info.cwd.fd); + if (e_error) { + e_function = "fchdir"; + e_arg = Field(v_cwd, 0); + goto cleanup; + } + break; + } + + for (int fd = 0; fd < 3; fd++) { + int tmp_fd = tmp_fds[fd] = safe_dup(info.std_fds[fd]); I'm not sure this function benefitted from solving every problem at the same time. I would probably still keep at least the retry loop in a separate function. Anyway, I found that F_DUPFD_CLOEXEC is a thing (a flag you can pass to fcntl). Maybe that just works, and we don't need the ugly open/dup3 trick? — Reply to this email directly, view it on GitHub <#47 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAFS3BHNUGXIK7F4VXXQGP3VWQABRANCNFSM54WVD6ZA> . You are receiving this because you authored the thread.Message ID: ***@***.***>

-- Jesse Tov he/him/his Jane Street VES, 4th Floor, 4D3A6 New York, NY 10281 tel: +1 (646) 908-5576

[rgrinberg]

It looks like posix_spawn is kinda broken on Linux (my uid box), in that it 1) isn't checking whether the program exists before forking, and 2) doesn't seem to be setting the child's sigmask. So posix_spawn may not be suitable on (some) Linux for implementing this API.

It's also broken on macos :)

rust-lang/rust#80537

[rgrinberg]

On mac, there seems to be a workaround for the bug around doing chdir with the posix_spawn api libuv/libuv#3257

We could use pthread_chdir_np for a thread local chdir before spawning