On Mon, 15 May 2023 18:45:24 GMT, Roger Riggs <[email protected]> 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.
>
> I think writing a test is worth a little bit of time but getting a clean test
> run using the KILLTEST might be a bit tricky. The exit(-1) of the parent
> would be detected by jtreg as a failure. And verifying that the spawned child
> runs to completion might be difficult without writing a driver app to monitor
> the child; though it may be sufficient to know just that it does not hang and
> timeout. When the parent dies, the child will be re-parented.
> And it would only be run in debug builds.
@RogerRiggs @simonis
Okay, I wrote a jtreg test case for the issue, just to see if I could. Rebased
atop of Volkers change, you can just take the commit if you want:
https://github.com/tstuefe/jdk/tree/Added-TestCase-For-Volker
https://github.com/openjdk/jdk/commit/9119b442dac8dd95228d47d70871ee59862649ce
Test case fails immediately (so, no relying on timeouts) if Volkers patch is
missing. Succeeds immediately with Volkers patch. I also tried commenting out
the fixing close() call, test fails immediately.
Patch relies on jspawnhelper to write a small marker file on communication
error with parent if KILLTEST is set. Only in debug code. I think this is very
reliable. No second-guessing aliveness of the child and worrying about PID
reuse. Re-parenting is no issue either. And I think such a file make be a good
idea anyway to analyze handshake errors with jspawnhelper.
Tested on Linux x64.
@simonis, feel free to add me as contributor if you take this test. I spent way
to much time on this :-)
-------------
PR Comment: https://git.openjdk.org/jdk/pull/13956#issuecomment-1549172824
