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

David Holmes david.holmes at oracle.com
Tue Jul 30 02:37:53 UTC 2019 

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

> 
> 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

More information about the serviceability-dev mailing list