Hi,
This mail contains a potential patch to the issue[1] that I had reported
a couple of years back. The issue is still valid and reproducible with
latest upstream JDK.
In brief, if a certain code has:
public void doSomething(String foo) {...}
and then it gets invoked through reflection:
thatMethod = // get hold of that method through reflection
thatMethod.invoke(validObjectInstance, null);
then as per the javadoc of the Method#invoke[2] you would expect an
IllegalArgumentException (since the method expects 1 parameter and we
are sending none). This does throw an IllegalArgumentException, but when
the invocation happens through a (dynamically generated) MethodAccessor,
instead of a native MethodAccessor, the IllegalArgumentException that
gets thrown is due to a NPE that happens and the NPE's toString() output
is contained as a message of the IllegalArgumentException, as noted in
the JIRA. This happens even for Constructor invocations, through
ConstructorAccessor.
The commit in the attached patch, adds bytecode instructions to check if
the method arguments being passed is null and if so, doesn't attempt an
arraylength instruction and instead just stores 0 on to the stack. This
prevents the NullPointerException being reported, enclosed as a message
within the IllegalArgumentException and instead throws back a proper
IllegalArgumentException (as you would expect if the invocation had
happened over a native MethodAccessor).
The goal of the patch _isn't_ to match the exception message with what
the native MethodAccessor throws but just prevent the NPE from being
playing a role in the IllegalArgumentException. i.e. this change
_doesn't_ throw a new IllegalArgumentException("wrong number of arguments").
The patch also contains a new testcase which reproduces this against the
current upstream and verifies this patch works.
This is the first time I'm fiddling with bytecode generation, so I would
appreciate some feedback on the changed bytecode and if there's a
different/better way to do it. Furthermore, although I do have a signed
and approved OCA, I will need a sponsor for this patch. Anyone willing
to review and sponsor, please?
[1] https://bugs.openjdk.java.net/browse/JDK-8159797
[2]
https://docs.oracle.com/javase/10/docs/api/java/lang/reflect/Method.html#invoke(java.lang.Object,java.lang.Object...)
-Jaikiran
# HG changeset patch
# User Jaikiran Pai <jaikiran....@gmail.com>
# Date 1525350350 -19800
# Thu May 03 17:55:50 2018 +0530
# Node ID 2b535248cf02d90f6fea1c89150a277f16954c04
# Parent 7379e6f906aeb6e69ed8eccda9de285e606ab1ff
JDK-8159797 Prevent NPE from the generated Method/ConstructorAccessor when
invoking with incorrect number of arguments
diff --git
a/src/java.base/share/classes/jdk/internal/reflect/ClassFileConstants.java
b/src/java.base/share/classes/jdk/internal/reflect/ClassFileConstants.java
--- a/src/java.base/share/classes/jdk/internal/reflect/ClassFileConstants.java
+++ b/src/java.base/share/classes/jdk/internal/reflect/ClassFileConstants.java
@@ -137,4 +137,7 @@
// Access flags
public static final short ACC_PUBLIC = (short) 0x0001;
+
+ // push int constant to stack
+ public static final byte opc_iconst_0 = (byte) 0x03;
}
diff --git
a/src/java.base/share/classes/jdk/internal/reflect/MethodAccessorGenerator.java
b/src/java.base/share/classes/jdk/internal/reflect/MethodAccessorGenerator.java
---
a/src/java.base/share/classes/jdk/internal/reflect/MethodAccessorGenerator.java
+++
b/src/java.base/share/classes/jdk/internal/reflect/MethodAccessorGenerator.java
@@ -474,7 +474,13 @@
// aload_2 | aload_1 (Method | Constructor)
// ifnull <success label>
// aload_2 | aload_1
+ // ifnull <null args label>
+ // aload_2 | aload_1
// arraylength
+ // goto <arg count check label>
+ // <null args label:>
+ // iconst_0
+ // <arg count check label:>
// sipush <num parameter types>
// if_icmpeq <success label>
// new <IllegalArgumentException>
@@ -496,7 +502,22 @@
} else {
cb.opc_aload_2();
}
+ Label nullArgsLabel = new Label();
+ Label argCountCheckLabel = new Label();
+
+ cb.opc_ifnull(nullArgsLabel);
+ if (isConstructor) {
+ cb.opc_aload_1();
+ } else {
+ cb.opc_aload_2();
+ }
cb.opc_arraylength();
+ cb.opc_goto(argCountCheckLabel);
+
+ nullArgsLabel.bind();
+ cb.emitByte(ClassFileConstants.opc_iconst_0);
+
+ argCountCheckLabel.bind();
cb.opc_sipush((short) parameterTypes.length);
cb.opc_if_icmpeq(successLabel);
cb.opc_new(illegalArgumentClass);
diff --git
a/test/jdk/jdk/internal/reflect/Reflection/MethodAccessGeneratorTest.java
b/test/jdk/jdk/internal/reflect/Reflection/MethodAccessGeneratorTest.java
new file mode 100644
--- /dev/null
+++ b/test/jdk/jdk/internal/reflect/Reflection/MethodAccessGeneratorTest.java
@@ -0,0 +1,106 @@
+/*
+ * Copyright (c) 2012, 2013, 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
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * 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.
+ */
+
+/*
+ * @test
+ * @bug 8159797
+ * @modules java.base/jdk.internal.reflect
+ * @run main/othervm -Dsun.reflect.noInflation=true MethodAccessGeneratorTest
+ * @summary Verify the bytecode generated by MethodAccessor/ConstructorAccessor
+ */
+
+import java.lang.reflect.Method;
+import java.lang.reflect.Constructor;
+import jdk.internal.reflect.ReflectionFactory;
+import jdk.internal.reflect.MethodAccessor;
+import jdk.internal.reflect.ConstructorAccessor;
+
+public class MethodAccessGeneratorTest {
+ public static void main(String[] args) throws Exception {
+ doMethodTests();
+ doConstructorTests();
+ }
+
+ private static void doMethodTests() throws Exception {
+ final Method method =
MethodAccessGeneratorTest.class.getMethod("doSomething", String.class);
+ final MethodAccessor methodAccessor =
ReflectionFactory.getReflectionFactory().newMethodAccessor(method);
+ if
(!methodAccessor.getClass().getName().startsWith("jdk.internal.reflect.GeneratedMethodAccessor"))
{
+ throw new RuntimeException("Unexpected MethodAccessor
class " + methodAccessor.getClass().getName());
+ }
+ final MethodAccessGeneratorTest self = new
MethodAccessGeneratorTest();
+ // intentionally pass wrong args
+ try {
+ methodAccessor.invoke(self, null);
+ throw new RuntimeException("Expected a
IllegalArgumentException, but didn't receive one");
+ } catch (IllegalArgumentException iae) {
+ assertMessageNotNPE(iae);
+ }
+ try {
+ methodAccessor.invoke(self, new String[] {"one",
"two"});
+ throw new RuntimeException("Expected a
IllegalArgumentException, but didn't receive one");
+ } catch (IllegalArgumentException iae) {
+ assertMessageNotNPE(iae);
+ }
+ // pass valid args
+ methodAccessor.invoke(self, new String[] {"hello world"});
+ methodAccessor.invoke(self, new Object[] {null});
+ }
+
+ private static void doConstructorTests() throws Exception {
+ final Constructor constructor =
SomeClass.class.getConstructor(new Class[] {String.class});
+ final ConstructorAccessor constructorAccessor =
ReflectionFactory.getReflectionFactory().newConstructorAccessor(constructor);
+ if
(!constructorAccessor.getClass().getName().startsWith("jdk.internal.reflect.GeneratedConstructorAccessor"))
{
+ throw new RuntimeException("Unexpected MethodAccessor
class " + constructorAccessor.getClass().getName());
+ }
+ // intentionally pass wrong args
+ try {
+ constructorAccessor.newInstance(null);
+ throw new RuntimeException("Expected a
IllegalArgumentException, but didn't receive one");
+ } catch (IllegalArgumentException iae) {
+ assertMessageNotNPE(iae);
+ }
+ try {
+ constructorAccessor.newInstance(new String[] {"one",
"two"});
+ throw new RuntimeException("Expected a
IllegalArgumentException, but didn't receive one");
+ } catch (IllegalArgumentException iae) {
+ assertMessageNotNPE(iae);
+ }
+ // pass valid args
+ constructorAccessor.newInstance(new String[] {"hello world"});
+ constructorAccessor.newInstance(new Object[] {null});
+ }
+
+ private static void assertMessageNotNPE(final IllegalArgumentException
iae) {
+ if (iae.getMessage() != null &&
iae.getMessage().contains(NullPointerException.class.getName())) {
+ throw new RuntimeException("IllegalArgumentException's
message contains NullPointerException", iae);
+ }
+ }
+
+ public void doSomething(final String foo) {
+ }
+
+ private static class SomeClass {
+ public SomeClass(final String val) {
+ }
+ }
+}
