Hi Thomas,
Some thoughts...
Did you determine exactly how the failure of exec did not result in an
IOException?
The change at 696 just prevents an exception from getting overwritten but
not one that is not thrown or was cleared.
An improved test for a successful spawnChild could be added to
initialize resultPid to -1
and check that it has been overwritten with the pid. If posix_spawn
returned success,
there would still be an indication that the process was or was not created.
Can the function be named to indicate what is it checking? For example,
isHelperExecutable?
It returns a boolean and it would be more conventional to stick to the C
interpretation
of 0 == true, anything else is not.
Requiring all three exec bits may be overkill and does not match the
actual access test.
It might be confusing if the permissions were set so Linux could exec
it, but the Java runtime
threw an exception because it had a stricter requirement (not documented).
Since this is in the normal path and will very rarely fail, it is
reasonable to ask
about any potential performance impact. Can the test for exec and more
specific message
be used only if the exec fails.
The exception includes the literal "jspawnhelper" which would be
misleading if it were
changed and it might be clearer to mention 'exec' permission in the message.
Thanks, Roger
On 05/13/2019 12:00 PM, Thomas Stüfe wrote:
Hi all,
may I have please your opinions about the following change:
Bug: https://bugs.openjdk.java.net/browse/JDK-8223777
webrev:
http://cr.openjdk.java.net/~stuefe/webrevs/8223777-posix_spawn-no-exec-error/webrev.00/webrev/
In short, since some weeks posix_spawn() is used on Linux to spawn childs.
But the glibc implementation of posix_spawn does not report errors back to
the caller if exec()ing the target binary fails.
We only ever exec the jspawnhelper, which is part of the JDK and which then
exec()s the real target binary. The second exec() is under our control and
Martin did provide proper error reporting.
However, the first exec(), performed inside posix_spawn(), may fail too.
Most posix_spawn implementations will tell us if that happens, not glibc
(it is cumbersome to implement, one needs a pipe and somesuch, and POSIX
leaves a bit of headroom for the implementor to cheap out, so they did.)
So, if someone messes with the JDK installation and e.g. removes the
execute bits from jspawnhelper, that first exec() fails, but we get no
error. From the outside it looks like Runtime.exec() succeeded.
This is impossible to fix completely - see Martin's comment about the
impossibility to foresee whether an exec() will succeed without actually
exec()ing. But at least we can test the execute permissions on the
jspawnhelper. Which this fix does. This fixes Ubuntu 16.4 (Now, I get an
IOException if jspawnhelper is not executable).
Since I like to be super careful with runtime.exec coding, I will run this
through our nightlies and also would like feedback from the core libs
people. Also, I am not sure about the runtime costs of this hack. Seems a
bit excessive for something which is just a setup error (a correctly
installed JDK should never have this problem).
As a side note, it may help if jspawnhelper would live in the bin folder of
the JDK (currently lives in lib), because everyone should be aware that
programs in bin need execute permissions...
Cheers, Thomas
