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

David Holmes david.holmes at oracle.com
Tue Aug 20 11:55:26 UTC 2019


Hi Adam,

On 20/08/2019 1:21 am, Adam Farley8 wrote:
> Hi Coleen, David,
> 
> Here's the new webrev: http://cr.openjdk.java.net/~afarley/8227021.5/webrev/

In this comment:

!  * in use, you can pass the length of that in n, to ensure
!  * we detect if any path exceeds the maximum path length
!  * once prepended onto the sub-path/file name.
    * It is the callers responsibility to:
    *   a> check the value of n, and n may be 0.

the three references to 'n' should be to the new file_name_length parameter.

Looking at the test:

   2  * Copyright (c) 2019, 2019, Oracle and/or its affiliates. All 
rights reserved.

For a new file there is only a single year specified so this should be:

   2  * Copyright (c) 2019, Oracle and/or its affiliates. All rights 
reserved.

  31  * @run main TestSunBootLibraryPath

As this just launches another VM it is more efficient to use "@run driver".

  35 import java.lang.IllegalArgumentException;

Nit: java.lang.* is implicitly imported.

  39     static String expectedErrorMessage =("The VM tried to use a 
path that exceeds the maximum path length for "
  40                                   + "this system. Review 
path-containing parameters and properties, such as "
  41                                   + "sun.boot.library.path, to 
identify potential sources for this path.");

Nit: no need for parentheses.

Probably also no need to check for the entire message - the first line 
should suffice.

  48             //Add enough characters to make it "too long".
  49             for (int i = 0 ; i < tooLongPathSize ; i++) tooLongPath 
+= "a";

This can be done more efficiently and succinctly as:

tooLongPath += "a".repeat(5000);

There's probably an even better way. :)

  60             throw new java.lang.IllegalArgumentException("Test was 
launched with an invalid argument.");

No need to specify "java.lang".

Thanks,
David
-----

> I made the requested changes, and moved the null check as suggested.
> 
> (I agree passing a null for any of these values would be odd, and 
> unlikely to be done on purpose.)
> 
> It also includes a new jtreg test for this change, which uses ProcessTools.
> 
> Let me know what you think (inc Serguei + others).
> 
> Best Regards
> 
> Adam Farley
> IBM Runtimes
> 
> 
> coleen.phillimore at oracle.com wrote on 16/08/2019 23:59:53:
> 
>> From: coleen.phillimore at oracle.com
>> To: Adam Farley8 <adam.farley at uk.ibm.com>
>> Cc: David Holmes <david.holmes at oracle.com>,  hotspot-dev at openjdk.java.net
>> Date: 17/08/2019 00:00
>> Subject: Re: RFR: JDK-8227021:  VM fails if any sun.boot.library.path
>> paths are longer than JVM_MAXPATHLEN
>> 
>> 
> 
>> On 8/16/19 7:35 AM, Adam Farley8  wrote:
>> Hi Coleen, 
>> 
>> To confirm your suspicions, I didn't see your responses. No offence  intended.
>> 
>> I send a lot of mail so I'm sure I can be caught in people's spam  filter. :)
>> 
>> 
>> Responses below.
>> 
>> Best Regards
>> 
>> Adam Farley 
>> IBM Runtimes
>> 
>> 
>> coleen.phillimore at oracle.com wrote on 15/08/2019 13:42:51:
>> 
>> > From: coleen.phillimore at oracle.com 
>> > To: hotspot-dev at openjdk.java.net, Adam Farley8 
>> > <adam.farley at uk.ibm.com>, David Holmes <david.holmes at oracle.com>
>> > Date: 15/08/2019 13:43 
>> > Subject: Re: RFR: JDK-8227021: VM fails if any sun.boot.library.path
>> > paths are longer than JVM_MAXPATHLEN 
>> > 
>> > Hi Adam,  David noticed that my code review replies only  went to the
>> > mailing list.  So here it is again.
>> > Thanks,
>> > Coleen
>> > 
>> > On 8/13/19 5:29 PM, coleen.phillimore at oracle.com wrote:
>> > >
>> > >
>> > > On 8/13/19 11:48 AM, Adam Farley8 wrote:
>> > >> Hi David,
>> > >>
>> > >> Since we have two positive reviews (yours and Serguei's),  could you
>> > >> sponsor the change please?
>> > >
>> > > If you get two positive reviews, but then one with comments,  you need
>> > > to also answer that third reviews.  I had some comments  on your
>> > > change, which I'll repeat here:
>> > >
>> > >
>> > > https://urldefense.proofpoint.com/v2/url?
>> > u=http-3A__cr.openjdk.java.net_-7Eafarley_8227021.
>> > 
>> 2_webrev_src_hotspot_share_runtime_os.cpp.frames.html&d=DwIDaQ&c=jf_iaSHvJObTbx-
>> > siA1ZOg&r=P5m8KWUXJf-
>> > CeVJc0hDGD9AQ2LkcXDC0PMV9ntVw5Ho&m=wjd3KxxgFQv49yjlb4Bndy-
>> > F1XV4pbuPZrmemiaFthA&s=xXwflqvuzoEhcREhr-x7rNMVDeroZ8Xh1LnW6NytumE&e= 
>> > >
>> > >
>> > > This is code is unnecessarily hard to read and understand:
>> > >
>> > > 1329 char** os::split_path(const char* path, size_t* n)  {
>> > > 1330 size_t pathmod = (size_t)0;
>> > > 1331 if (n != NULL) {
>> > > 1332 pathmod = *n;
>> > > 1333 }
>> > > 1334 *n = (size_t)0;
>> > >
>> > > It appears that 'n' is both an input and output parameter  of this
>> > > function.  Can you just make it another input parameter  with some
>> > > descriptive name.  Is it the length of the library  name that you're
>> > > looking for?  "pathmod" is huh? 
>> 
>> The 'n' is both an input and an output, yes. 
>> 
>> I could make it another input parameter, but that means modifying  the
>> method signature. This seemed unnecessary, given that we have an input
>> vector we're not using for anything.
>> 
>> It's not a big deal to add a parameter, especially since there's 
>> only one call site, and the names don't have to be squished up C names.
>> 
>> How about:  char** split_path(char* path, size_t file_length,
>> size_t* elements) ?
> 
>> 
>> 
>> The 'n' input is used as a modifier for the length of the path, as
>> mentioned in the comment. I called it "pathmod" because  it's a PATH
>> length MODIFIER. I couldn't think of a better name that's also concise.
>> Would pathlenmod be better? Pathsizemod sounds like we're making the  path
>> physically bigger, or maybe that's just me. :) 
>> 
>> > >
>> > > Then you don't have to check if someone has passed NULL. There's only
>> > > one caller to this from what I see. 
>> 
>> I'm not sure I understand this. We'd still need to check for null,  even if
>> the pathmod has its own input parameter. I believe adding a NULL to  an int
>> causes a segmentation error at runtime. 
>> 
>> If you're saying the null check isn't needed because we can ensure  anon-null
>> is passed in via the only place we call split_path, I disagree that  this is a
>> future-proof assumption. A null check seems prudent here.
>> 
>> I think the function doesn't make any sense if you pass an output
>> parameter as NULL.  Or at worst you could add it to the line:
>> 
>> 1335 if (path == NULL || strlen(path) == 0 || n == NULL) {
>> 1336      return NULL;
>> 1337 }
>> 1334 *n = (size_t)0; 
>> 
>> But that's just weird. Why would a caller pass a non-null path and
>> not want the element count?
>> 
>> Thanks,
>> Coleen
>> 
>> 
>> > >
>> > > 1352   char** opath = (char**) NEW_C_HEAP_ARRAY(char*,  count,
>> > > mtInternal);
>> > > 1353   if (opath == NULL) {
>> > > 1354     return NULL;
>> > > 1355   }
>> > >
>> > >
>> > > Not your change but this calls vm_exit_out_of_memory() if
>> > > NEW_C_HEAP_ARRAY fails.  It doesn't return NULL so  you don't have to
>> > > check. 
>> 
>> Seems legit. I'll include this in the .5 version once we have consensus  on
>> the other changes. Good spot. 
>> 
>> > >
>> > > 1375     char* s  = (char*)NEW_C_HEAP_ARRAY(char,  len + 1, mtInternal);
>> > >
>> > > Here you want to use NEW_C_HEAP_ARRAY_RETURN_NULL so you  can check for
>> > > null and free memory. 
>> 
>> Good idea. Noted for .5. 
>> 
>> Let me know your thoughts on the other points above.
>> 
>> 
>> > >
>> > >
>> > > The rest seems okay.
>> > >
>> > > thanks,
>> > > Coleen
>> > >
>> > >>
>> > >> 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