RFR: JDK-8227021: VM fails if any sun.boot.library.path paths are longer than JVM_MAXPATHLEN
Adam Farley8
adam.farley at uk.ibm.com
Fri Aug 16 11:35:31 UTC 2019
Hi Coleen,
To confirm your suspicions, I didn't see your responses. No offence
intended.
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.
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.
> >
> > 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