RFR: 8307990: jspawnhelper must close its writing side of a pipe before reading from it [v3]

[Roger Riggs]

On Wed, 17 May 2023 16:00:23 GMT, Volker Simonis <simonis at 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