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

[Jaikiran Pai]

Mandy, thank you for reviewing this change. Comments inline.


On 11/05/18 11:10 PM, mandy chung wrote:
>
> 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.

I'm open to adding an exception message (just like what the Native 
method accessor does right now). I'll update the patch and send a 
updated version soon.
>
> 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.
>

Sure. I too am pretty new to this code (and OpenJDK in general) so would 
like more reviews, especially the part where this introduces an 
additional "aload" instruction.

-Jaikiran


> 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
>>>
>>
>