feat(loader): bundle native library inside the JAR - #77
andygrove merged 2 commits into
Conversation
Sorry, something went wrong.
Rework NativeLibraryLoader so it can load datafusion_jni from a
JAR-bundled native library instead of requiring java.library.path:
1. Try System.loadLibrary so operators can still override with a
system-installed build via java.library.path / LD_LIBRARY_PATH.
2. On UnsatisfiedLinkError, detect the host OS/arch, look up the
bundled resource at
org/apache/datafusion/<os>/<arch>/lib<name>.<ext>, extract it to
$TMPDIR/datafusion-java/<sha256>/ and load via System.load.
Concurrent JVMs sharing the temp directory converge on the same SHA-256
hash directory; the extraction uses ATOMIC_MOVE so racing writers don't
clobber each other.
Wire core/pom.xml to copy the host's locally built native lib from
native/target/<profile>/lib<name>.<ext> into target/classes at the
matching resource path, so the produced JAR works out of the box on the
build host. Per-platform Maven profiles set the OS/arch directory
segments and library filename; -Ddatafusion.native.profile=release
switches the copy source from debug to release.
Drop -Djava.library.path from the surefire and exec-maven-plugin
argLines so the test path now exercises the same resource-extraction
code path users will hit.
Refs apache#33 (work items 1 and 3); cross-build CI, Sonatype publishing, and
release docs remain as follow-ups.
| <property name="datafusion.native.lib.source" | ||
| value="${maven.multiModuleProjectDirectory}/native/target/${datafusion.native.profile}/${datafusion.lib.filename}"/> | ||
| <fail message="Native library not found at ${datafusion.native.lib.source}. Run 'cd native && cargo build' (or 'make') before building the JAR."> | ||
| <condition><not><available file="${datafusion.native.lib.source}"/></not></condition> | ||
| </fail> |
There was a problem hiding this comment.
On Windows, none of the new native profiles defines datafusion.lib.filename, datafusion.lib.os, or datafusion.lib.arch, so this path remains unresolved and the new process-classes fail check aborts mvn test/package even after cargo build creates native/target/debug/datafusion_jni.dll. This regresses the previous java.library.path flow before NativeLibraryLoader can fall back to System.loadLibrary. One fix is adding a Windows profile mirroring the Linux/Mac ones: <datafusion.lib.os>windows</datafusion.lib.os>, <datafusion.lib.arch>amd64</datafusion.lib.arch>, <datafusion.lib.filename>datafusion_jni.dll</datafusion.lib.filename>. Even though no Windows binary will be bundled today, the build at least stops failing, or skipping the copy step on unsupported platforms.
Sorry, something went wrong.
There was a problem hiding this comment.
Thanks. I have addressed this.
Sorry, something went wrong.
| private static Path extractToTempDir(String resource, String fileName) throws IOException { | ||
| Path tmpRoot = Files.createDirectories( | ||
| Paths.get(System.getProperty("java.io.tmpdir"), TMP_DIR_NAME)); | ||
| Path staging = Files.createTempFile(tmpRoot, fileName + ".", ".part"); | ||
|
|
||
| String hash; | ||
| try (InputStream raw = NativeLibraryLoader.class.getResourceAsStream(resource); | ||
| DigestInputStream in = new DigestInputStream(raw, sha256()); | ||
| OutputStream out = Files.newOutputStream(staging)) { | ||
| in.transferTo(out); | ||
| hash = toHex(in.getMessageDigest().digest()); | ||
| } catch (IOException e) { | ||
| Files.deleteIfExists(staging); | ||
| throw e; | ||
| } | ||
|
|
||
| Path versionedDir = Files.createDirectories(tmpRoot.resolve(hash)); | ||
| Path target = versionedDir.resolve(fileName); | ||
|
|
||
| if (Files.exists(target) && Files.size(target) == Files.size(staging)) { | ||
| Files.deleteIfExists(staging); | ||
| return target; | ||
| } | ||
|
|
||
| try { | ||
| Files.move(staging, target, StandardCopyOption.ATOMIC_MOVE); | ||
| } catch (FileAlreadyExistsException e) { | ||
| // Another JVM extracted the same content while we were writing. | ||
| // Their copy is identical (same SHA-256), so discard ours. | ||
| Files.deleteIfExists(staging); | ||
| } catch (IOException e) { | ||
| // Atomic move not supported on this filesystem. Fall back to a | ||
| // replacement move; the hash directory guarantees content equality. | ||
| try { | ||
| Files.move(staging, target, StandardCopyOption.REPLACE_EXISTING); | ||
| } catch (IOException retry) { | ||
| Files.deleteIfExists(staging); | ||
| throw retry; | ||
| } | ||
| } | ||
| return target; |
There was a problem hiding this comment.
I am far from a security expert, but I think this allows for an attack where the JVM loads a malicious library rather than the intended one. If the attacker knows the hash and the size of the native library, they can create the temp dir based on the hash and the malicious lib with the right size, and this code will load it. I believe checking the hash of the actual library too would prevent that.
Maybe it's not an attack surface we care about, but since this library might be used pretty generally at some point, I figured I'd mentioned with approval.
Sorry, something went wrong.
There was a problem hiding this comment.
Good feedback thanks. I have addressed.
Sorry, something went wrong.
Address PR apache#77 review feedback: - NativeLibraryLoader extracts to a per-JVM private temp directory via Files.createTempDirectory (mode 0700 on POSIX, user-private on Windows) instead of a shared $TMPDIR/datafusion-java/<sha256>/ tree. Removes the size-only existence check that would have let another user plant a same-sized library at the predictable hash path. - Add native-windows-amd64 Maven profile so datafusion.lib.os/arch/filename are defined on Windows; otherwise the process-classes copy step would abort the build even when the loader could fall back to System.loadLibrary. - Map Windows x86_64 -> amd64 in Platform.detectArch to match the new profile and Linux convention; extend PlatformTest accordingly.
Which issue does this PR close?
Rationale for this change
NativeLibraryLoader currently calls System.loadLibrary("datafusion_jni"), so consumers need to set java.library.path or install the .so/.dylib system-wide before importing the library. That's a barrier to publishing a Maven-Central artifact that just works.
This PR adds the runtime piece of the fat-JAR design from #33: the loader can pull a platform-specific native library out of the JAR's resource tree, and the Maven build copies the locally-built datafusion_jni into that tree before packaging.
What changes are included in this PR?
NativeLibraryLoader:
Maven (core/pom.xml):
Are these changes tested?
Other host platforms (linux/amd64, linux/aarch64, darwin/x86_64) will be exercised once the cross-build CI matrix lands.
Are there any user-facing changes?
The NativeLibraryLoader.loadLibrary() public API is unchanged. Behavior is strictly broader — code that previously needed java.library.path set still works, and code that depended only on the bundled lib now works too.