RFR: JDK-8262955: Unify os::fork_and_exec() across Posix platforms

[David Holmes]

Hi Thomas,

On 5/03/2021 4:22 pm, Thomas Stuefe wrote:
> `os::fork_and_exec()` can be used from within the hotspot to start a child process. It is only called in fatal situations, in two cases:
> a) to automatically start a debugger when ShowMessageBoxOnError is specified (uses *fork*())
> b) to start a caller provided binary on OOM if -XX:OnOutOfMemoryError is specified (uses *vfork*())
> 
> The variants for AIX, Linux, Bsd are almost completely identical. So, this function can be unified under posix.
> 
> In addition to that, this patch does a number of small changes:
> 
> 1) Before, whether we would vfork() only on Linux and only for case (b). I changed this to always use vfork unconditionally, on all platforms, because:
>    - even though vfork() can be unsafe, the way we use it - calling  vfork()->exec()->_exit() with no intermediate steps - is safe.

Is it? The reason we use fork() for the error/crash case is because it 
can get launched from a signal handling context and vfork is not 
async-signal-safe.

There is some commentary in:

https://bugs.openjdk.java.net/browse/JDK-8027434

Cheers,
David

>    - Using vfork is good for OOM situations on all platforms, not just Linux, and also for starting the debugger in non-OOM cases. Keep in mind that we do this only for cases where the parent VM is about to die, so even if it were unsafe, the damage would be limited.
> 2) I added a comment to the function to not use it outside of fatal error situations.
> 3) I added a posix wrapper for getting the environ pointer, to hide MacOS specifics, and used it in two places to unify that coding.
> 4) consistently used global scope :: for posix APIs.
> 
> Note that if we wanted to make os::fork_and_exec() a first class function, always safe to use, we should modify it to at least not leak any parent process file descriptors. Possibly safest would be to completely rewrite this function and use posix_spawn(). posix_spawn() we use in Runtime.exec() by default since JDK 13 (1). But as long as this is spawned by only dying VMs I think this function is fine.
> 
> ----
> 
> Tests: GAs, manual tests using -XX:ShowMessageBoxOnError
> 
> -------------
> 
> Commit messages:
>   - start
> 
> Changes: https://git.openjdk.java.net/jdk/pull/2810/files
>   Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2810&range=00
>    Issue: https://bugs.openjdk.java.net/browse/JDK-8262955
>    Stats: 293 lines in 8 files changed: 85 ins; 202 del; 6 mod
>    Patch: https://git.openjdk.java.net/jdk/pull/2810.diff
>    Fetch: git fetch https://git.openjdk.java.net/jdk pull/2810/head:pull/2810
> 
> PR: https://git.openjdk.java.net/jdk/pull/2810
>