[ping] RFR(M): 8186072: dll_build_name returns true even if file is missing.
David Holmes
david.holmes at oracle.com
Mon Aug 28 05:37:56 UTC 2017
Hi Goetz,
On 25/08/2017 12:19 AM, Lindenmaier, Goetz wrote:
> Hi,
>
> I please need a second review and a sponsor:
> http://cr.openjdk.java.net/~goetz/wr17/8186072-dllBuildName/webrev.04
>
> To update my description of the change to the status after Thomas' review:
>
> dll_build_name builds the proper path to a library given a list of paths separated by
> path_seperator and a library name. It adds in the platform specific endings etc.
> It is documented to return whether the file exists, but only does so if a path_seperator
> exists in the path.
> Especially if the path is empty, it just returns ‘true’ without checking.
>
> 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.
>
> I renamed dll_build_name to dll_locate_lib and changed it's behavior to always return
> a full path to the lib, inserting current working directory if no path is given.
> For the use case where "" was actually passed to the function, I added a new function
> (reusing the old function name) dll_build_name that just adds system dependent prefix and suffix
> to the name.
> I merged all unix implementations to the posix os branch.
I started to look at this and have applied the patch to run through some
basic testing. The overall approach seems reasonable. But it is hard to
track all the details - in particular whether there were any subtle
differences across the "posix" systems?
I'm wondering what, if any, significant differences exist between the
Windows and POSIX versions? I would hope the platform differences could
easily be hidden behind macros (for path separator, library suffix etc).
Then perhaps this could just go in shared code (os.hpp, os.cpp)?
That aside, in the Windows code shouldn't the hardwired .dll strings
actually be JNI_LIB_SUFFIX?
Thanks,
David
> Best regards,
> Goetz.
>
>
>
>> -----Original Message-----
>> From: Thomas Stüfe [mailto:thomas.stuefe at gmail.com]
>> Sent: Dienstag, 22. August 2017 17:30
>> 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.
>>
>> Looks good.
>>
>> ..Thomas
>>
>> On Tue, Aug 22, 2017 at 4:33 PM, Lindenmaier, Goetz
>> <goetz.lindenmaier at sap.com <mailto:goetz.lindenmaier at sap.com> > wrote:
>>
>>
>> I mistyped the path to webrev, this should work:
>> http://cr.openjdk.java.net/~goetz/wr17/8186072-
>> dllBuildName/webrev.04 <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
>> <mailto:thomas.stuefe at gmail.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,
>> >
>> > could I please get a second review?
>> > http://cr.openjdk.java.net/~goetz/wr17/8186072-dllBuildName-
>> hs/webrev.04 <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
>> <mailto:thomas.stuefe at gmail.com> ]
>> > > Sent: Donnerstag, 17. August 2017 19:54
>> > > 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 6:03 PM, Lindenmaier, Goetz
>> > > <goetz.lindenmaier at sap.com
>> <mailto:goetz.lindenmaier at sap.com> <mailto: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-
>> <http://cr.openjdk.java.net/~goetz/wr17/8186072->
>> > > dllBuildName/webrev.03/
>> <http://cr.openjdk.java.net/~goetz/wr17/8186072-
>> <http://cr.openjdk.java.net/~goetz/wr17/8186072->
>> > > dllBuildName/webrev.03/>
>> > >
>> > > incremental diff:
>> > >
>> > > http://cr.openjdk.java.net/~goetz/wr17/8186072-
>> <http://cr.openjdk.java.net/~goetz/wr17/8186072->
>> > > dllBuildName/webrev.03/diffs-incremental.patch
>> > > <http://cr.openjdk.java.net/~goetz/wr17/8186072-
>> <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>
>> > > <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>
>> > > <mailto: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> <mailto:hotspot-runtime- <mailto:hotspot-
>> runtime->
>> > > dev at openjdk.java.net <mailto: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> <mailto: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-
>> <http://cr.openjdk.java.net/~goetz/wr17/8186072->
>> > > dllBuildName/webrev.02/
>> <http://cr.openjdk.java.net/~goetz/wr17/8186072-
>> <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-
>> <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-
>> <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-
>> <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-
>> <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-
>> <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-
>> <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-
>> <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-
>> <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->
>> > > <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->
>> > > <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