RFR(s): 8171508: Multiple issues with os::jvm_path

[Thomas Stüfe]

Hi David,

On Thu, Dec 22, 2016 at 1:56 AM, David Holmes <david.holmes at oracle.com>
wrote:

> Hi Thomas,
>
> I think this all seems reasonable. Unfortunately I can't see this as
> anything higher than the P4 bug it is already categorised as, and as such
> we are out of time to push this unless we get a second review right now so
> this can be pushed by the Dec 21 deadline.
>
>
No problem. As I wrote, I leave the severity to Oracle. We can also do it
in 10 and backport to 9.

I honestly do not fully understand the story behind -XXaltjvm, I did not
find any documentation and the source comments are sparse. But I see that
it did catch quite a lot of bad PR as a security hole (googling XXaltjvm +
exploit gives quite some hits). There are even suggestions in the net that
this option was intended as a deliberate backdoor :) I do not understand
why Oracle does not strip the option out of release builds, but as I wrote
I do not know the story behind this option fully.

As for us at SAP, I'll talk this over with the guys at work and on the SAP
JVM we might just remove this option from the release builds altogether.
Unfortunately this would mean the gtests would not run on a release VM. I
see this not as such a big deal however because gtests on release VM are of
limited use anyway when you miss all asserts.


> Does it prevent the AIX port for -XXaltjvm?
>

Not really. I would have preferred to fix this first in the other OSes and
port it to AIX, but I can do the fixed version in AIX first. Then we would
be better than other ports :)

I started vacation now, so I won't be able to do the AIX changes before
next year anyway.


>
> Thanks,
> David
>
>
Thank you!

Thomas


>
> On 22/12/2016 1:20 AM, Thomas Stüfe wrote:
>
>> Hi all,
>>
>> I would like your reviews for the following issue:
>>
>> issue: https://bugs.openjdk.java.net/browse/JDK-8171508
>> webrev:
>> http://cr.openjdk.java.net/~stuefe/webrevs/8171508-multiple-
>> issues-with-os_jvmpath/webrev.00/webrev/index.html
>>
>> I found that we had forgotten to port -XXaltjvm handling to AIX (needed
>> for
>> gtests). -XXaltjvm is used to run the jvm with a different hotspot. The
>> option gets used in the gtestLauncher to hand it a jdk located separately
>> from gtest libjvm.
>>
>> When looking at the code a number of issues came up and I decided to fix
>> them first before porting the code to AIX.
>>
>> The main issue I found is that realpath() was used in an unsafe way.
>> Before
>> POSIX.1-2008, realpath() was broken by design, because there was no safe
>> way to prevent the output buffer from overflowing. The function returns a
>> path, but there is no sure way to predict the maximum path length.
>>
>> POSIX1.2008 fixed this by specifying that if the output buffer argument
>> was
>> NULL, the function would return the path in dynamically allocated memory
>> which the user would have to free().
>>
>> Before POSIX1.2008, if the output buffer argument was NULL, the behaviour
>> was left up to the implementators by POSIX; I believe that on Linux and
>> BSD, the behaviour was always to return dynamically allocated memory.
>>
>> I hope it is safe to assume that the POSIX1.2008 behaviour is the standard
>> now for our Posix platforms. My fix wraps realpath() with a new function
>> os::Posix::realpath() which uses the new Posix behaviour.
>>
>> See also:
>> Pre POSIX1.2008:
>> http://pubs.opengroup.org/onlinepubs/009695399/functions/realpath.html
>> POSIX1.2008:
>> http://pubs.opengroup.org/onlinepubs/9699919799/functions/realpath.html
>> (see Rationale section below)
>>
>>
>> Other issues this change fixes:
>>
>> - There is a logic trying to determine if the path of the loaded libjvm.so
>> points to a valid JDK image by searching for "/jre/lib". That logic was
>> broken after JDK-8066474 "Remove the lib/$ARCH directory from Linux and
>> Solaris images".
>> - At some places return codes of realpath() were not checked.
>> - At other places error conditions were asserted, but not handled in the
>> release case.
>>
>> Please note that I tried to keep the change minimal. The code would
>> benefit
>> from a cleanup in a future release. I only fixed obvious issues, and
>> refrained from any unnecessary changes.
>>
>> I built the changes on Solaris, Linux and MacOS. I tested on all platforms
>> simply by letting the gtestLauncher run, which executes the -XXaltjvm code
>> path.
>>
>> The realpath issue may be security related, but the severity depends on
>> how
>> large PATH_MAX is on the build platform compared to the maximum possible
>> path length one would see on filesystems at target systems. I would like
>> to
>> see this fixed in jdk 9 but that decision is up to Oracle.
>>
>> Thanks & Kind Regards, Thomas
>>
>>