On Wed, 17 May 2023 12:33:48 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:
>
> Added test case
The fixes look reasonable, though I would prefer them to be split over
individual RFEs. Would make backporting easier too.
I would still like an automated test as part of the jtreg suite. Please see my
test proposal, that thing should be adaptable to work with your fix.
src/java.base/unix/native/jspawnhelper/jspawnhelper.c line 140:
> 138: struct stat buf;
> 139: /* argv[0] contains the fd number to read all the child info */
> 140: int r, fdinr, fdinw, fdout;
Since you are here, can you init these?
src/java.base/unix/native/libjava/ProcessImpl_md.c line 490:
> 488: pid_t resultPid;
> 489: int i, offset, rval, bufsize, magic;
> 490: char *buf, buf1[24];
Since you are here could you please increase buffer size to something safe? Max
len of INT_MIN is 11 bytes, so lets have at least 3*11 + 3 bytes.
-------------
PR Review: https://git.openjdk.org/jdk/pull/13956#pullrequestreview-1430748541
PR Review Comment: https://git.openjdk.org/jdk/pull/13956#discussion_r1196551493
PR Review Comment: https://git.openjdk.org/jdk/pull/13956#discussion_r1196586907
