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
Tue Aug 13 21:29:12 UTC 2019



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:


http://cr.openjdk.java.net/~afarley/8227021.2/webrev/src/hotspot/share/runtime/os.cpp.frames.html 


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?

Then you don't have to check if someone has passed NULL.  There's only 
one caller to this from what I see.

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.

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.


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
>



More information about the hotspot-dev mailing list