Any reviews/sponsors please?
-Jaikiran
On 03/05/18 6:24 PM, Jaikiran Pai wrote:
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
