RFR: JDK-8227021: VM fails if any sun.boot.library.path paths are longer than JVM_MAXPATHLEN
David Holmes
david.holmes at oracle.com
Thu Aug 15 09:42:35 UTC 2019
Hi Serguei,
On 15/08/2019 6:18 pm, serguei.spitsyn at oracle.com wrote:
> Thank you, Adam!
>
> David, what do you thin about this testing coverage?
> Is it enough for this fix?
I don't see any real testing of sun.boot.library.path in our tests only:
./hotspot/jtreg/runtime/6819213/TestBootNativeLibraryPath.java
./hotspot/jtreg/runtime/jni/FindClass/FindClassFromBoot.java
seem to refer to it but the former is restricted to 32-bit systems only!
So this seems to be an area devoid of any real test code outside of
32-bit. I don't understand why that is the case. So if there's no
regression test then there's no real testing of this code at all. That
said it seems unfair to force Adam to define tests for code that is
effectively untested. It would be reasonable for him to extend existing
tests to cover the long path problem, if they existed.
But a basic regression test should at least be created IMO.
David
-----
> Thanks,
> Serguei
>
>
> On 8/14/19 04:09, Adam Farley8 wrote:
>> Addendum: I've run the test/hotspot/jtreg/runtime tests against a
>> patched and unpatched build on linux x86 64bit, and the pass/fail rate
>> is identical.
>>
>> 630 passed, 7 failed, and 63 error in both cases.
>>
>> Best Regards
>>
>> Adam Farley
>> IBM Runtimes
>>
>>
>> Adam Farley8/UK/IBM wrote on 13/08/2019 17:36:44:
>>
>> > From: Adam Farley8/UK/IBM
>> > To: "serguei.spitsyn at oracle.com" <serguei.spitsyn at oracle.com>,
>> > daniel.daugherty at oracle.com
>> > Cc: David Holmes <david.holmes at oracle.com>, hotspot-dev at openjdk.java.net
>> > Date: 13/08/2019 17:36
>> > Subject: Re: RFR: JDK-8227021: VM fails if any sun.boot.library.path
>> > paths are longer than JVM_MAXPATHLEN
>> >
>> > Hi Serguei, Daniel,
>> >
>> > I ran the bug specific test mentioned in the bug, as well as this:
>> > test/hotspot/jtreg/sanity
>> >
>> > No failures. Well, except for the one we wanted in the bug test. :)
>> >
>> > On reflection, this seems thin, so I'm running some more hotspot
>> tests now.
>> >
>> > Advice welcome.
>> >
>> > Best Regards
>> >
>> > Adam Farley
>> > IBM Runtimes
>>
>> >
>> > "serguei.spitsyn at oracle.com" <serguei.spitsyn at oracle.com> wrote on
>> > 13/08/2019 17:09:17:
>> >
>> > > From: "serguei.spitsyn at oracle.com" <serguei.spitsyn at oracle.com>
>> > > To: Adam Farley8 <adam.farley at uk.ibm.com>, David Holmes
>> > > <david.holmes at oracle.com>
>> > > Cc: hotspot-dev at openjdk.java.net
>> > > Date: 13/08/2019 17:20
>> > > Subject: Re: RFR: JDK-8227021: VM fails if any sun.boot.library.path
>> > > paths are longer than JVM_MAXPATHLEN
>> > >
>> > > Hi Adam,
>> > >
>> > > Could you, please, tell a little bit on how did you test the fix?
>> > >
>> > > Thanks,
>> > > Serguei
>> > >
>> > >
>> > > On 8/13/19 09:00, serguei.spitsyn at oracle.com wrote:
>> > > Hi Adam,
>> > >
>> > > I'll sponsor the push.
>> > >
>> > > Thanks,
>> > > Serguei
>> > >
>> > >
>> > > On 8/13/19 08:48, Adam Farley8 wrote:
>> > > Hi David,
>> > >
>> > > Since we have two positive reviews (yours and Serguei's), could you
>> > > sponsor the change please?
>> > >
>> > > Best Regards
>> > >
>> > > Adam Farley
>> > > IBM Runtimes
>> > >
>> > >
>> > > David Holmes <david.holmes at oracle.com> wrote on 12/08/2019 23:32:14:
>> > >
>> > > > From: David Holmes <david.holmes at oracle.com>
>> > > > To: Adam Farley8 <adam.farley at uk.ibm.com>
>> > > > Cc: hotspot-dev at openjdk.java.net, "serguei.spitsyn at oracle.com"
>> > > > <serguei.spitsyn at oracle.com>
>> > > > Date: 12/08/2019 23:32
>> > > > Subject: Re: RFR: JDK-8227021: VM fails if any sun.boot.library.path
>> > > > paths are longer than JVM_MAXPATHLEN
>> > > >
>> > > > Looks fine.
>> > > >
>> > > > Thanks,
>> > > > David
>> > > >
>> > > > On 13/08/2019 1:19 am, Adam Farley8 wrote:
>> > > > > Hi David,
>> > > > >
>> > > > > Changes made as requested:
>> > > > > https://urldefense.proofpoint.com/v2/url?
>> > > > u=http-3A__cr.openjdk.java.net_-7Eafarley_8227021.4_webrev&d=DwID-
>> > > > g&c=jf_iaSHvJObTbx-siA1ZOg&r=P5m8KWUXJf-
>> > > >
>> > >
>> >
>> CeVJc0hDGD9AQ2LkcXDC0PMV9ntVw5Ho&m=mZM8IR4e_zmrfcsF3XJjoGEMrZb4WtEL7Y6Ugd6Naqg&s=sZ2UKqqUIq0El-
>> > > > RsqYz6jmTh4Q2UghwdrQX6of8Lw0E&e=
>> > > > >
>> > > > > Best Regards
>> > > > >
>> > > > > Adam Farley
>> > > > > IBM Runtimes
>> > > > >
>> > > > >
>> > > > > David Holmes <david.holmes at oracle.com> wrote on 12/08/2019
>> 04:55:36:
>> > > > >
>> > > > >> From: David Holmes <david.holmes at oracle.com>
>> > > > >> To: Adam Farley8 <adam.farley at uk.ibm.com>,
>> > > > >> "serguei.spitsyn at oracle.com" <serguei.spitsyn at oracle.com>
>> > > > >> Cc: hotspot-dev at openjdk.java.net
>> > > > >> Date: 12/08/2019 04:56
>> > > > >> Subject: Re: RFR: JDK-8227021: VM fails if any
>> sun.boot.library.path
>> > > > >> paths are longer than JVM_MAXPATHLEN
>> > > > >>
>> > > > >> Hi Adam,
>> > > > >>
>> > > > >> On 10/08/2019 2:47 am, Adam Farley8 wrote:
>> > > > >> > Hi Serguei, David,
>> > > > >> >
>> > > > >> > My turn to apologise for the delay. :)
>> > > > >> >
>> > > > >> > I've modified the code as per Serguei's request. Take a look
>> > > and let me
>> > > > >> > know if this is the sort of thing you were thinking of.
>> > > > >> >
>> > > > >> > Webrev: https://urldefense.proofpoint.com/v2/url?
>> > > > >>
>> u=http-3A__cr.openjdk.java.net_-7Eafarley_8227021.3_webrev_&d=DwID-
>> > > > >> g&c=jf_iaSHvJObTbx-siA1ZOg&r=P5m8KWUXJf-
>> > > > >> CeVJc0hDGD9AQ2LkcXDC0PMV9ntVw5Ho&m=8Wa8Zdfmvn-
>> > > > >>
>> > > >
>> > >
>> >
>> yvzvCAhOyJ_etFblRA4vmLGbKF4aW8PY&s=L19aeFXoR9JIO62QRPFzZObIU8RbhpCtXSvUibD2ISk&e=
>> > > > >>
>> > > > >> I'd prefer to see the helper just as a file static function
>> > rather than
>> > > > >> adding it to the os class.
>> > > > >>
>> > > > >> + * supplied array of arrays of chars (a), where n
>> > > > >>
>> > > > >> I assume (a) is meant to refer to the parameter, but you
>> > actually called
>> > > > >> it arrayarray. I think "a" or "arr" would suffice.
>> > > > >>
>> > > > >> Thanks,
>> > > > >> David
>> > > > >>
>> > > > >> > Best Regards
>> > > > >> >
>> > > > >> > Adam Farley
>> > > > >> > IBM Runtimes
>> > > > >> >
>> > > > >> >
>> > > > >> > "serguei.spitsyn at oracle.com" <serguei.spitsyn at oracle.com>
>> wrote on
>> > > > >> > 31/07/2019 17:18:05:
>> > > > >> >
>> > > > >> >> From: "serguei.spitsyn at oracle.com"
>> <serguei.spitsyn at oracle.com>
>> > > > >> >> To: Adam Farley8 <adam.farley at uk.ibm.com>, David Holmes
>> > > > >> >> <david.holmes at oracle.com>
>> > > > >> >> Cc: serviceability-dev <serviceability-dev at openjdk.java.net>,
>> > > > >> >> hotspot-dev at openjdk.java.net
>> > > > >> >> Date: 31/07/2019 17:18
>> > > > >> >> Subject: Re: RFR: JDK-8227021: VM fails if any
>> > sun.boot.library.path
>> > > > >> >> paths are longer than JVM_MAXPATHLEN
>> > > > >> >>
>> > > > >> >> Hi Adam,
>> > > > >> >>
>> > > > >> >> It looks Okay to me.
>> > > > >> >>
>> > > > >> >> A couple of minor comments.
>> > > > >> >>
>> > > > >> >> https://urldefense.proofpoint.com/v2/url?
>> > > > >> u=http-3A__cr.openjdk.java.net_-7Eafarley_8227021.
>> > > > >> 2_webrev_src_hotspot_&d=DwID-g&c=jf_iaSHvJObTbx-
>> > > > >>
>> siA1ZOg&r=P5m8KWUXJf-CeVJc0hDGD9AQ2LkcXDC0PMV9ntVw5Ho&m=8Wa8Zdfmvn-
>> > > > >>
>> > > >
>> > >
>> >
>> yvzvCAhOyJ_etFblRA4vmLGbKF4aW8PY&s=NL6tYuwwDod3DSmj-1ztxAywpO8L52HEyO0wvTR05bs&e=
>> > > > >> >> share/runtime/os.cpp.frames.html
>> > > > >> >
>> > > > >> >> 1362 //release allocated storage before exiting the vm
>> > > > >> >> 1363 while (i > 0) {
>> > > > >> >> 1364 i--;
>> > > > >> >> 1365 if (opath[i] != NULL) {
>> > > > >> >> 1366 FREE_C_HEAP_ARRAY(char, opath[i]);
>> > > > >> >> 1367 }
>> > > > >> >> 1368 }
>> > > > >> >> 1369 FREE_C_HEAP_ARRAY(char*, opath);
>> > > > >> >>
>> > > > >> >> 1377 //release allocated storage before returning null
>> > > > >> >> 1378 while (i > 0) {
>> > > > >> >> 1379 i--;
>> > > > >> >> 1380 if (opath[i] != NULL) {
>> > > > >> >> 1381 FREE_C_HEAP_ARRAY(char, opath[i]);
>> > > > >> >> 1382 }
>> > > > >> >> 1383 }
>> > > > >> >> 1384 FREE_C_HEAP_ARRAY(char*, opath);
>> > > > >> >>
>> > > > >> >> This duplicated fragments is worth to refactor to a function.
>> > > > >> >> Also a space is missed at the beginning of the comment.
>> > > > >> >>
>> > > > >> >>
>> > > > >> >> Thanks,
>> > > > >> >> Serguei
>> > > > >> >>
>> > > > >> >>
>> > > > >> >>
>> > > > >> >> On 7/31/19 02:01, Adam Farley8 wrote:
>> > > > >> >> Hi All,
>> > > > >> >>
>> > > > >> >> Reviewers requested for the change below.
>> > > > >> >>
>> > > > >> >> @David - Agreed. Would you be prepared to sponsor the change?
>> > > > >> >>
>> > > > >> >> Bug: https://urldefense.proofpoint.com/v2/url?
>> > > > >> u=https-3A__bugs.openjdk.java.net_browse_JDK-2D8227021&d=DwID-
>> > > > >> g&c=jf_iaSHvJObTbx-siA1ZOg&r=P5m8KWUXJf-
>> > > > >> CeVJc0hDGD9AQ2LkcXDC0PMV9ntVw5Ho&m=8Wa8Zdfmvn-
>> > > > >>
>> > > >
>> > >
>> >
>> yvzvCAhOyJ_etFblRA4vmLGbKF4aW8PY&s=xykJ0KLy9AKWO8zmC0amfR7xxUsvyKEjlf3y7WWOqvE&e=
>> > > > >> >> Webrev: https://urldefense.proofpoint.com/v2/url?
>> > > > >>
>> u=http-3A__cr.openjdk.java.net_-7Eafarley_8227021.2_webrev_&d=DwID-
>> > > > >> g&c=jf_iaSHvJObTbx-siA1ZOg&r=P5m8KWUXJf-
>> > > > >> CeVJc0hDGD9AQ2LkcXDC0PMV9ntVw5Ho&m=8Wa8Zdfmvn-
>> > > > >>
>> > > >
>> > >
>> >
>> yvzvCAhOyJ_etFblRA4vmLGbKF4aW8PY&s=NvIza4VVWG3CiDhmQVmXsghH_4h_c5mFJbHwkCUcut0&e=
>> > > > >> >>
>> > > > >> >> Best Regards
>> > > > >> >>
>> > > > >> >> Adam Farley
>> > > > >> >> IBM Runtimes
>> > > > >> >>
>> > > > >> >> P.S. Remembered to add the links this time. :)
>> > > > >> >>
>> > > > >> >>
>> > > > >> >> David Holmes <david.holmes at oracle.com> wrote on
>> 30/07/2019 03:37:53:
>> > > > >> >>
>> > > > >> >> > From: David Holmes <david.holmes at oracle.com>
>> > > > >> >> > To: Adam Farley8 <adam.farley at uk.ibm.com>
>> > > > >> >> > Cc: hotspot-dev at openjdk.java.net, serviceability-dev
>> > > > >> >> > <serviceability-dev at openjdk.java.net>
>> > > > >> >> > Date: 30/07/2019 03:38
>> > > > >> >> > Subject: Re: RFR: JDK-8227021: VM fails if any
>> > sun.boot.library.path
>> > > > >> >> > paths are longer than JVM_MAXPATHLEN
>> > > > >> >> >
>> > > > >> >> > Hi Adam,
>> > > > >> >> >
>> > > > >> >> > On 25/07/2019 3:57 am, Adam Farley8 wrote:
>> > > > >> >> > > Hi David,
>> > > > >> >> > >
>> > > > >> >> > > Welcome back. :)
>> > > > >> >> >
>> > > > >> >> > Thanks. Sorry for the delay in getting back to this.
>> > > > >> >> >
>> > > > >> >> > I like .v2 as it is much simpler (notwithstanding freeing
>> > > > the already
>> > > > >> >> > allocated arrays adds some complexity - thanks for
>> fixing that).
>> > > > >> >> >
>> > > > >> >> > I'm still not sure we can't optimise things better for
>> > unchangeable
>> > > > >> >> > properties like the boot libary path, but that's another
>> RFE.
>> > > > >> >> >
>> > > > >> >> > Thanks,
>> > > > >> >> > David
>> > > > >> >> >
>> > > > >> >> Unless stated otherwise above:
>> > > > >> >> IBM United Kingdom Limited - Registered in England and
>> Wales with
>> > > > >> >> number 741598.
>> > > > >> >> Registered office: PO Box 41, North Harbour, Portsmouth,
>> > > > Hampshire PO6 3AU
>> > > > >> > Unless stated otherwise above:
>> > > > >> > IBM United Kingdom Limited - Registered in England and Wales
>> > > with number
>> > > > >> > 741598.
>> > > > >> > Registered office: PO Box 41, North Harbour, Portsmouth,
>> > > > Hampshire PO6 3AU
>> > > > >>
>> > > > >
>> > > > > Unless stated otherwise above:
>> > > > > IBM United Kingdom Limited - Registered in England and Wales
>> > with number
>> > > > > 741598.
>> > > > > Registered office: PO Box 41, North Harbour, Portsmouth,
>> > Hampshire PO6 3AU
>> > > >
>> > >
>> > > Unless stated otherwise above:
>> > > IBM United Kingdom Limited - Registered in England and Wales with
>> > > number 741598.
>> > > Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire
>> PO6 3AU
>> > Unless stated otherwise above:
>> > IBM United Kingdom Limited - Registered in England and Wales with
>> > number 741598.
>> > Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire
>> PO6 3AU
>>
>> Unless stated otherwise above:
>> IBM United Kingdom Limited - Registered in England and Wales with
>> number 741598.
>> Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU
>
More information about the hotspot-dev
mailing list