allow java.home config files to be included in the build output #7109

kkriske · 2023-08-02T21:58:45Z

set up basic support for copying files from the java.home directory to the build output

…o the build output

fniephaus · 2023-08-04T06:40:11Z

substratevm/src/com.oracle.svm.core/src/com/oracle/svm/core/BuildArtifacts.java

@@ -47,6 +47,8 @@ enum ArtifactType {
        JDK_LIBRARY("jdk_libraries"),
        /* For all library shims for the JDK needed at run-time. */
        JDK_LIBRARY_SHIM(JDK_LIBRARY.getJsonKey()), // distinction should not be important to users.
+        /* For all configuration files from java.home needed at run-time */
+        JDK_CONFIG_FILE("jdk_config"),


If you extend this enum, you'll also have to extend this JSON schema. Since v0.9.0 is already release, you'll have to copy it and call it v0.9.1. Also, search for references to the file name and update the help text of the corresponding option. We should probably add a note about this to the enum's javadoc.

example build artifacts file:

{ "executables": [ "native-tests.exe.exe" ], "jdk_config": [ "lib\\fontconfig.bfc" ], "jdk_libraries": [ "awt.dll", "fontmanager.dll", "freetype.dll", "javaaccessbridge.dll", "javajpeg.dll", "jawt.dll", "lcms.dll", "java.dll", "jvm.dll" ] }

I'm assuming the double backslash may have to be normalized?

fniephaus · 2023-08-04T06:43:47Z

substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/jdk/JDKConfigFiles.java

+        if (configFile.startsWith("/")) {
+            configFiles.add(configFile.substring(1));
+        } else {
+            configFiles.add(configFile);
+        }


How about this?

Suggested change

if (configFile.startsWith("/")) {

configFiles.add(configFile.substring(1));

} else {

configFiles.add(configFile);

}

configFiles.add(configFile.startsWith("/") ? configFile.substring(1) : configFile);

Also, shouldn't we use File.separator instead of "/" here?

The thing that's validated here is user input (as in GraalVM dev/contributer, as it's not exposes to end users), so it all depends on how JDKConfigFiles.singleton().register("") is called.
In theory, removing the leading slash isn't required either if nobody passes one in.. So depends on how much input sanitisation you want to do.

Right, how about changing this to something like register(String... configPathParts), which can then be used like register("lib", "bar", "my.file");. Parts are not allowed to be .. or to start with / or \. register() then joins the parts using the correct File.separator and validates that the joined path exists in java.home. If any of this is not the case, fail with an error.

You could even just use Paths.get() in register().

A vararg does make more sense indeed. I've modified it and changed the validation accordingly.

fniephaus · 2023-08-04T06:44:17Z

substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/jdk/JDKConfigFiles.java

