RFR(M): 8186072: dll_build_name returns true even if file is missing.
Lindenmaier, Goetz
goetz.lindenmaier at sap.com
Tue Aug 22 14:33:09 UTC 2017
I mistyped the path to webrev, this should work:
http://cr.openjdk.java.net/~goetz/wr17/8186072-dllBuildName/webrev.04
Sorry,
Goetz
> -----Original Message-----
> From: Lindenmaier, Goetz
> Sent: Dienstag, 22. August 2017 15:48
> To: 'Thomas Stüfe' <thomas.stuefe at gmail.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,
>
> could I please get a second review?
> http://cr.openjdk.java.net/~goetz/wr17/8186072-dllBuildName-hs/webrev.04
>
> I had to update the webrev because of a problem on windows.
> @Thomas I had edited os.hpp, but not saved :(
>
> Best regards,
> Goetz.
>
> PS: Didn't double-check the webrev as cr server is slow.
>
> > -----Original Message-----
> > From: Thomas Stüfe [mailto:thomas.stuefe at gmail.com]
> > Sent: Donnerstag, 17. August 2017 19:54
> > 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 6:03 PM, Lindenmaier, Goetz
> > <goetz.lindenmaier at sap.com <mailto:goetz.lindenmaier at sap.com> > wrote:
> >
> >
> > 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/ <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
> > <http://cr.openjdk.java.net/~goetz/wr17/8186072-
> > dllBuildName/webrev.03/diffs-incremental.patch>
> >
> > (fixed indentation on windows)
> >
> >
> >
> > Best regards,
> >
> > Goetz.
> >
> >
> >
> >
> >
> >
> > Comments in os.hpp seem unchanged ?
> >
> > But looks fine otherwise. I do not need another webrev.
> >
> > Thanks, Thomas
> >
> >
> >
> >
> >
> >
> >
> >
> >
> > From: Thomas Stüfe [mailto:thomas.stuefe at gmail.com
> > <mailto:thomas.stuefe at gmail.com> ]
> > Sent: Thursday, August 17, 2017 3:48 PM
> > To: Lindenmaier, Goetz <goetz.lindenmaier at sap.com
> > <mailto:goetz.lindenmaier at sap.com> >
> > Cc: hotspot-runtime-dev at openjdk.java.net <mailto: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/ <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
> > <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
> > <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", 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
> > <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
> > <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-
> > <http://cr.openjdk.java.net/~goetz/wr17/8186072->
> > > dllBuildName/webrev.01/
> > <http://cr.openjdk.java.net/~goetz/wr17/8186072-
> > <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