RFR(M): 8186072: dll_build_name returns true even if file is missing.

Lindenmaier, Goetz goetz.lindenmaier at sap.com
Thu Aug 17 11:35:44 UTC 2017


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.

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



More information about the hotspot-runtime-dev mailing list