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
