[PATCH] JDK-8159797 Throw a right IllegalArgumentException from the bytecode generated by Method/ConstructorAccessor

[Jaikiran Pai]

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
>