From 64184d3a0b39b0e9ac62e9c575b92b52f6001dfe Mon Sep 17 00:00:00 2001 From: Foivos Zakkak Date: Wed, 12 Jul 2023 18:31:57 +0300 Subject: [PATCH 1/5] Throw exception for null in RuntimeJNIAccess/RuntimeReflection reg. Don't allow null values to be passed to the `register` method of `RuntimeJNIAccess` and `RuntimeReflection`. Since these are public APIs GraalVM should either handle null values (by ignoring them in this case) or throw a `NullPointerException` before creating an asynchronous task to perform the registration in the analysis, which then results in `NullPointerException`s being thrown later when it's no longer possible to understand where the null value originate from. (cherry picked from commit e6c12dd389609f0eac07aa93d085fa33dd21bce0) --- .../nativeimage/impl/ReflectionRegistry.java | 8 +- substratevm/mx.substratevm/mx_substratevm.py | 4 +- .../ConditionalConfigurationRegistry.java | 8 ++ .../hosted/reflect/ReflectionDataBuilder.java | 8 ++ .../svm/test/ReflectionRegistrationTest.java | 120 ++++++++++++++++++ 5 files changed, 145 insertions(+), 3 deletions(-) create mode 100644 substratevm/src/com.oracle.svm.test/src/com/oracle/svm/test/ReflectionRegistrationTest.java diff --git a/sdk/src/org.graalvm.nativeimage/src/org/graalvm/nativeimage/impl/ReflectionRegistry.java b/sdk/src/org.graalvm.nativeimage/src/org/graalvm/nativeimage/impl/ReflectionRegistry.java index 4198a895ec0..7f1141b6435 100644 --- a/sdk/src/org.graalvm.nativeimage/src/org/graalvm/nativeimage/impl/ReflectionRegistry.java +++ b/sdk/src/org.graalvm.nativeimage/src/org/graalvm/nativeimage/impl/ReflectionRegistry.java @@ -46,7 +46,13 @@ public interface ReflectionRegistry { default void register(ConfigurationCondition condition, Class... classes) { - Arrays.stream(classes).forEach(clazz -> register(condition, false, clazz)); + Arrays.stream(classes).forEach(clazz -> { + if (clazz == null) { + throw new NullPointerException("Cannot register null value as class for reflection. " + + "Please ensure that all values you register are not null."); + } + register(condition, false, clazz); + }); } void register(ConfigurationCondition condition, boolean unsafeAllocated, Class clazz); diff --git a/substratevm/mx.substratevm/mx_substratevm.py b/substratevm/mx.substratevm/mx_substratevm.py index 44940e208f8..c2b60e1f5d0 100644 --- a/substratevm/mx.substratevm/mx_substratevm.py +++ b/substratevm/mx.substratevm/mx_substratevm.py @@ -1,7 +1,7 @@ # # ---------------------------------------------------------------------------------------------------- # -# Copyright (c) 2018, 2021, Oracle and/or its affiliates. All rights reserved. +# Copyright (c) 2018, 2023, Oracle and/or its affiliates. All rights reserved. # DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. # # This code is free software; you can redistribute it and/or modify it @@ -296,7 +296,7 @@ def native_image_func(args, **kwargs): yield native_image_func native_image_context.hosted_assertions = ['-J-ea', '-J-esa'] -_native_unittest_features = '--features=com.oracle.svm.test.ImageInfoTest$TestFeature,com.oracle.svm.test.ServiceLoaderTest$TestFeature,com.oracle.svm.test.SecurityServiceTest$TestFeature' +_native_unittest_features = '--features=com.oracle.svm.test.ImageInfoTest$TestFeature,com.oracle.svm.test.ServiceLoaderTest$TestFeature,com.oracle.svm.test.SecurityServiceTest$TestFeature,com.oracle.svm.test.ReflectionRegistrationTest$TestFeature' IMAGE_ASSERTION_FLAGS = svm_experimental_options(['-H:+VerifyGraalGraphs', '-H:+VerifyPhases']) diff --git a/substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/ConditionalConfigurationRegistry.java b/substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/ConditionalConfigurationRegistry.java index dab90726dfb..e696f21e303 100644 --- a/substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/ConditionalConfigurationRegistry.java +++ b/substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/ConditionalConfigurationRegistry.java @@ -38,6 +38,14 @@ public abstract class ConditionalConfigurationRegistry { private final Map> pendingReachabilityHandlers = new ConcurrentHashMap<>(); protected void registerConditionalConfiguration(ConfigurationCondition condition, Runnable runnable) { + if (condition == null) { + throw new NullPointerException("Cannot use null value as condition for conditional configuration. " + + "Please ensure that you register a non-null condition."); + } + if (runnable == null) { + throw new NullPointerException("Cannot use null value as runnable for conditional configuration. " + + "Please ensure that you register a non-null runnable."); + } if (ConfigurationCondition.alwaysTrue().equals(condition)) { /* analysis optimization to include new types as early as possible */ runnable.run(); diff --git a/substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/reflect/ReflectionDataBuilder.java b/substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/reflect/ReflectionDataBuilder.java index 64283d256c8..488c8a4e994 100644 --- a/substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/reflect/ReflectionDataBuilder.java +++ b/substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/reflect/ReflectionDataBuilder.java @@ -284,6 +284,10 @@ public void register(ConfigurationCondition condition, boolean queriedOnly, Exec checkNotSealed(); register(analysisUniverse -> registerConditionalConfiguration(condition, () -> { for (Executable executable : executables) { + if (executable == null) { + throw new NullPointerException("Cannot register null value as executable for reflection. " + + "Please ensure that all values you register are not null."); + } analysisUniverse.getBigbang().postTask(debug -> registerMethod(queriedOnly, executable)); } })); @@ -416,6 +420,10 @@ public void register(ConfigurationCondition condition, boolean finalIsWritable, private void registerInternal(ConfigurationCondition condition, Field... fields) { register(analysisUniverse -> registerConditionalConfiguration(condition, () -> { for (Field field : fields) { + if (field == null) { + throw new NullPointerException("Cannot register null value as field for reflection. " + + "Please ensure that all values you register are not null."); + } analysisUniverse.getBigbang().postTask(debug -> registerField(field)); } })); diff --git a/substratevm/src/com.oracle.svm.test/src/com/oracle/svm/test/ReflectionRegistrationTest.java b/substratevm/src/com.oracle.svm.test/src/com/oracle/svm/test/ReflectionRegistrationTest.java new file mode 100644 index 00000000000..555cc683657 --- /dev/null +++ b/substratevm/src/com.oracle.svm.test/src/com/oracle/svm/test/ReflectionRegistrationTest.java @@ -0,0 +1,120 @@ +/* + * Copyright (c) 2023, 2023, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2023, 2023, Red Hat Inc. All rights reserved. + * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. + * + * This code is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License version 2 only, as + * published by the Free Software Foundation. Oracle designates this + * particular file as subject to the "Classpath" exception as provided + * by Oracle in the LICENSE file that accompanied this code. + * + * This code is distributed in the hope that it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License + * version 2 for more details (a copy is included in the LICENSE file that + * accompanied this code). + * + * You should have received a copy of the GNU General Public License version + * 2 along with this work; if not, write to the Free Software Foundation, + * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA. + * + * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA + * or visit www.oracle.com if you need additional information or have any + * questions. + */ +package com.oracle.svm.test; + +import com.oracle.svm.hosted.FeatureImpl; +import com.oracle.svm.hosted.substitute.SubstitutionReflectivityFilter; +import org.graalvm.nativeimage.ImageSingletons; +import org.graalvm.nativeimage.hosted.Feature; +import org.graalvm.nativeimage.hosted.RuntimeReflection; +import org.graalvm.nativeimage.impl.RuntimeReflectionSupport; +import org.junit.Test; + +import java.lang.reflect.Executable; +import java.lang.reflect.Field; + +/** + * Tests the {@link RuntimeReflection}. + */ +public class ReflectionRegistrationTest { + + public static class TestFeature implements Feature { + + @SuppressWarnings("unused")// + int unusedVariableOne = 1; + @SuppressWarnings("unused")// + int unusedVariableTwo = 2; + + @Override + public void beforeAnalysis(final BeforeAnalysisAccess access) { + try { + RuntimeReflection.register((Class) null); + assert false; + } catch (NullPointerException e) { + assert e.getMessage().startsWith("Cannot register null value"); + } + try { + RuntimeReflection.register((Executable) null); + assert false; + } catch (NullPointerException e) { + assert e.getMessage().startsWith("Cannot register null value"); + } + try { + RuntimeReflection.register((Field) null); + assert false; + } catch (NullPointerException e) { + assert e.getMessage().startsWith("Cannot register null value"); + } + + try { + ImageSingletons.lookup(RuntimeReflectionSupport.class).register(null, this.getClass()); + assert false; + } catch (NullPointerException e) { + assert e.getMessage().startsWith("Cannot use null value"); + } + + try { + ImageSingletons.lookup(RuntimeReflectionSupport.class).register(null, true, this.getClass().getMethods()); + assert false; + } catch (NullPointerException e) { + assert e.getMessage().startsWith("Cannot use null value"); + } + + try { + ImageSingletons.lookup(RuntimeReflectionSupport.class).register(null, true, this.getClass().getFields()); + assert false; + } catch (NullPointerException e) { + assert e.getMessage().startsWith("Cannot use null value"); + } + + FeatureImpl.BeforeAnalysisAccessImpl impl = (FeatureImpl.BeforeAnalysisAccessImpl) access; + try { + SubstitutionReflectivityFilter.shouldExclude((Class) null, impl.getMetaAccess(), impl.getUniverse()); + assert false; + } catch (NullPointerException e) { + // expected + } + try { + SubstitutionReflectivityFilter.shouldExclude((Executable) null, impl.getMetaAccess(), impl.getUniverse()); + assert false; + } catch (NullPointerException e) { + // expected + } + try { + SubstitutionReflectivityFilter.shouldExclude((Field) null, impl.getMetaAccess(), impl.getUniverse()); + assert false; + } catch (NullPointerException e) { + // expected + } + } + + } + + @Test + public void test() { + // nothing to do + } +} From 1d0bab4379040cf60e57ac329dad5d859eb75fe8 Mon Sep 17 00:00:00 2001 From: Fabio Niephaus Date: Wed, 1 Nov 2023 17:43:25 +0100 Subject: [PATCH 2/5] Fix style. (cherry picked from commit d621dbd5b1e9f3e3095e05b062b39afc4c6f3588) --- .../src/org/graalvm/nativeimage/impl/ReflectionRegistry.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sdk/src/org.graalvm.nativeimage/src/org/graalvm/nativeimage/impl/ReflectionRegistry.java b/sdk/src/org.graalvm.nativeimage/src/org/graalvm/nativeimage/impl/ReflectionRegistry.java index 7f1141b6435..0179e2db9a9 100644 --- a/sdk/src/org.graalvm.nativeimage/src/org/graalvm/nativeimage/impl/ReflectionRegistry.java +++ b/sdk/src/org.graalvm.nativeimage/src/org/graalvm/nativeimage/impl/ReflectionRegistry.java @@ -49,7 +49,7 @@ default void register(ConfigurationCondition condition, Class... classes) { Arrays.stream(classes).forEach(clazz -> { if (clazz == null) { throw new NullPointerException("Cannot register null value as class for reflection. " + - "Please ensure that all values you register are not null."); + "Please ensure that all values you register are not null."); } register(condition, false, clazz); }); From 373204c2b418096cff678a08b405b9e0983b0100 Mon Sep 17 00:00:00 2001 From: Fabio Niephaus Date: Thu, 2 Nov 2023 14:26:29 +0100 Subject: [PATCH 3/5] Move null checks to the beginning of register methods. Not before the register methods, which can miss cases, nor later on in a runnable. (cherry picked from commit f94551abad61ae492d9a0b266a07564ab8aff92e) --- .../nativeimage/impl/ReflectionRegistry.java | 8 +------ .../hosted/reflect/ReflectionDataBuilder.java | 22 ++++++++++++------- 2 files changed, 15 insertions(+), 15 deletions(-) diff --git a/sdk/src/org.graalvm.nativeimage/src/org/graalvm/nativeimage/impl/ReflectionRegistry.java b/sdk/src/org.graalvm.nativeimage/src/org/graalvm/nativeimage/impl/ReflectionRegistry.java index 0179e2db9a9..4198a895ec0 100644 --- a/sdk/src/org.graalvm.nativeimage/src/org/graalvm/nativeimage/impl/ReflectionRegistry.java +++ b/sdk/src/org.graalvm.nativeimage/src/org/graalvm/nativeimage/impl/ReflectionRegistry.java @@ -46,13 +46,7 @@ public interface ReflectionRegistry { default void register(ConfigurationCondition condition, Class... classes) { - Arrays.stream(classes).forEach(clazz -> { - if (clazz == null) { - throw new NullPointerException("Cannot register null value as class for reflection. " + - "Please ensure that all values you register are not null."); - } - register(condition, false, clazz); - }); + Arrays.stream(classes).forEach(clazz -> register(condition, false, clazz)); } void register(ConfigurationCondition condition, boolean unsafeAllocated, Class clazz); diff --git a/substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/reflect/ReflectionDataBuilder.java b/substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/reflect/ReflectionDataBuilder.java index 488c8a4e994..0c714845534 100644 --- a/substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/reflect/ReflectionDataBuilder.java +++ b/substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/reflect/ReflectionDataBuilder.java @@ -56,6 +56,7 @@ import java.util.Collections; import java.util.List; import java.util.Map; +import java.util.Objects; import java.util.Optional; import java.util.Set; import java.util.concurrent.Callable; @@ -166,6 +167,7 @@ private void setQueryFlag(Class clazz, int flag) { @Override public void register(ConfigurationCondition condition, boolean unsafeInstantiated, Class clazz) { + Objects.requireNonNull(clazz, () -> nullErrorMessage("class")); checkNotSealed(); register(analysisUniverse -> registerConditionalConfiguration(condition, () -> analysisUniverse.getBigbang().postTask(debug -> registerClass(clazz, unsafeInstantiated)))); @@ -281,13 +283,10 @@ public void registerAllSignersQuery(ConfigurationCondition condition, Class c @Override public void register(ConfigurationCondition condition, boolean queriedOnly, Executable... executables) { + requireNonNull(executables, "executable"); checkNotSealed(); register(analysisUniverse -> registerConditionalConfiguration(condition, () -> { for (Executable executable : executables) { - if (executable == null) { - throw new NullPointerException("Cannot register null value as executable for reflection. " + - "Please ensure that all values you register are not null."); - } analysisUniverse.getBigbang().postTask(debug -> registerMethod(queriedOnly, executable)); } })); @@ -413,6 +412,7 @@ public void registerConstructorLookup(ConfigurationCondition condition, Class @Override public void register(ConfigurationCondition condition, boolean finalIsWritable, Field... fields) { + requireNonNull(fields, "field"); checkNotSealed(); registerInternal(condition, fields); } @@ -420,10 +420,6 @@ public void register(ConfigurationCondition condition, boolean finalIsWritable, private void registerInternal(ConfigurationCondition condition, Field... fields) { register(analysisUniverse -> registerConditionalConfiguration(condition, () -> { for (Field field : fields) { - if (field == null) { - throw new NullPointerException("Cannot register null value as field for reflection. " + - "Please ensure that all values you register are not null."); - } analysisUniverse.getBigbang().postTask(debug -> registerField(field)); } })); @@ -1080,4 +1076,14 @@ public int getReflectionMethodsCount() { public int getReflectionFieldsCount() { return registeredFields.size(); } + + private static void requireNonNull(Object[] values, String kind) { + for (Object value : values) { + Objects.requireNonNull(value, () -> nullErrorMessage(kind)); + } + } + + private static String nullErrorMessage(String kind) { + return "Cannot register null value as " + kind + " for reflection. Please ensure that all values you register are not null."; + } } From 6af144fce4e6e541164082918070b29ce8111481 Mon Sep 17 00:00:00 2001 From: Fabio Niephaus Date: Thu, 2 Nov 2023 14:27:00 +0100 Subject: [PATCH 4/5] Apply non-null strategy to `JNIAccessFeature`. (cherry picked from commit d996f323b7c6ce796caccff6ae50bd1c2fa5a81f) --- .../oracle/svm/hosted/jni/JNIAccessFeature.java | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/jni/JNIAccessFeature.java b/substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/jni/JNIAccessFeature.java index 2f1dfeb40bf..37bf159f74b 100644 --- a/substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/jni/JNIAccessFeature.java +++ b/substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/jni/JNIAccessFeature.java @@ -34,6 +34,7 @@ import java.util.IdentityHashMap; import java.util.List; import java.util.Map; +import java.util.Objects; import java.util.Set; import java.util.concurrent.ConcurrentHashMap; import java.util.function.Predicate; @@ -202,18 +203,21 @@ private class JNIRuntimeAccessibilitySupportImpl extends ConditionalConfiguratio @Override public void register(ConfigurationCondition condition, boolean unsafeAllocated, Class clazz) { assert !unsafeAllocated : "unsafeAllocated can be only set via Unsafe.allocateInstance, not via JNI."; + Objects.requireNonNull(clazz, () -> nullErrorMessage("class")); abortIfSealed(); registerConditionalConfiguration(condition, () -> newClasses.add(clazz)); } @Override public void register(ConfigurationCondition condition, boolean queriedOnly, Executable... methods) { + requireNonNull(methods, "methods"); abortIfSealed(); registerConditionalConfiguration(condition, () -> newMethods.addAll(Arrays.asList(methods))); } @Override public void register(ConfigurationCondition condition, boolean finalIsWritable, Field... fields) { + requireNonNull(fields, "field"); abortIfSealed(); registerConditionalConfiguration(condition, () -> registerFields(finalIsWritable, fields)); } @@ -622,4 +626,14 @@ private static boolean anyFieldMatches(ResolvedJavaType sub, String name) { return false; } } + + private static void requireNonNull(Object[] values, String kind) { + for (Object value : values) { + Objects.requireNonNull(value, () -> nullErrorMessage(kind)); + } + } + + private static String nullErrorMessage(String kind) { + return "Cannot register null value as " + kind + " for JNI access. Please ensure that all values you register are not null."; + } } From 9980fdfc9a92b73adb467e9c867c5ddfc1c1b9a1 Mon Sep 17 00:00:00 2001 From: Fabio Niephaus Date: Thu, 2 Nov 2023 14:35:01 +0100 Subject: [PATCH 5/5] Use `Objects.requireNonNull()` in `ConditionalConfigurationRegistry`. (cherry picked from commit 0ba6cc2c33725a482e190525c6f2bd153ec82b2b) --- .../svm/hosted/ConditionalConfigurationRegistry.java | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/ConditionalConfigurationRegistry.java b/substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/ConditionalConfigurationRegistry.java index e696f21e303..8af9e717078 100644 --- a/substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/ConditionalConfigurationRegistry.java +++ b/substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/ConditionalConfigurationRegistry.java @@ -26,6 +26,7 @@ import java.util.Collection; import java.util.Map; +import java.util.Objects; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.ConcurrentLinkedQueue; @@ -38,14 +39,8 @@ public abstract class ConditionalConfigurationRegistry { private final Map> pendingReachabilityHandlers = new ConcurrentHashMap<>(); protected void registerConditionalConfiguration(ConfigurationCondition condition, Runnable runnable) { - if (condition == null) { - throw new NullPointerException("Cannot use null value as condition for conditional configuration. " + - "Please ensure that you register a non-null condition."); - } - if (runnable == null) { - throw new NullPointerException("Cannot use null value as runnable for conditional configuration. " + - "Please ensure that you register a non-null runnable."); - } + Objects.requireNonNull(condition, "Cannot use null value as condition for conditional configuration. Please ensure that you register a non-null condition."); + Objects.requireNonNull(runnable, "Cannot use null value as runnable for conditional configuration. Please ensure that you register a non-null runnable."); if (ConfigurationCondition.alwaysTrue().equals(condition)) { /* analysis optimization to include new types as early as possible */ runnable.run();