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
Wed Jul 24 17:57:47 UTC 2019
Hi David,
Welcome back. :)
David Holmes <david.holmes at oracle.com> wrote on 22/07/2019 03:34:37:
> From: David Holmes <david.holmes at oracle.com>
> To: Adam Farley8 <adam.farley at uk.ibm.com>, hotspot-
> dev at openjdk.java.net, serviceability-dev
<serviceability-dev at openjdk.java.net>
> 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20190724/19f782df/attachment-0001.html>
More information about the serviceability-dev
mailing list