Load bundled HDF core libraries before the JNI wrappers initialize - #480
Load bundled HDF core libraries before the JNI wrappers initialize#480mattjala wants to merge 2 commits into
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
HDFView sometimes fails to start when another HDF5 or HDF4 installation is visible to the operating system's dynamic loader. The workaround so far has been to require users to hand-edit their PATH to avoid detection of the pre-existing libraries. The cause is that two native libraries load by two different mechanisms. The JNI wrapper (hdf5_java) is loaded by the JVM and honors java.library.path, so our packaging already pins it correctly. But the wrapper lists the core library (libhdf5.so.320 on Linux, hdf5.dll on Windows) as a NEEDED dependency, and that one is resolved by the OS loader rather than the JVM. On Linux the loader checks LD_LIBRARY_PATH before the RUNPATH we ship, and on Windows the search order includes PATH. Either way a foreign copy can be detected first. This change loads the bundled core libraries by absolute path before anything touches the wrapper. Once a library is mapped into the process, the loader satisfies the wrapper's dependency from the already-mapped copy, matching on soname on Linux and on module base name on Windows, and never searches PATH or LD_LIBRARY_PATH at all. NativeLibraryLoader looks for each library in -Dhdfview.nativedir, then -Dhdfview.root (jpackage already sets this to $APPDIR), then the directory named by -Dhdf.hdf5lib.H5.hdf5lib, then each entry of java.library.path. The call sits at the top of FileFormat's static initializer, which is the earliest point the wrapper can initialize.
3a3312c to
04dc9a8
Compare
| // Best-effort pin to the bundled core libraries before any JNI wrapper initializes. | ||
| // Failures fallback to the default library loading path. | ||
| NativeLibraryLoader.preloadHDF5(); | ||
| NativeLibraryLoader.preloadHDF4(); |
There was a problem hiding this comment.
This seems like a solution at the wrong level. HDFView shouldn't need to know anything about how to look for and load the native libraries, it should only be concerned with using the object library and, consequently, the JNI wrappers. The JNI wrappers already perform the loading of HDF5 and HDF4 and all library loading logic should stay there only, as they're the primary consumers of those libraries and directly coupled to them. It's possible there's an issue with the loading logic there, but that seems unlikely since the logic there is basically exactly the same as in this PR.
To me it seems like this is an obvious problem with how either jpackage or another mechanism of the bundling process chooses to setup the order in which libraries are looked for.
There was a problem hiding this comment.
It doesn't look like the JNI wrappers deal with this particular problem. The instances of loading in H5.loadH5Lib() (lines 334, 355, 380) all seem to deal with loading the JNI wrappers, not the underlying C library, which is handled at link/load time outside the JVM. So this PR doesn't seem to be duplicating any logic from HDF5.
That said, there are platform-specific workarounds we can use on HDFView's side to pin the bundled libraries without HDFView carrying the library-loading logic.
On Linux, this is partly a regression. Before 3.4.0 the bundles shipped the JNI wrapper with DT_RPATH, and current builds ship DT_RUNPATH. DT_RPATH is searched before LD_LIBRARY_PATH and DT_RUNPATH is searched after, so the bundled copy used to be found first but now is not. This doesn't seem to have been a deliberate change, and we could get the old behavior back by patching the bundled libraries at package time (patchelf --force-rpath) or linking with --disable-new-dtags. Probably worth doing either way.
On Mac I haven't checked what the bundled dylibs actually look like, but it would presumably need the equivalent install-name/rpath treatment.
On Windows, jpackage's --app-content could probably be used to place the DLLs at the app image root, which would be searched ahead of the system PATH.
However, we could also keep the preloading pattern as it is here and simply move it upstream into HDF5/HDF4. That would take the library-loading logic out of HDFView and give us one mechanism that behaves the same on all three platforms.
There was a problem hiding this comment.
It doesn't look like the JNI wrappers deal with this particular problem. The instances of loading in H5.loadH5Lib() (lines 334, 355, 380) all seem to deal with loading the JNI wrappers, not the underlying C library, which is handled at link/load time outside the JVM. So this PR doesn't seem to be duplicating any logic from HDF5.
Those static blocks load the JNI wrapper libraries (libhdf5_java.so on Linux for the HDF5 JNI wrapper library), but those necessarily have a dependency on the HDF5/HDF4 libraries, which will get loaded as well. A Java application should never have to worry about directly loading either the JNI wrapper libraries or the HDF5/HDF4 libraries, it should only worry about including the H5.java/HDFLibrary.java Java classes and letting them do all the work. The author compiling the application may need to help the JVM find the native libraries for loading (-Dhdf.hdf5lib.H5.loadLibraryName=, -Dhdf.hdf5lib.H5.hdf5lib), but that should be it and isn't usually necessary.
As this has only become a problem with the jpackage bundling, I suspect this is entirely a problem with the packaging approach.
On Linux, this is partly a regression. Before 3.4.0 the bundles shipped the JNI wrapper with DT_RPATH, and current builds ship DT_RUNPATH. DT_RPATH is searched before LD_LIBRARY_PATH and DT_RUNPATH is searched after, so the bundled copy used to be found first but now is not. This doesn't seem to have been a deliberate change, and we could get the old behavior back by patching the bundled libraries at package time (patchelf --force-rpath) or linking with --disable-new-dtags. Probably worth doing either way.
For a bundled app, I suspect using rpaths is the appropriate way to go here. LD_LIBRARY_PATH could be feasible, but I would generally try to avoid that. As runpath is the lowest priority among these three, it does seem to be an inappropriate choice. I would definitely avoid using patchelf as a strategy since it's a bit hack to do so, but if it has to be used as a last resort let's discuss.
On Mac I haven't checked what the bundled dylibs actually look like, but it would presumably need the equivalent install-name/rpath treatment.
On Windows, jpackage's --app-content could probably be used to place the DLLs at the app image root, which would be searched ahead of the system PATH.
However, we could also keep the preloading pattern as it is here and simply move it upstream into HDF5/HDF4. That would take the library-loading logic out of HDFView and give us one mechanism that behaves the same on all three platforms.
I'd like to avoid any sort of logic addition, if possible, since the approach we have now for loading libraries has worked "correctly" for a long time and I suspect we just need to alter our packaging a little bit. One hacky workaround approach for the bundles might be to set hdf.hdf5lib.H5.loadLibraryName/hdf.hdflib.HDFLibrary.loadLibraryName to bogus values and set hdf.hdf5lib.H5.hdf5lib/hdf.hdflib.HDFLibrary.hdflib to the paths of the bundled libs to force the library to fallback to loading the JNI libraries according to a specified full path. Of course this may have some issues with relocatability of an HDFView install, so ideally we can implement rpath-like solutions with relative paths.
The loading logic is now addressed per platform where the bundle is assembled, and HDFView itself carries no library-loading logic. Linux: The new pin-rpath.py script refiles the HDF binaries' existing search path under DT_RPATH instead of DT_RUNPATH, so its consulted before LD_LIBRARY_PATH. Windows: The core DLLs move beside HDFView.exe, whose directory is searched ahead of both the system folder and PATH. macOS: No specific extra work needed, and the bundled libraries are already found first. PATH and LD_LIBRARY_PATH should no longer redirect a packaged HDFView to a different HDF build.
3 participants
HDFView sometimes fails to start when another HDF5 or HDF4 installation is visible to the operating system's dynamic loader. The workaround so far has been to require users to hand-edit their PATH to avoid detection of the pre-existing libraries.
The cause is that two native libraries load by two different mechanisms. The JNI wrapper (hdf5_java) is loaded by the JVM and honors java.library.path, so our packaging already pins it correctly. But the wrapper lists the core library (libhdf5.so.320 on Linux, hdf5.dll on Windows) as a NEEDED dependency, and that one is resolved by the OS loader rather than the JVM. On Linux the loader checks LD_LIBRARY_PATH before the RUNPATH we ship, and on Windows the search order includes PATH. Either way a foreign copy can be detected first.
This change loads the bundled core libraries by absolute path before anything touches the wrapper. Once a library is mapped into the process, the loader satisfies the wrapper's dependency from the already-mapped copy, matching on soname on Linux and on module base name on Windows, and never searches PATH or LD_LIBRARY_PATH at all.
NativeLibraryLoader looks for each library in -Dhdfview.nativedir, then -Dhdfview.root (jpackage already sets this to $APPDIR), then the directory named by -Dhdf.hdf5lib.H5.hdf5lib, then each entry of java.library.path. The call sits at the top of FileFormat's static initializer, which is the earliest point the wrapper can initialize.