Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unix.{system,open_process_{in,out,full}} are not threadsafe #5266

Closed
vicuna opened this issue May 16, 2011 · 9 comments
Closed

Unix.{system,open_process_{in,out,full}} are not threadsafe #5266

vicuna opened this issue May 16, 2011 · 9 comments

Comments

@vicuna
Copy link

vicuna commented May 16, 2011

Original bug ID: 5266
Reporter: till
Status: acknowledged (set by @damiendoligez on 2011-05-17T12:16:32Z)
Resolution: open
Priority: normal
Severity: feature
Target version: later
Category: otherlibs
Tags: patch
Monitored by: mehdi

Bug description

When calling fork in a multi-threaded program the forked side can only call async-signal-safe functions [http://pubs.opengroup.org/onlinepubs/009695399/functions/fork.html] this means that we probably cannot rely at all on using the ocaml runtime on the forked side.

Even if we can use the ocaml runtime (because we do no allocation and therefor are compiled down to fairly straghtforward c calls) we shouldn't call the c [exit] function on errors but rather the [_exit] function which doesn't call the [at_exit] registered handler (similarly we do not want to run the ones registered in ocaml). These are not necessarily async-signal-safe and might perform flushing/cleaning up that only makes sense in the parent process (e.g.: cleaning up temporary files).

File attachments

@vicuna
Copy link
Author

vicuna commented May 16, 2011

Comment author: till

Attached a file that implements a thread safe fork-exec.

The signature of the function is:

external fork_exec :
stdin : file_descr
-> stdout : file_descr
-> stderr : file_descr
-> ?working_dir : string
-> ?env : (string) array
-> string
-> string array
-> int
= "extended_ml_create_process_bc" "extended_ml_create_process"

I'd be happy to clean up this file and make it a patch to the ocaml Unix library if you want.

@vicuna
Copy link
Author

vicuna commented Aug 5, 2012

Comment author: gmelquiond

I have been experimenting with your code since it has some nice properties that the original implementation of create_process lacks. In particular, it has proper error handling (e.g. calling _exit in case of error) and sends back to the parent process the errno value. It also avoids a rare race condition that can bring the system to its knees, since it does not allow the parent process to perform a major collection before the child process has finished discarding the obsolete address space (at least for single-threaded programs). And finally, it supports setting the working directory, as does the system call on Windows. Thanks a lot.

I have encountered a few issues though, that I will list here in case someone else stumbles upon your code:

  • closing pfd[1] must be performed before reading the pipe, otherwise the parent process will deadlock,
  • the pipe descriptors must be moved above fd 3 before forking, otherwise they might get overwritten by the standard descriptors, hence breaking error reporting,
  • clearing cloexec on the duplicated file descriptors is useless, since duplication does not preserve this attribute,
  • however, cloexec should be set on the original file descriptors (either that or they should be manually erased), otherwise the child process will possibly end up with up to 6 file descriptors instead of 3 only.

@vicuna
Copy link
Author

vicuna commented Sep 19, 2012

Comment author: till

Good points. This code was surprisingly hard to write and I am sad to see that I still let mistakes slip through (especially the fd leak). I will try to submit a fix soon.

Coming back to the pfd issue: can you think of a way that would make the parent deadlock? pdf[0] being set cloexec should be closed when the forked-side fails (and thus exits) or succeed (and exec's)

@vicuna
Copy link
Author

vicuna commented Sep 19, 2012

Comment author: gmelquiond

It has been some time, so my memories are a bit hazy. The read on pfd[0] will abort only if all the write ends of the pipe are closed, which is not the case if pfd[1] is still open in the parent, hence the deadlock. Am I missing something? Anyway, I have uploaded the variant of your code I have been using successfully.

@vicuna
Copy link
Author

vicuna commented Sep 19, 2012

Comment author: till

Oh. Palm/forehead! that is definitely a bug. I must have uploaded that file before fixing it. There's no way this could work. Thanks!

@vicuna
Copy link
Author

vicuna commented Dec 7, 2016

Comment author: @mshinwell

FWIW, https://github.com/janestreet/shexp contains a process creation library that shouldn't suffer from async signal safety problems. (I think there may be an update to that library soon.)

@github-actions
Copy link

This issue has been open one year with no activity. Consequently, it is being marked with the "stale" label. What this means is that the issue will be automatically closed in 30 days unless more comments are added or the "stale" label is removed. Comments that provide new information on the issue are especially welcome: is it still reproducible? did it appear in other contexts? how critical is it? etc.

@github-actions github-actions bot added the Stale label May 15, 2020
@xavierleroy
Copy link
Contributor

Multicore OCaml will probably need to restrict uses of Unix.fork and require a new implementation of Unix.create_process. I think that this new implementation should use posix_spawn when available.

@xavierleroy xavierleroy self-assigned this May 15, 2020
@gasche
Copy link
Member

gasche commented Jun 24, 2020

Those Unix process-creation functions where reimplemented by @xavierleroy using posix_spawn in #9573, which should be part of OCaml 4.12.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants