Skip to content

Commit

Permalink
fix(core): System.load calls from incorrect classloader. Close #648
Browse files Browse the repository at this point in the history 
This fix applies to Windows only. Also:

- The System.load call will be skipped altogether when the library path
  does not contain characters above the ASCII range.
- SLL now handles more unusual configurations gracefully (some libraries
  pre-extracted, others in JARs, in any load order).
  • Loading branch information
@Spasi
Spasi committed Sep 4, 2021
1 parent dfe0a6f commit c5bab97
Show file tree
Hide file tree
Showing 4 changed files with 72 additions and 33 deletions.
1 change: 1 addition & 0 deletions doc/notes/3.3.0.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,7 @@ This build includes the following changes:
- Generator: Native libraries are now initialized before constants. (#630)
- Core: Fixed unsafe field reads in `MemoryUtil` setup code. (#632)
- Core: The `SharedLibraryLoader` now prepends a `.` to `SHARED_LIBRARY_EXTRACT_DIRECTORY` when the target directory is not temporary.
- Core: The `SharedLibraryLoader` will now load libraries in the correct `ClassLoader` when testing for [JDK-8195129](https://bugs.openjdk.java.net/browse/JDK-8195129). (#648)
- EGL: Fixed nullability of `eglInitialize` arguments.
- GLFW: Fixed main thread check. Setting `GLFW_CHECK_THREAD0` to `false` is now required for offscreen interop. (#538)
- OpenCL: Added missing `errcode_ret` parameter to `clLinkProgram`. (#560)
Expand Down
4 changes: 2 additions & 2 deletions modules/lwjgl/core/src/main/java/org/lwjgl/system/Library.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ public static void loadSystem(
apiLog("\tUsing SharedLibraryLoader...");
}
// Extract from classpath and try org.lwjgl.librarypath
try (FileChannel ignored = SharedLibraryLoader.load(name, libName, libURL)) {
try (FileChannel ignored = SharedLibraryLoader.load(name, libName, libURL, load)) {
if (loadSystemFromLibraryPath(load, context, module, libName, bundledWithLWJGL)) {
return;
}
Expand Down Expand Up @@ -262,7 +262,7 @@ private static SharedLibrary loadNative(Class<?> context, String module, String
apiLog("\tUsing SharedLibraryLoader...");
}
// Extract from classpath and try org.lwjgl.librarypath
try (FileChannel ignored = SharedLibraryLoader.load(name, libName, libURL)) {
try (FileChannel ignored = SharedLibraryLoader.load(name, libName, libURL, null)) {
lib = loadNativeFromLibraryPath(context, module, libName, bundledWithLWJGL);
if (lib != null) {
return lib;
Expand Down
2 changes: 1 addition & 1 deletion modules/lwjgl/core/src/main/java/org/lwjgl/system/LibraryResource.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ private static Path load(Class<?> context, String module, String name, boolean b
apiLog("\tUsing SharedLibraryLoader...");
}
// Extract from classpath and try org.lwjgl.librarypath
try (FileChannel ignored = SharedLibraryLoader.load(name, name, resourceURL)) {
try (FileChannel ignored = SharedLibraryLoader.load(name, name, resourceURL, null)) {
path = loadFromLibraryPath(module, name, bundledWithLWJGL);
if (path != null) {
return path;
Expand Down
98 changes: 68 additions & 30 deletions modules/lwjgl/core/src/main/java/org/lwjgl/system/SharedLibraryLoader.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,9 @@
import java.net.*;
import java.nio.channels.*;
import java.nio.file.*;
import java.util.*;
import java.util.concurrent.locks.*;
import java.util.function.*;
import java.util.stream.*;
import java.util.zip.*;

Expand All @@ -38,6 +40,10 @@ final class SharedLibraryLoader {
@Nullable
private static Path extractPath;

private static HashSet<Path> extractPaths = new HashSet<>(4);

private static boolean checkedJDK8195129;

private SharedLibraryLoader() {
}

Expand All @@ -47,24 +53,36 @@ private SharedLibraryLoader() {
* @param name the resource name
* @param filename the resource filename
* @param resource the classpath {@link URL} were the resource can be found
* @param load should call {@code System::load} in the context of the appropriate ClassLoader
*
* @return a {@link FileChannel} that has locked the resource file
*/
static FileChannel load(String name, String filename, URL resource) {
static FileChannel load(String name, String filename, URL resource, @Nullable Consumer<String> load) {
try {
Path extractedFile;

EXTRACT_PATH_LOCK.lock();
try {
if (extractPath != null) {
// Reuse the lwjgl shared library location
// This path is already tested and safe to use
extractedFile = extractPath.resolve(filename);
} else {
extractedFile = getExtractPath(filename, resource);
// Do not store unless the test for JDK-8195129 has passed
if (Platform.get() != Platform.WINDOWS || workaroundJDK8195129(extractedFile)) {
initExtractPath(extractPath = extractedFile.getParent());
extractedFile = getExtractPath(filename, resource, load);

Path parent = extractedFile.getParent();
// Do not store unless the test for JDK-8195129 has passed.
// This means that in the worst case org.lwjgl.librarypath
// will contain multiple directories. (Windows only)
// -----------------
// Example scenario:
// -----------------
// * load lwjgl.dll - already extracted and in classpath (SLL not used)
// * load library with loadNative - extracted to a directory with unicode characters
// * then another with loadSystem - this will hit LoadLibraryA in the JVM, need an ANSI-safe directory.
if (Platform.get() != Platform.WINDOWS || checkedJDK8195129) {
extractPath = parent;
}
initExtractPath(parent);
}
} finally {
EXTRACT_PATH_LOCK.unlock();
Expand All @@ -77,6 +95,11 @@ static FileChannel load(String name, String filename, URL resource) {
}

private static void initExtractPath(Path extractPath) {
if (extractPaths.contains(extractPath)) {
return;
}
extractPaths.add(extractPath);

String newLibPath = extractPath.toAbsolutePath().toString();

// Prepend the path in which the libraries were extracted to org.lwjgl.librarypath
Expand All @@ -96,34 +119,38 @@ private static void initExtractPath(Path extractPath) {
*
* @return the extracted library
*/
private static Path getExtractPath(String filename, URL resource) {
private static Path getExtractPath(String filename, URL resource, @Nullable Consumer<String> load) {
Path root, file;

String override = Configuration.SHARED_LIBRARY_EXTRACT_PATH.get();
if (override != null) {
return Paths.get(override, filename);
file = (root = Paths.get(override)).resolve(filename);
if (canWrite(root, file, resource, load)) {
return file;
}
apiLog(String.format("\tThe path %s is not accessible. Trying other paths.", override));
}

String version = Version.getVersion().replace(' ', '-');

Path root, file;

// Temp directory with username in path
file = (root = Paths.get(System.getProperty("java.io.tmpdir")))
.resolve(Paths.get(Configuration.SHARED_LIBRARY_EXTRACT_DIRECTORY.get("lwjgl" + System.getProperty("user.name")), version, filename));
if (canWrite(root, file, resource)) {
if (canWrite(root, file, resource, load)) {
return file;
}

Path lwjgl_version_filename = Paths.get("." + Configuration.SHARED_LIBRARY_EXTRACT_DIRECTORY.get("lwjgl"), version, filename);

// Working directory
file = (root = Paths.get("").toAbsolutePath()).resolve(lwjgl_version_filename);
if (canWrite(root, file, resource)) {
if (canWrite(root, file, resource, load)) {
return file;
}

// User home
file = (root = Paths.get(System.getProperty("user.home"))).resolve(lwjgl_version_filename);
if (canWrite(root, file, resource)) {
if (canWrite(root, file, resource, load)) {
return file;
}

Expand All @@ -132,7 +159,7 @@ private static Path getExtractPath(String filename, URL resource) {
String env = System.getenv("SystemRoot");
if (env != null) {
file = (root = Paths.get(env, "Temp")).resolve(lwjgl_version_filename);
if (canWrite(root, file, resource)) {
if (canWrite(root, file, resource, load)) {
return file;
}
}
Expand All @@ -141,7 +168,7 @@ private static Path getExtractPath(String filename, URL resource) {
env = System.getenv("SystemDrive");
if (env != null) {
file = (root = Paths.get(env + "/")).resolve(Paths.get("Temp").resolve(lwjgl_version_filename));
if (canWrite(root, file, resource)) {
if (canWrite(root, file, resource, load)) {
return file;
}
}
Expand All @@ -152,7 +179,7 @@ private static Path getExtractPath(String filename, URL resource) {
file = Files.createTempDirectory("lwjgl");
root = file.getParent();
file = file.resolve(filename);
if (canWrite(root, file, resource)) {
if (canWrite(root, file, resource, load)) {
return file;
}
} catch (IOException ignored) {
Expand Down Expand Up @@ -187,10 +214,10 @@ private static FileChannel extract(Path file, URL resource) throws IOException {
}

// If file doesn't exist or the CRC doesn't match, extract it to the temp dir.
apiLog(String.format(" Extracting: %s", resource.getPath()));
apiLog(String.format("\tExtracting: %s", resource.getPath()));
//noinspection FieldAccessNotGuarded (already inside the lock)
if (extractPath == null) {
apiLog(String.format(" to: %s", file));
apiLog(String.format("\t to: %s", file));
}

Files.createDirectories(file.getParent());
Expand All @@ -212,13 +239,11 @@ private static FileChannel lock(Path file) {
try {
FileChannel fc = FileChannel.open(file);

//noinspection resource
if (fc.tryLock(0L, Long.MAX_VALUE, true) == null) {
if (Configuration.DEBUG_LOADER.get(false)) {
apiLog("\tFile is locked by another process, waiting...");
}

//noinspection resource
fc.lock(0L, Long.MAX_VALUE, true); // this will block until the file is locked
}

Expand Down Expand Up @@ -254,7 +279,7 @@ private static long crc(InputStream input) throws IOException {
*
* @return true if the file is writable
*/
private static boolean canWrite(Path root, Path file, URL resource) {
private static boolean canWrite(Path root, Path file, URL resource, @Nullable Consumer<String> load) {
Path testFile;
if (Files.exists(file)) {
if (!Files.isWritable(file)) {
Expand All @@ -276,13 +301,8 @@ private static boolean canWrite(Path root, Path file, URL resource) {
Files.write(testFile, new byte[0]);
Files.delete(testFile);

if (workaroundJDK8195129(file)) {
// We have full access, the JVM has locked the file, but System.load can still fail if
// the path contains unicode characters, due to JDK-8195129. Test for this here and
// return false if it fails to try other paths.
try (FileChannel ignored = extract(file, resource)) {
System.load(file.toAbsolutePath().toString());
}
if (load != null && Platform.get() == Platform.WINDOWS) {
workaroundJDK8195129(file, resource, load);
}

return true;
Expand Down Expand Up @@ -314,8 +334,26 @@ private static void canWriteCleanup(Path root, Path file) {
}
}

private static boolean workaroundJDK8195129(Path file) {
return Platform.get() == Platform.WINDOWS && file.toString().endsWith(".dll");
private static void workaroundJDK8195129(Path file, URL resource, @Nonnull Consumer<String> load) throws Throwable {
String filepath = file.toAbsolutePath().toString();
if (filepath.endsWith(".dll")) {
boolean mustCheck = false;
for (int i = 0; i < filepath.length(); i++) {
if (0x80 <= filepath.charAt(i)) {
mustCheck = true;
}
}
if (mustCheck) {
// We have full access, the JVM has locked the file, but System.load can still fail if
// the path contains unicode characters, due to JDK-8195129. Test for this here and
// try other paths if it fails.
try (FileChannel ignored = extract(file, resource)) {
load.accept(file.toAbsolutePath().toString());
}
}
checkedJDK8195129 = true;
}
}


}

0 comments on commit c5bab97

Please sign in to comment.