+
+    @Override
+    public void afterImageWrite(AfterImageWriteAccess access) {
+        Path jdkHomeDir = Path.of(System.getProperty("java.home")).normalize();


java.home cannot be null, right?

I don't think it can be. What are you hinting at?

Just checking. As you know, java.home isn't always set in SVM 😉

fniephaus · 2023-08-04T06:46:50Z

substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/jdk/JDKConfigFiles.java

+        Path jdkHomeDir = Path.of(System.getProperty("java.home")).normalize();
+        for (String configFile : new TreeSet<>(JDKConfigFiles.singleton().configFiles)) {
+            Path jdkConfigPath = jdkHomeDir.resolve(configFile).normalize();
+            VMError.guarantee(jdkConfigPath.startsWith(jdkHomeDir));


Doesn't resolve().normalize() take care of this? If you want to keep the guarantee, please add a descriptive message.

I still need to validate this, but the idea was to protect against passing in ../../../someSystemFile, which after .normalize() would no longer contain the .. directories, but would point to a fille outside of the java.home dir. That is unless I'm missing something.
Since it's not actually exposes to end user (will it ever be?) this validation is maybe a bit overly cautious.

I agree that we probably don't need to be too cautious here as it's internal API. However, the validation again happens late in the process, I'd prefer if we validate such things as early as possible (in the register() hook in this case).

fniephaus · 2023-08-04T06:47:52Z

substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/jdk/JDKConfigFiles.java

+                Files.copy(jdkConfigPath, imageConfigPath, REPLACE_EXISTING);
+                BuildArtifacts.singleton().add(BuildArtifacts.ArtifactType.JDK_CONFIG_FILE, imageConfigPath);
+            } catch (IOException e) {
+                VMError.shouldNotReachHere(e);


Please add a descriptive reason such as

Suggested change

VMError.shouldNotReachHere(e);

VMError.shouldNotReachHere("Failed to copy JDK configuration files", e);

fniephaus · 2023-08-04T06:49:06Z

substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/jdk/JDKConfigFiles.java

+final class JDKConfigFilesFeature implements InternalFeature {
+    @Override
+    public void afterRegistration(AfterRegistrationAccess access) {
+        ImageSingletons.add(JDKConfigFiles.class, new JDKConfigFiles());


You could annotate JDKConfigFiles with AutomaticallyRegisteredImageSingleton and drop this manual registration if you like, but it can also stay as is.

fniephaus · 2023-08-04T06:52:28Z

substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/jdk/JDKConfigFiles.java

+    }
+
+    public void register(String configFile) {
+        if (configFile.startsWith("/")) {


I wonder whether it would make sense to check the existence of the configFile here, so that we can fail when an incorrect file is registered, rather than later before the image is being created.

Makes sense, I moved the validation to the register call.

- make input validation more readable

- include lib/fontconfig.bfc

oracle-contributor-agreement bot added the OCA Verified All contributors have signed the Oracle Contributor Agreement. label Aug 2, 2023

kkriske force-pushed the java_home_config_files branch from e4296bc to 1b24a1d Compare August 2, 2023 22:08

set up basic support for copying files from the java.home directory t…

67f7cc7

…o the build output

kkriske force-pushed the java_home_config_files branch from 1b24a1d to 67f7cc7 Compare August 3, 2023 21:40

fniephaus reviewed Aug 4, 2023

View reviewed changes

fniephaus requested a review from pejovica August 4, 2023 06:49

fniephaus assigned kkriske Aug 4, 2023

fniephaus added the native-image label Aug 4, 2023

fniephaus reviewed Aug 4, 2023

View reviewed changes

validate the existence of files at registration to fail fast

f06cd4f

kkriske force-pushed the java_home_config_files branch 2 times, most recently from 3e92a8a to 6e8c7da Compare August 4, 2023 19:45

- update build-artifacts-schema

84fed02

- make input validation more readable

kkriske force-pushed the java_home_config_files branch from 6e8c7da to 84fed02 Compare August 4, 2023 19:55

- set java.home to the image directory

ab95a49

- include lib/fontconfig.bfc

fniephaus mentioned this pull request Jan 15, 2024

Support for Liberica Native Image graalvm/setup-graalvm#76

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

allow java.home config files to be included in the build output #7109

allow java.home config files to be included in the build output #7109

kkriske commented Aug 2, 2023

fniephaus Aug 4, 2023

kkriske Aug 27, 2023

fniephaus Aug 4, 2023

kkriske Aug 4, 2023 •

edited

Loading

fniephaus Aug 4, 2023

fniephaus Aug 4, 2023

kkriske Aug 4, 2023

fniephaus Aug 4, 2023

kkriske Aug 4, 2023

fniephaus Aug 4, 2023

fniephaus Aug 4, 2023

kkriske Aug 4, 2023

fniephaus Aug 4, 2023

fniephaus Aug 4, 2023

fniephaus Aug 4, 2023

fniephaus Aug 4, 2023

kkriske Aug 4, 2023

	VMError.shouldNotReachHere(e);
	VMError.shouldNotReachHere("Failed to copy JDK configuration files", e);

allow java.home config files to be included in the build output #7109

Are you sure you want to change the base?

allow java.home config files to be included in the build output #7109

Conversation

kkriske commented Aug 2, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kkriske Aug 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kkriske Aug 4, 2023 •

edited

Loading