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

Thu Aug 17 17:54:02 UTC 2017

Hi Goetz,

On Thu, Aug 17, 2017 at 6:03 PM, Lindenmaier, Goetz <
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/
>
> incremental diff:
>
> 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]
> *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> 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", 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
>
>
>
>
>