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

[David Holmes]

Hi Adam,

Some higher-level issues/concerns ...

On 22/07/2019 11:25 am, David Holmes wrote:
> Hi Adam,
> 
> Adding in serviceability-dev as you've made changes in that area too.
> 
> Will take a closer look at the changes soon.
> 
> David
> -----
> 
> On 18/07/2019 2:05 am, Adam Farley8 wrote:
>> Hey All,
>>
>> Reviewers and sponsors requested to inspect the following.
>>
>> I've re-written the code change, as discussed with David Holes in emails
>> last week, and now the webrev changes do this:
>>
>> - Cause the VM to shut down with a relevant error message if one or more
>> of the sun.boot.library.path paths is too long for the system.

I'm not seeing that implemented at the moment. Nor am I clear that such 
an error will always be detected during VM initialization. The code 
paths look fairly general purpose, but perhaps that is an illusion and 
we will always check this during initialization? (also see discussion at 
end)

>> - Apply similar error-producing code to the (legacy?) code in 
>> linker_md.c.

I think the JDWP changes need to be split off and handled under their 
own issue. It's a similar issue but not directly related. Also the 
change to sys.h raises the need for a CSR request as it seems to be 
exported for external use - though I can't find any existing test code 
that includes it, or which uses the affected code (which is another 
reason so split this of and let serviceability folk consider it).

>> - Allow the numerical parameter for split_path to indicate anything we
>> plan to add to the path once split, allowing for more accurate path 
>> length detection.

This is a bit icky but I understand your desire to be more accurate with 
the checking - as otherwise you would still need to keep overflow checks 
in other places once the full path+name is assembled. But then such 
checks must be missing in places now ??

I'm not clear why you have implemented the path check the way you 
instead of simply augmenting the existing code ie. where we have:

1347   // do the actual splitting
1348   p = inpath;
1349   for (int i = 0 ; i < count ; i++) {
1350     size_t len = strcspn(p, os::path_separator());
1351     if (len > JVM_MAXPATHLEN) {
1352       return NULL;
1353     }

why not just change the calculation at line 1351 to include the prefix 
length, and then report the error rather than return NULL?

BTW the existing code fails to free opath before returning NULL.

>> - Add an optional parameter to the os::split_path function that specifies
>> where the paths came from, for a better error message.

It's not appropriate to set that up in os::dll_locate_lib, hardwired as 
"sun.boot.library.path". os::dll_locate_lib doesn't know where it is 
being asked to look, it is the callers that usually use 
Arguments::get_dll_dir(), but in one case in jvmciEnv.cpp we have:

os::dll_locate_lib(path, sizeof(path), JVMCILibPath, ...

so the error message would be wrong in that case. If you want to pass 
through this diagnostic help information then it needs to be set by the 
callers of, and passed into, os::dll_locate_lib.

Looking at all the callers of os::dll_locate_lib that all pass 
Arguments::get_dll_dir, it seems quite inefficient that we will 
potentially split the same set of paths multiple times. I wonder whether 
we can do this specifically during VM initialization and cache the split 
paths instead? That doesn't address the problem of a path element that 
only exceeds the maximum length when a specific library name is added, 
but I'm trying to see how to minimise the splitting and put the onus for 
the checking back on the code creating the paths.

Lets see if others have comments/suggestions here.

Thanks,
David

>>
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8227021
>>
>> New Webrev: http://cr.openjdk.java.net/~afarley/8227021.1/webrev/
>>
>> Best Regards
>>
>> Adam Farley
>> IBM Runtimes
>>
>> 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
>>