On Sat, 27 Feb 2021 18:20:52 GMT, Ioi Lam <ik...@openjdk.org> wrote: >> Hi Yumin, >> >> This is a very useful addition. >> >> My biggest concern with this patch though is the use of >> `os::fork_and_exec()` in regular, non-fatal situations. I had a look at that >> function and I think that is very unsafe. >> >> - it does not close file descriptors, so the child vm will inherit all file >> descriptors not opened with FD_CLOEXEC. In Runtime.exec, we do this whole >> dance around safely closing off all unused file descriptors, which is >> missing here. Note that even though we mostly open all fds with CLOEXEC, >> this does not matter, since user code may not do that. >> - It uses vfork "if available", which is probably always, but that may be >> okay since the child exec's right away. Still, vfork makes me nervous. >> - Weirdly enough, it always spawns the child program via one indirection >> using a shell; so there is always the shell between you and your spawned VM, >> and you probably wont get the return code of your child vm process back. >> >> Until now, os::fork_and_exec() was only used in fatal situations where the >> VM was about to die anyway. And where it did not really matter if it worked >> or not. >> >> If we now want to use it in regular situations, we need to give that thing >> an overhaul and make it a first class fork api, and also test it better that >> it is today. The file descriptor issue has to be addressed at the very least. >> >> I really would consider rewriting the whole thing using posix_spawn. Since >> JDK15 I think posix_spawn is the default for Runtime.exec, so we know it >> works well. I would also do this in a separate RFE. >> >> Alternatively, you could call into java and use Runtime.exec(). >> >> Further remarks inline. >> >> Thanks, Thomas > >> I really would consider rewriting the whole thing using posix_spawn. Since >> JDK15 I think posix_spawn is the default for Runtime.exec, so we know it >> works well. I would also do this in a separate RFE. >> >> Alternatively, you could call into java and use Runtime.exec(). > > I think we should call into Java and use `Runtime.exec()`. Running a > subprocess is very complicated, so we shouldn't try to duplicate the code in > the VM.
@iklam @calvinccheung @Hamlin-Li Thanks for review! ------------- PR: https://git.openjdk.java.net/jdk/pull/2737
