Incorrect return value for Java_jdk_internal_loader_NativeLibraries_load() in java.base/share/native/libjava/NativeLibraries.c

[Mandy Chung]

This is not an issue and does not affect the runtiime.  When it fails to 
load a native library, the VM will throw UnsatisfiedLinkError (see 
JVM_LoadLibrary).   The return value is only important for the JNI case 
and calling System::loadLibrary on the system with dynamic linker cache 
(Big Sur) since throwExceptionIfFail == false.

I agree that the JNI_TRUE return value for non-JNI case (i.e. raw 
library library mechanism) is confusing to the readers.  We should fix 
it to return handle != 0L to match the specified return value (true only 
when succeed).

I will file an issue.

Mandy

On 2/23/22 4:08 PM, Cheng Jin wrote:
> Hi there,
>
> The return value from Java_jdk_internal_loader_NativeLibraries_load() athttps://github.com/openjdk/jdk/blob/master/src/java.base/share/native/libjava/NativeLibraries.c
> seems incorrect according to the code logic in there.
>
> Looking at the calling path from  loadLibrary() to load() at src/java.base/share/classes/jdk/internal/loader/NativeLibraries.java as follows:
>
>      public NativeLibrary loadLibrary(Class<?> fromClass, String name) {
>          ...
>          NativeLibrary lib = findFromPaths(LibraryPaths.SYS_PATHS, fromClass, name); <----------
>          if (lib == null && searchJavaLibraryPath) {
>              lib = findFromPaths(LibraryPaths.USER_PATHS, fromClass, name); <----
>          }
>          return lib;
>      }
>
>        private NativeLibrary findFromPaths(String[] paths, Class<?> fromClass, String name) {
>          for (String path : paths) {  <---------------- keep searching the library paths till the library is located at the correct lib path
>              File libfile = new File(path, System.mapLibraryName(name));
>              NativeLibrary nl = loadLibrary(fromClass, libfile);  ?
>              if (nl != null) {
>                  return nl; <--------------------
>              }
>              libfile = ClassLoaderHelper.mapAlternativeName(libfile);
>              if (libfile != null) {
>                  nl = loadLibrary(fromClass, libfile);
>                  if (nl != null) {
>                      return nl;
>                  }
>              }
>          }
>          return null;
> }
>
>      public NativeLibrary loadLibrary(Class<?> fromClass, File file) {
>         // Check to see if we're attempting to access a static library
>          String name = findBuiltinLib(file.getName());
>          boolean isBuiltin = (name != null);
>          ...
>          return loadLibrary(fromClass, name, isBuiltin);  <------------
>      }
>
>     private NativeLibrary loadLibrary(Class<?> fromClass, String name, boolean isBuiltin) {
>     ...
>              NativeLibraryImpl lib = new NativeLibraryImpl(fromClass, name, isBuiltin, isJNI);
>
>                         // load the native library
>              NativeLibraryContext.push(lib);
>              try {
>                  if (!lib.open()) {  <--------------------- call load()
>                      return null;    // fail to open the native library
>                  }
>               ...
>             // register the loaded native library
>              loadedLibraryNames.add(name);
>              libraries.put(name, lib);
>              return lib;  <--------------- return an invalid lib as lib.open() returns true
>          } finally {
>              releaseNativeLibraryLock(name);
>          }
>    ...
>    }
>
>          /*
>           * Loads the named native library
>           */
>          boolean open() {
>              ...
>              return load(this, name, isBuiltin, isJNI, loadLibraryOnlyIfPresent);
>          }
>
>      private static native boolean load(NativeLibraryImpl impl, String name, boolean isBuiltin, boolean isJNI, boolean throwExceptionIfFail);
>
> and then native code in java.base/share/native/libjava/NativeLibraries.c
>
> Java_jdk_internal_loader_NativeLibraries_load
>    (JNIEnv *env, jobject this, jobject lib, jstring name,
>     jboolean isBuiltin, jboolean isJNI, jboolean throwExceptionIfFail)
> {
>      const char *cname;
>      jint jniVersion;
>      jthrowable cause;
>      void * handle;
>      jboolean loaded = JNI_FALSE;
>     ...
>      handle = isBuiltin ? procHandle : JVM_LoadLibrary(cname, throwExceptionIfFail);
>      if (isJNI) {
>         ...
>      }
>      (*env)->SetLongField(env, lib, handleID, ptr_to_jlong(handle));
>      loaded = JNI_TRUE;  <----- always return true when isJNI is false and a null handle
>
> done:
>      JNU_ReleaseStringPlatformChars(env, name, cname);
>      return loaded;
> }
>
> Assuming there are multiple library paths existing in the jdk/lib which are added to sun.boot.library.path and java.library.path
> and the expected library (libnative.so) is only placed in jdk/lib. e.g.
> lib/libA
> lib/libB
> lib/libC
> lib/libnative.so
>
> when the code in findFromPaths() searches the library paths set in sun.boot.library.path and java.library.path starting from lib/LibA,
> it will end up with an invalid value if load() returns true in the case of a false isJNI and a null handle returned from JVM_LoadLibrary()
> as the library doesn't exist in lib/libA passed to JVM_LoadLibrary().
>
> To ensure findFromPaths() keeps searching the remaining lib paths,  the simple fix should look like:
> Java_jdk_internal_loader_NativeLibraries_load( ...)
> {
> ...
>      (*env)->SetLongField(env, lib, handleID, ptr_to_jlong(handle));
>      if (handle) {  <------------------------- return true only for a non-null handle
>          loaded = JNI_TRUE;
>      }
> ...
> }
>
> Please help check the code around there to see whether the fix is reasonable to avoid the unexpected situation as explained above (the problem was spotted since JDK17)
>
> Best Regards
> Cheng Jin