RFR(M): 8186072: dll_build_name returns true even if file is missing.
Lindenmaier, Goetz
goetz.lindenmaier at sap.com
Thu Aug 17 16:03:13 UTC 2017
Hi Thomas,
I adapted the comments in os.hpp.
If I move the call to dll_build_name out of dll_locate_lib
I have to do a lot of coding in all the places where it is called.
That seems not useful to me.
Fixed the type to size_t.
One could merge posix/windows if putting the check for ‘:’
into a WINDOWS_ONLY() I guess. The check for \ could be
done in posix as well, if using file_seperator().
Ø Not your change, but: why does the code in os::dll_locate_lib() even
Ø differentiate between a PATH containing no os::path_separator()
Ø and a path containing os::path_separator()?
I assume this was done to avoid all the allocations and copying of the path.
Also adapted the comment in jvmtiExport.cpp.
New webrev:
http://cr.openjdk.java.net/~goetz/wr17/8186072-dllBuildName/webrev.03/
incremental diff:
http://cr.openjdk.java.net/~goetz/wr17/8186072-dllBuildName/webrev.03/diffs-incremental.patch
(fixed indentation on windows)
Best regards,
Goetz.
From: Thomas Stüfe [mailto:thomas.stuefe at gmail.com]
Sent: Thursday, August 17, 2017 3:48 PM
To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com>
Cc: hotspot-runtime-dev at openjdk.java.net
Subject: Re: RFR(M): 8186072: dll_build_name returns true even if file is missing.
Hi Goetz,
On Thu, Aug 17, 2017 at 1:35 PM, Lindenmaier, Goetz <goetz.lindenmaier at sap.com<mailto:goetz.lindenmaier at sap.com>> wrote:
Hi Thomas,
I reworked the whole thing.
First, there is dll_build_name. It just does <name> -> lib<name>.so.
Second, I renamed the legacy dll_build_name to dll_locate_lib.
I merged all the unix variants to one in os_posix.
I removed the buffer overflow check at the top.
It's too restrictive because the path argument
can contain several paths. I added the overflow
checks into the single cases.
Also, I first assemble the pure name using the new, simple
dll_build_name. This is for reuse and readability.
In case of an empty directory, I use get_current_directory
to complete the path as indicated by the original documentation
where it was called with "".
Dll_locate_lib now always returns a name with a full path if
the file exists.
Also, on windows, I think I fixed a bug by reversing the order
of checks. A path list ending in ':' or '\' would not have
been recognized.
On Bsd, I removed JNI_LIB_* because that already is defined
in jvm_bsh.h
New webrev:
http://cr.openjdk.java.net/~goetz/wr17/8186072-dllBuildName/webrev.02/
Best regards,
Goetz.
I like this better than before. Remarks:
http://cr.openjdk.java.net/~goetz/wr17/8186072-dllBuildName/webrev.02/src/share/vm/runtime/os.hpp.udiff.html
+ // Builds the platform-specific name of a library.
+ // Returns false on __buffer overflow__.
Hopefully not! :D
How about: "Returns false no truncation" instead.
+ // Builds a platform-specific full library path given an ld path and lib name.
+ // Returns true if the buffer contains a full path to an existing file, false
+ // otherwise. If pathname is empty, checks the current directory.
+ static bool dll_locate_lib(char* buffer, size_t size,
const char* pathname, const char* fname);
Might be worth mentioning that "fname" is the unadorned library name, e.g. "verify" for libverify.so or verify.dll.
Would the following alternative be valid:
one could make dll_locate_lib take the real file name, and let caller use dll_build_name() to build the libary name first before handing it to dll_locate_lib(). In that case, dll_locate_lib() could be renamed to a generic "find_file_in_path" because it would work for any kind of file.
As an added bonus, there would be no need to create a temporary array in dll_build_name/dll_locate_lib, and no need to call free() so no cleanup-related control flow changes in these functions.
=====
http://cr.openjdk.java.net/~goetz/wr17/8186072-dllBuildName/webrev.02/src/os/windows/vm/os_windows.cpp.udiff.html
+ int fullfnamelen = strlen(JNI_LIB_PREFIX) + strlen(fname) + strlen(JNI_LIB_SUFFIX);
int -> size_t (does that even compile without warning?)
+ // Check current working directory.
+ const char* p = get_current_directory(buffer, buflen);
+ if (p != NULL &&
+ strlen(buffer) + 1 + fullfnamelen + 1 <= buflen) {
+ strcat(buffer, "\\");
+ strcat(buffer, fullfname);
+ retval = file_exists(buffer);
Small nit: I'd use jio_snprintf instead of strcat. Functionally identical but will make scanners (e.g. coverity) happy. One could then avoid the length calculation and rely on jio_snprintf truncation:
const char* p = get_current_directory(buffer, buflen);
if (p != NULL) {
const size_t end = strlen(p);
if (jio_snprintf(end, buflen - end, "\\%s<file:///\\%25s>", fullname) != -1) {
retval = file_exists(buffer);
}
}
--
Not your change, but: why does the code in os::dll_locate_lib() even differentiate between a PATH containing no os::path_separator() and a path containing os::path_separator()?
Would the former not be just a PATH with only one directory and hence need no special treatment?
=====
http://cr.openjdk.java.net/~goetz/wr17/8186072-dllBuildName/webrev.02/src/os/posix/vm/os_posix.cpp.udiff.html
Could os::dll_locate_lib be consolidated between windows and unix? Seems to be the implementation is almost identical.
====
http://cr.openjdk.java.net/~goetz/wr17/8186072-dllBuildName/webrev.02/src/share/vm/prims/jvmtiExport.cpp.udiff.html
+ // not found - try library path
Proposal: "not found - try OS default library path"
Find some comments inline:
> Especially if the path is empty, it just returns 'true'.
> Dll_build_name is usually used before calling dll_load. If dll_load does not get a full path it searches
> in well known unix/windows locations. This is intended in the two cases where dll_build_name
> is called with an empty path.
>
> So, for both cases (thread.cpp, jvmtiExport.cpp),
>
> before, we would call os::dll_build_name() with an empty string for the path
> which, for relative paths, would result in feeding that path unexpanded to
> dlopen(), which would use whatever the OS does in those cases (LIBPATH,
> LD_LIBRARY_PATH, PATH on windows). Note that this does not necessarily
> include searching the current directory.
Right. With changed dll_biuld_name it's again exactly as before.
> With your change, we now use java.library.path, which is not necessarily the
> same?
You are right, I oversaw that java.library.path can be overwritten. Initially,
it's set to the right thing.
> (BTW, I think the old comments in thread.cpp and jniExport.cpp were wrong:"//
> Try the local directory" - if "local" means "current", this is not what did
> happen).
Right, I tried to adapt them, did I miss one?
> I added a second variant of dll_build_name without the path argument that adds the path
> from system property java.lang.path and use that in these two cases.
> I changed the original function to actually check file availability in all cases,
> and to check . if the path is empty.
> I think that may be a bit confusing. We would then have three options:
>
> - call os::dll_build_name with a real "<aa>;<bb>;.." PATH and get a file name
> resolved from that path
> - call os::dll_build_name with "" for the PATH and get OS dll resolution
No, in that case, as I called file_exists(), it would only work if the dll is in the
current working directory. But I changed this now, anyways.
> - call your new overloaded version of os::dll_build_name(), which uses -
> Djava.library.path.
>
> Please review this change. I please need a sponsor.
> http://cr.openjdk.java.net/~goetz/wr17/8186072-
> dllBuildName/webrev.01/ <http://cr.openjdk.java.net/~goetz/wr17/8186072-
> dllBuildName/webrev.01/>
>
> Best regards,
> Goetz.
>
>
>
>
> Kind Regards, Thomas
Best Regards, Thomas
More information about the hotspot-runtime-dev
mailing list