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

[mandy chung]

Hi Jaikiran,

Thanks for the contribution.

With your patch, IAE thrown with empty message is less desirable even 
though it does not provide less information than the current message 
(NPE::toString).   I wonder if we could define an exception message in 
MethodAccessorImpl for this IAE or even better null parameter is passed in.

Furthermore, this code has been written for a long time and not many one 
is close to it.  It would require someone to study the implementation 
and generated code to review your patch properly. I'll be on vacation 
next week.  I may be able to help when I return.

Mandy

On 5/10/18 8:21 PM, Jaikiran Pai wrote:
> 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
>>
>