Merge pull request bernardladenthin#260 from vaiju1981/fix/loader-extraction-race

fix(loader): make native-lib extraction race-safe (atomic write + con…

Author: bernardladenthin

TODO.md

@@ -26,12 +26,19 @@ read-confirmed by the audit but warrant a quick re-check before the fix.
 doc) **DONE** — verified (452/452 C++ tests, affected Java tests + new regressions, Javadoc + Spotless +
 clang-format 22.1.5 clean).
 
-**Deferred — `LlamaLoader` native-lib extraction temp-path race (open).** Fixing it (per-process temp
-dir / atomic move) is the one audit item left undone: it sits on *the* native-library load path, so the
-change has a high blast radius and needs careful Windows / `deleteOnExit` / `cleanup()` co-design plus
-cross-platform load testing (a locked-DLL replace on Windows, leftover unique-dir accumulation) that the
-fix-batch could not safely exercise. The race only bites concurrent JVMs sharing one `java.io.tmpdir`
-(e.g. some CI matrices). Pick it up as its own change with a Windows test pass.
+**`LlamaLoader` native-lib extraction temp-path race — DONE (atomic write + content-reuse).**
+`extractFile` now (1) reuses a byte-identical existing copy instead of rewriting it — so it never
+replaces a file another JVM has already loaded (which fails on Windows) — and (2) otherwise extracts
+to a per-attempt unique temp file and **atomically moves** it into place, so a concurrent loader can
+never observe a half-written library. `jllama` is statically linked (`BUILD_SHARED_LIBS OFF`), so the
+extracted file is self-contained — no multi-DLL co-location to coordinate. Verified by the
+`NativeLibraryLoadSmokeTest` (real extract+load on macOS) + a `resourceMatchesFile` unit test; the
+Windows locked-replace path is exercised by CI's Windows jobs.
+
+*Optional follow-up (lower priority):* full per-process extraction **directory** isolation + a
+`cleanup()` that recursively removes dead-process dirs. Now that writes are atomic and content-checked,
+this is a tidiness improvement (stops the shared-tmpdir `cleanup()` from racing a live peer's flat
+file), not a correctness fix — and it still needs the Windows locked-file co-design noted before.
 
 **Tier 1 — high impact, fix first**
 

src/main/java/net/ladenthin/llama/loader/LlamaLoader.java

@@ -210,33 +210,40 @@ public static boolean loadNativeLibrary(Path path) {
         Path extractedFilePath = Paths.get(targetDirectory, fileName);
 
         try {
-            // Extract a native library file into the target directory
-            try (InputStream reader = LlamaLoader.class.getResourceAsStream(nativeLibraryFilePath)) {
-                if (reader == null) {
-                    return null;
-                }
-                Files.copy(reader, extractedFilePath, StandardCopyOption.REPLACE_EXISTING);
-            } finally {
-                // Delete the extracted lib file on JVM exit.
+            // Fast path: a byte-identical copy already exists — extracted by a previous run or by a
+            // concurrent JVM sharing this tmpdir. Reuse it rather than rewriting in place: replacing a
+            // file another process has already loaded fails on Windows (the lib is locked), and an
+            // in-place rewrite risks a partial file a concurrent loader could observe.
+            if (Files.exists(extractedFilePath) && resourceMatchesFile(nativeLibraryFilePath, extractedFilePath)) {
+                permissionSetter.apply(extractedFilePath.toFile());
                 extractedFilePath.toFile().deleteOnExit();
+                return extractedFilePath;
             }
 
-            // Set executable (x) flag to enable Java to load the native library
-            permissionSetter.apply(extractedFilePath.toFile());
-
-            // Check whether the contents are properly copied from the resource folder
-            try (InputStream nativeIn = LlamaLoader.class.getResourceAsStream(nativeLibraryFilePath);
-                    InputStream extractedLibIn = Files.newInputStream(extractedFilePath)) {
-                if (nativeIn == null) {
-                    System.err.println(String.format("Native library resource missing at %s", nativeLibraryFilePath));
-                    return null;
+            // Otherwise extract into a per-attempt unique temp file, verify it, then atomically move it
+            // into place so a concurrent loader never observes a half-written library.
+            Path tempFile = Files.createTempFile(Paths.get(targetDirectory), fileName + ".", ".tmp");
+            try {
+                try (InputStream reader = LlamaLoader.class.getResourceAsStream(nativeLibraryFilePath)) {
+                    if (reader == null) {
+                        return null;
+                    }
+                    Files.copy(reader, tempFile, StandardCopyOption.REPLACE_EXISTING);
                 }
-                if (!contentsEquals(nativeIn, extractedLibIn)) {
+                if (!resourceMatchesFile(nativeLibraryFilePath, tempFile)) {
                     System.err.println(String.format("Failed to write a native library file at %s", extractedFilePath));
                     return null;
                 }
+                moveIntoPlace(tempFile, extractedFilePath);
+            } finally {
+                // No-op once moveIntoPlace consumed it; cleans up if any step above bailed out.
+                Files.deleteIfExists(tempFile);
             }
 
+            // Set executable (x) flag to enable Java to load the native library.
+            permissionSetter.apply(extractedFilePath.toFile());
+            extractedFilePath.toFile().deleteOnExit();
+
             System.out.println("Extracted '" + fileName + "' to '" + extractedFilePath + "'");
             return extractedFilePath;
         } catch (IOException e) {
@@ -245,6 +252,29 @@ public static boolean loadNativeLibrary(Path path) {
         }
     }
 
+    /**
+     * Atomically replace {@code target} with {@code source}, falling back to a plain move when the
+     * filesystem does not support atomic moves.
+     */
+    private static void moveIntoPlace(Path source, Path target) throws IOException {
+        try {
+            Files.move(source, target, StandardCopyOption.REPLACE_EXISTING, StandardCopyOption.ATOMIC_MOVE);
+        } catch (IOException atomicUnsupported) {
+            Files.move(source, target, StandardCopyOption.REPLACE_EXISTING);
+        }
+    }
+
+    /** Whether the classpath resource at {@code resourcePath} is byte-identical to {@code file}. */
+    static boolean resourceMatchesFile(String resourcePath, Path file) throws IOException {
+        try (InputStream resource = LlamaLoader.class.getResourceAsStream(resourcePath);
+                InputStream onDisk = Files.newInputStream(file)) {
+            if (resource == null) {
+                return false;
+            }
+            return contentsEquals(resource, onDisk);
+        }
+    }
+
     /**
      * Extracts and loads the specified library file to the target folder
      *

src/test/java/net/ladenthin/llama/loader/LlamaLoaderTest.java

@@ -99,6 +99,18 @@ public void testContentsEqualsIdenticalContent() throws IOException {
         assertTrue(LlamaLoader.contentsEquals(new ByteArrayInputStream(data), new ByteArrayInputStream(data)));
     }
 
+    @Test
+    public void resourceMatchesFileFalseWhenResourceAbsent() throws IOException {
+        java.nio.file.Path tmp = java.nio.file.Files.createTempFile("llama-loader-test", ".bin");
+        try {
+            java.nio.file.Files.write(tmp, new byte[] {1, 2, 3});
+            // A missing classpath resource must compare as "not matching", never throw.
+            assertFalse(LlamaLoader.resourceMatchesFile("/net/ladenthin/llama/does-not-exist.bin", tmp));
+        } finally {
+            java.nio.file.Files.deleteIfExists(tmp);
+        }
+    }
+
     @Test
     public void testContentsEqualsBothEmpty() throws IOException {
         assertTrue(LlamaLoader.contentsEquals(