RFR: JDK-8227021: VM fails if any sun.boot.library.path paths are longer than JVM_MAXPATHLEN

Thu Aug 15 15:45:34 UTC 2019

Hi David,

Okay, thanks!
This was my guess too.

Thanks,
Serguei

On 8/15/19 02:42, David Holmes wrote:
> 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
>>