On Wed, 17 May 2023 16:00:23 GMT, Volker Simonis <simo...@openjdk.org> wrote:
>> Since JDK13, executing commands in a sub-process defaults to the so called
>> `POSIX_SPAWN` launching mechanism (i.e.
>> `-Djdk.lang.Process.launchMechanism=POSIX_SPAWN`) on Linux. This works by
>> using `posix_spawn(3)` to firstly start a tiny helper program called
>> `jspawnhelper` in a subprocess. In a second step, `jspawnhelper` reads the
>> command data from the parent Java process over a Unix pipe and finally
>> executes (i.e. `execvp(3)`) the requested command.
>>
>> In cases where the parent process terminates abnormally before
>> `jspawnhelper` has read all the expected data from the pipe, `jspawnhelper`
>> will block indefinitely on the reading end of the pipe. This is especially
>> harmful if the parent process had open sockets, because in that case, the
>> forked `jspawnhelper` process will inherit them and keep all the
>> corresponding ports open effectively preventing other processes to bind to
>> them. Notice that this is not an abstract scenario. We've observed this
>> regularly in production with services which couldn't be restarted after a
>> crash after migrating to JDK 17.
>>
>> The fix of the issue is rather trivial. `jspawnhelper` has to close its
>> writing end of the pipe which connects it with the parent Java process
>> *before* starting to read from that pipe such that reads from the pipe can
>> immediately return with EOF if the parent process terminates abnormally.
>>
>> Also did some cleanup:
>> - in `jspawnhelper.c` correctly chek the result of `readFully()`
>> - in `ProcessImpl_md.c` use `restartableWrite()` instead of `write()` to
>> write the command data from the parent process to `jspawnhelper`
>> - in `childproc.{c,h}` remove remaining traces of `MODE_CLONE` because
>> that's long gone.
>
> Volker Simonis has updated the pull request incrementally with one additional
> commit since the last revision:
>
> REALLY adding the test :)
test/jdk/java/lang/ProcessBuilder/JspawnhelperProtocol.java line 63:
> 61: if (p.exitValue() == 0) {
> 62: String pwd = p.inputReader().readLine();
> 63: if (!Path.of("").toAbsolutePath().toString().equals(pwd)) {
It would be useful to print the unexpected string; it might help isolate what
went wrong.
And make it easier to understand what ERROR+2 means.
test/jdk/java/lang/ProcessBuilder/JspawnhelperProtocol.java line 78:
> 76: pb =
> ProcessTools.createJavaProcessBuilder("-Djdk.lang.Process.launchMechanism=posix_spawn",
> 77: "JspawnhelperProtocol",
> 78: "normalExec");
I would just redirect the output to the parent.
`pb.inheritIO()` and avoid the extra code at line 88.
test/jdk/java/lang/ProcessBuilder/JspawnhelperProtocol.java line 106:
> 104: }
> 105: line = br.readLine();
> 106: }
Try-with-resources works well in cases like this. (Moving foundCrashInfo out
of the t-w-r).
test/jdk/java/lang/ProcessBuilder/JspawnhelperProtocol.java line 116:
> 114: int ret = p.exitValue();
> 115: if (ret == 0) {
> 116: throw new Exception("Expected error in child execution");
Mixing the two styles of error reporting seems a bit odd, some throw exceptions
and others just exit.
Being consistent throwing an exception with a message would be better.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/13956#discussion_r1196820848
PR Review Comment: https://git.openjdk.org/jdk/pull/13956#discussion_r1196824653
PR Review Comment: https://git.openjdk.org/jdk/pull/13956#discussion_r1196825895
PR Review Comment: https://git.openjdk.org/jdk/pull/13956#discussion_r1196827447
