RFR: JDK-8227021: VM fails if any sun.boot.library.path paths are longer than JVM_MAXPATHLEN
coleen.phillimore at oracle.com
coleen.phillimore at oracle.com
Fri Aug 16 22:59:53 UTC 2019
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 a
> non-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
More information about the hotspot-dev
mailing list