David Holmes <[email protected]> wrote on 22/07/2019 03:34:37:
From: David Holmes <[email protected]>
To: Adam Farley8 <[email protected]>, hotspot-
[email protected], serviceability-dev <[email protected]>
Date: 22/07/2019 03:34
Subject: Re: RFR: JDK-8227021: VM fails if any sun.boot.library.path
paths are longer than JVM_MAXPATHLEN
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 Holmes 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)
This is implemented in the ".1" webrev, though I did comment out a
necessary
line to attempt to test the linker_md changes. I've removed the "//" and
re-uploaded. It's the added line in the os.cpp file that begins
"vm_exit_during_initialization".
You're correct in that this code would only be triggered if we're loading a
library, though I'm not sure that's a problem. We seem to load a couple of
libraries every time we run even the most minimalist of classes, and if
we somehow manage not to load any libraries *at all*, the contents of a
library path property seems irrelevant.
>> - 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).
A reasonable suggestion. Thanks for the tip about sys.h. Seemed cleaner to
change sys.h, but this change isn't worth a CSR.
The jdwp changes were removed from the new ".2" webrev.
http://cr.openjdk.java.net/~afarley/8227021.2/webrev
>> - 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 ??
Correct, to my understanding. Likely more a problem on Windows than Linux.
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?
You're right. The code was originally changed to enable the "skip too-long
paths" logic, and then when we went to a "fail immediately" policy, I
tweaked
the modified code rather than start over again.
See the .2 webrev for this change.
http://cr.openjdk.java.net/~afarley/8227021.2/webrev
BTW the existing code fails to free opath before returning NULL.
True. I added a fix to free the memory in the two cases we do that.
Though not strictly needed in the vm-exit case, the internet suggested
it was bad practice to assume the os would handle it.
>> - 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.
Hmm, perhaps a simpler solution would be to make the error message more
vague and remove the passing-in of the path source.
E.g. "The VM tried to use a path that exceeds the maximum path length for "
"this system. Review path-containing parameters and properties,
such as "
"sun.boot.library.path, to identify potential sources for this path."
That way we're covered no matter where the path comes from.
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.
We'd have to check for changes to the source property every time we used
the value. E.g. copy the property into another string, split the paths,
cache the split, and compare that to the "live" property storage string
before using the cache.
That, or assume that sun.boot.library.path could never change after being
"split", an assumption which feels unsafe.
Lets see if others have comments/suggestions here.
Thanks,
David
Sure thing.
- Adam
>>
>> Bug: https://urldefense.proofpoint.com/v2/url?
u=https-3A__bugs.openjdk.java.net_browse_JDK-2D8227021&d=DwICaQ&c=jf_iaSHvJObTbx-
siA1ZOg&r=P5m8KWUXJf-
CeVJc0hDGD9AQ2LkcXDC0PMV9ntVw5Ho&m=eUr83eH1dOFWQ7zfl1cSle0RxJM9Ayl9AszJYR45Gvo&s=dAT1OR_BIZPvCjoGtIlC2J1CCoCB4n43JKHFLfuHrjA&e=
>>
>> New Webrev: https://urldefense.proofpoint.com/v2/url?
u=http-3A__cr.openjdk.java.net_-7Eafarley_8227021.
1_webrev_&d=DwICaQ&c=jf_iaSHvJObTbx-siA1ZOg&r=P5m8KWUXJf-
CeVJc0hDGD9AQ2LkcXDC0PMV9ntVw5Ho&m=eUr83eH1dOFWQ7zfl1cSle0RxJM9Ayl9AszJYR45Gvo&s=mGM6YxmVHe2xW8mlGgI0i7XBLCqdyHN0J1ECgZ8QuRo&e=
(Superseded by the .2 version)
>>
>> 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
>>
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
