[GLUTEN-12401][CORE] Use Path.toRealPath() for JniLibLoader symlink resolution#12402
Merged
Merged
Conversation
…lPath() for correct relative-symlink resolution and cycle detection
|
Run Gluten Clickhouse CI on x86 |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes JniLibLoader.toRealPath() symlink handling in gluten-core by delegating to the JDK’s Path#toRealPath(), ensuring correct resolution of relative symlink targets and proper cycle detection (instead of a hand-rolled loop that could mis-resolve paths or hang).
Changes:
- Replaced manual symlink-walking logic with
Paths.get(libPath).toRealPath()and narrowed exception handling fromThrowabletoException(lettingErrorpropagate). - Widened
toRealPathvisibility to package-private to enable direct unit testing. - Added JUnit tests covering canonicalization through a symlinked parent directory, relative-target symlink chains, and symlink cycles (with environment-aware skipping when symlinks aren’t supported).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| gluten-core/src/main/java/org/apache/gluten/jni/JniLibLoader.java | Delegates real-path resolution to Path#toRealPath() and updates visibility/exception handling accordingly. |
| gluten-core/src/test/java/org/apache/gluten/jni/JniLibLoaderTest.java | Adds regression tests for relative symlink resolution and cycle detection, with a symlink capability probe. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
Author
|
cc @jackylee-ch |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
JniLibLoader.toRealPath()previously walked symbolic links with a hand-rolled loop that calledFiles.readSymbolicLinkand fed the stored (often relative) target back intoPaths.get(...). That resolved relative targets against the process working directory instead of the link's own parent, and the loop had no cycle guard. This PR rewrites the method to delegate toPath#toRealPathso symlink-chain resolution (including./..normalisation) and cycle detection (FileSystemLoopExceptionon Linux;FileSystemExceptioncarrying "Too many levels of symbolic links" on current openjdk macOS) are handled by the JDK. Visibility is widened fromprivateto package-private so a unit test in the same package can call it directly. The caught type isException(notThrowable), wrappingIOException/InvalidPathException/SecurityException/NullPointerExceptionasGlutenExceptionwhile intentionally lettingErrorsubclasses propagate. A JUnit 4 test covers a plain regular file (canonicalisation via a symlinked parent dir, so the assertion bites on Linux too), a versioned-library symlink chain with relative targets, and a 2-cycle. A@Beforeprobe skips cleanly on filesystems that do not support symbolic-link creation or resolution.Why are the changes needed?
Closes #12401. Versioned native libraries are typically published as a chain like
libfoo.so -> libfoo.so.1 -> libfoo.so.1.2.3, where the stored target oflibfoo.sois the relativelibfoo.so.1. The old loop's second iteration calledFiles.isSymbolicLink(Paths.get("libfoo.so.1")), which probes the JVM's working directory instead of the directory holding the link, so resolution silently returned a wrong path thatSystem.loadthen rejected (or, if the working directory happened to contain a matching name, loaded the wrong library). A symlink cycle would also spin forever inside the loop.Path#toRealPathremoves both issues with a single JDK call.Does this PR introduce any user-facing change?
No. Behaviour change is limited to error cases the old loop would have mishandled (relative-symlink failures and cycles), where callers now get a wrapped
GlutenExceptioninstead of an incorrect or hung load.How was this patch tested?
mvn -pl gluten-core -Pspark-3.5 spotless:checkpasses. New testsJniLibLoaderTest(3 tests) pass against the fix. Each test was also verified against the old buggy implementation and observed to fail with the expected red signal (input-verbatim assertion on the regular-file test, path-not-equal on the chain test,Assert.failon the cycle test). Existinggluten-coretests are unchanged.