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

David Holmes david.holmes at oracle.com
Tue Aug 29 10:14:13 UTC 2017


On 29/08/2017 8:11 PM, Lindenmaier, Goetz wrote:
> Hi,
> 
> Thomas, thanks for looking at the new webrev ad hoc.
> 
>>> 2) If the user provided buffer is too small, we will fail, which looks
>>> like the dll could not have been located. I am not sure we have to be
>>> shy with allocating memory - internally, we malloc a buffer for
>>> assembling the filename, and then os::splitpath() will malloc a whole
>>> bunch of arrays too. So I think we could just return the dll path
>>> location in a malloced buffer and require the caller to free.
>>
>> That seems a much bigger change. My question before pushing this is:
>> have we in any way reduced the size of path that we may accept on some
>> platforms? If we have that would be bad.
> No. I nowhere change the size of the buffer passed into this method,
> and that's the limit.

Thanks for confirming - I will push this now.

David
-----

> On posix, paths no more contain // where concatenated, so the
> paths supported can contain one more char in this case. This is an
> improvement.
> The buffer passed in usually has size MAX_PATH, so the checks for
> overflow should never cause a problem. They are mostly there
> for safety.  Allocation of the memory rather would reduce memory
> consumption, but I think it's not that relevant.  Overall, I think it's
> a mismatch that some functions allocate the memory, others
> require a buffer ... but that's really out of scope.
> 
> Best regards,
>    Goetz.
> 
> 
> 
> 
>>
>>> 3) I do not understand ':' as a file separator on windows. So, a path is
>>> allowed to contain e.g. "C:;" ? Which would mean "a path relative to the
>>> current directory currently active on drive C". If I am not mistaken. Do
>>> we want to support this, what is the use case?
>>
>> IIUC the existing windows code supports this. So yes c:foo.dll is a
>> reference to foo.dll in whatever the current directory on drive c: is.
>> As for a usecase ... perhaps a way to workaround long paths or
>> historical path format restrictions (ie no spaces) ? But the main thing
>> is to not break what we currently support.
>>
>> Thanks,
>> David
>>
>>> Kind Regards, Thomas
>>>
>>>
>>> On Tue, Aug 29, 2017 at 10:41 AM, David Holmes
>> <david.holmes at oracle.com
>>> <mailto:david.holmes at oracle.com>> wrote:
>>>
>>>      On 29/08/2017 6:29 PM, Lindenmaier, Goetz wrote:
>>>
>>>          Hi David,
>>>
>>>          I fixed the indentation and added you as reviewer.
>>>          I replaced the webrev in-place:
>>>          http://cr.openjdk.java.net/~goetz/wr17/8186072-
>> dllBuildName/webrev.05/
>>>          <http://cr.openjdk.java.net/~goetz/wr17/8186072-
>> dllBuildName/webrev.05/>
>>>          The new code went through all our testing ... except for some
>>>          ppc/s390 builds
>>>          that failed because of an other change pushed to hs tonight. But
>>>          that should
>>>          not matter, it all passed with the jdk10/jdk10 testing.
>>>
>>>          Would you mind sponsoring?
>>>
>>>
>>>      No problem - but can we get Thomas to sign-off on this latest
>>>      version please.
>>>
>>>
>>>      Thanks,
>>>      David
>>>
>>>          Best regards,
>>>              Goetz.
>>>
>>>              -----Original Message-----
>>>              From: David Holmes [mailto:david.holmes at oracle.com
>>>              <mailto:david.holmes at oracle.com>]
>>>              Sent: Dienstag, 29. August 2017 09:53
>>>              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: [ping] RFR(M): 8186072: dll_build_name returns
>>>              true even if file
>>>              is missing.
>>>
>>>              Hi Goetz,
>>>
>>>              On 29/08/2017 4:18 PM, Lindenmaier, Goetz wrote:
>>>
>>>                  Hi,
>>>
>>>                  this is a webrev with merged windows and posix
>>>                  implementations:
>>>                  http://cr.openjdk.java.net/~goetz/wr17/8186072-
>>>                  <http://cr.openjdk.java.net/~goetz/wr17/8186072->
>>>
>>>              dllBuildName/webrev.05/
>>>
>>>              I like the look of this.
>>>
>>>              There are a couple of indention nits in os.cpp:
>>>
>>>                  247 static bool conc_path_file_and_check(char *buffer, char
>>>              *printbuffer, size_t printbuflen,
>>>                  248                               const char* pname,
>>>              char lastchar,
>>>              const char* fname) {
>>>
>>>
>>>                  251   const char *filesep = (WINDOWS_ONLY(lastchar ==
>>>              ':' ||) lastchar
>>>              == os::file_separator()[0]) ?
>>>                  252                   "" : os::file_separator();
>>>
>>>
>>>              Thanks,
>>>              David
>>>
>>>                  Best regards,
>>>                       Goetz
>>>
>>>                      -----Original Message-----
>>>                      From: Lindenmaier, Goetz
>>>                      Sent: Montag, 28. August 2017 12:10
>>>                      To: 'David Holmes' <david.holmes at oracle.com
>>>                      <mailto:david.holmes at oracle.com>>
>>>                      Cc: hotspot-runtime-dev at openjdk.java.net
>>>                      <mailto:hotspot-runtime-dev at openjdk.java.net>
>>>                      Subject: RE: [ping] RFR(M): 8186072: dll_build_name
>>>                      returns true even if
>>>
>>>              file
>>>
>>>                      is missing.
>>>
>>>                      Hi,
>>>
>>>                      this are the changes needed to make the windows
>>>                      dll_locate_lib
>>>                      universally applicable. I also merge the three
>>>                      similar jio_snprintf
>>>                      calls into one method.
>>>                      I do some gymnastics to avoid another buffer of
>>>                      MAX_PATH_LEN
>>>                      at the first call to conc_path_file_and_check.
>>>                      I'll test this tonight.
>>>
>>>                      Best regards,
>>>                           Goetz.
>>>
>>>                      diff -r e09e4eb985c5 src/os/windows/vm/os_windows.cpp
>>>                      --- a/src/os/windows/vm/os_windows.cpp  Thu Aug 17
>>>                      17:26:02
>>>
>>>              2017
>>>
>>>                      +0200
>>>                      +++ b/src/os/windows/vm/os_windows.cpp  Mon Aug 28
>>>                      12:02:26
>>>
>>>              2017
>>>
>>>                      +0200
>>>                      @@ -1205,6 +1205,17 @@
>>>                            return GetFileAttributes(filename) !=
>>>                      INVALID_FILE_ATTRIBUTES;
>>>                          }
>>>
>>>                      +bool conc_path_file_and_check(char *buffer, char
>>>                      *printbuffer, size_t
>>>                      printbuflen,
>>>                      +                              const char* pname,
>>>                      char lastchar, const char* fname) {
>>>                      +  char *filesep = (WINDOWS_ONLY(lastchar == ':' ||)
>>>                      lastchar ==
>>>                      os::file_seperator()[0]) ? "" : os::file_separator();
>>>                      +  int ret = jio_snprintf(printbuffer, printbuflen,
>>>                      "%s%s%s", path, filesep,
>>>                      fullfname);
>>>                      +  if (ret != -1) {
>>>                      +    struct stat statbuf;
>>>                      +    return os::stat(buffer, &statbuf) == 0;
>>>                      +  }
>>>                      +  return false;
>>>                      +}
>>>                      +
>>>                          bool os::dll_locate_lib(char *buffer, size_t buflen,
>>>                                                  const char* pname, const
>>>                      char* fname) {
>>>                            bool retval = false;
>>>                      @@ -1220,11 +1231,8 @@
>>>                                if (p != NULL) {
>>>                                  const size_t plen = strlen(buffer);
>>>                                  const char lastchar = buffer[plen - 1];
>>>                      -        char *filesep = (lastchar == ':' ||
>>>                      lastchar == '\\') ? "" : "\\";
>>>                      -        int ret = jio_snprintf(&buffer[plen],
>>>                      buflen - plen, "%s%s", filesep,
>>>                      fullfname);
>>>                      -        if (ret != -1) {
>>>                      -          retval = file_exists(buffer);
>>>                      -        }
>>>                      +        retval = conc_path_file_and_check(buffer,
>>>                      &buffer[plen], buflen -
>>>                      plen,
>>>                      +                                          "",
>>>                      lastchar, fullfname);
>>>                                }
>>>                              } else if (strchr(pname,
>>>                      *os::path_separator()) != NULL) {
>>>                                int n;
>>>                      @@ -1238,12 +1246,8 @@
>>>                                      continue; // skip the empty path values
>>>                                    }
>>>                                    const char lastchar = path[plen - 1];
>>>                      -          char *filesep = (lastchar == ':' ||
>>>                      lastchar == '\\') ? "" : "\\";
>>>                      -          int ret = jio_snprintf(buffer, buflen,
>>>                      "%s%s%s", path, filesep,
>>>                      fullfname);
>>>                      -          if (ret != -1 && file_exists(buffer)) {
>>>                      -            retval = true;
>>>                      -            break;
>>>                      -          }
>>>                      +          retval = conc_path_file_and_check(buffer,
>>>                      buffer, buflen, path,
>>>                      lastchar, fullfname);
>>>                      +          if (retval) break;
>>>                                  }
>>>                                  // release the storage
>>>                                  for (int i = 0; i < n; i++) {
>>>                      @@ -1255,11 +1259,7 @@
>>>                                }
>>>                              } else {
>>>                                const char lastchar = pname[pnamelen-1];
>>>                      -      char *filesep = (lastchar == ':' || lastchar
>>>                      == '\\') ? "" : "\\";
>>>                      -      int ret = jio_snprintf(buffer, buflen,
>>>                      "%s%s%s", pname, filesep,
>>>                      fullfname);
>>>                      -      if (ret != -1) {
>>>                      -        retval = file_exists(buffer);
>>>                      -      }
>>>                      +      retval = conc_path_file_and_check(buffer,
>>>                      buffer, buflen, path,
>>>
>>>              lastchar,
>>>
>>>                      fullfname);
>>>                              }
>>>                            }
>>>
>>>
>>>                          -----Original Message-----
>>>                          From: David Holmes
>>>                          [mailto:david.holmes at oracle.com
>>>                          <mailto:david.holmes at oracle.com>]
>>>                          Sent: Montag, 28. August 2017 07:38
>>>                          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: [ping] RFR(M): 8186072:
>>>                          dll_build_name returns true even if
>>>
>>>                      file
>>>
>>>                          is missing.
>>>
>>>                          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-
>>>                              <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
>>>                                  <mailto:thomas.stuefe at gmail.com>]
>>>                                  Sent: Dienstag, 22. August 2017 17:30
>>>                                  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.
>>>
>>>                                  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>
>>>                                  <mailto: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-
>>>                                  <http://cr.openjdk.java.net/~goetz/wr17/8186072->
>>>                                  dllBuildName/webrev.04
>>>
>>>                          <http://cr.openjdk.java.net/~goetz/wr17/8186072-
>>>                          <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>
>>>                                  <mailto: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>
>>>                                  <mailto:hotspot- <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,
>>>                                           >
>>>                                           > could I please get a second
>>>                                  review?
>>>                                           >
>>>                                  http://cr.openjdk.java.net/~goetz/wr17/8186072-
>> dllBuildName-
>>>                                  <http://cr.openjdk.java.net/~goetz/wr17/8186072-
>> dllBuildName->
>>>                                  hs/webrev.04
>>>                                  <http://cr.openjdk.java.net/~goetz/wr17/8186072-
>>>                                  <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>
>>>                                  <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>
>>>                                  <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- <mailto:hotspot->
>>>                                  runtime-dev at openjdk.java.net
>>>                                  <mailto: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>>
>>>
>>>                          <mailto: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->
>>>                                  <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->
>>>                                  <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->
>>>                                  <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->
>>>                                  <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>>
>>>                                           > >
>>>                                  <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>>
>>>                                           > >
>>>                                  <mailto: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- <mailto:hotspot->
>>>                                  runtime-dev at openjdk.java.net
>>>                                  <mailto:runtime-dev at openjdk.java.net>>
>>>                                  <mailto:hotspot-runtime-
>>>                                  <mailto:hotspot-runtime->
>>>
>>>                          <mailto:hotspot- <mailto:hotspot->
>>>
>>>                                  runtime->
>>>                                           > > dev at openjdk.java.net
>>>                                  <mailto:dev at openjdk.java.net>
>>>                                  <mailto: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>>
>>>
>>>                          <mailto: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->
>>>                                  <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->
>>>                                  <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->
>>>                                  <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->
>>>                                  <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->
>>>                                  <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->
>>>                                  <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->
>>>                                  <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.ht
>>>                          <http://os_posix.cpp.udiff.ht>ml
>>>
>>>                                           > >
>>>                                  <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.02/src/os/posix/vm/os_posix.cpp.udiff.ht
>>>                          <http://os_posix.cpp.udiff.ht>ml>
>>>
>>>                                           > >
>>>                                           > >
>>>                                           > >
>>>                                           > >     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->
>>>                                  <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->
>>>                                  <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->>
>>>                                           > >
>>>                                  <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->>
>>>                                           > >
>>>                                  <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