RFR(s): 8223777: In posix_spawn mode, failing to exec() jspawnhelper does not result in an error

[Roger Riggs]

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