Bug 57855 - Invoke MethodExpression with wrong pram count results in ArrayIndexOutOfBoundsException
Invoke MethodExpression with wrong pram count results in ArrayIndexOutOfBound...
Status: RESOLVED FIXED
Product: Tomcat 8
Classification: Unclassified
Component: EL
8.0.x-trunk
PC All
: P2 normal with 5 votes (vote)
: ----
Assigned To: Tomcat Developers Mailing List
:
Depends on:
Blocks:
  Show dependency tree
 
Reported: 2015-04-24 14:04 UTC by Christian Strebel
Modified: 2015-04-27 21:45 UTC (History)
0 users



Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Christian Strebel 2015-04-24 14:04:23 UTC
To reproduce add the following test code to TestMethodExpressionImpl:

@Test(expected=IllegalArgumentException.class)
public void test() {
    MethodExpression me = factory.createMethodExpression(context,
            "${beanAA.echo2}", null , new Class[]{String.class});
    me.invoke(context, new Object[0]);
}

The result is an ArrayIndexOutOfBoundsException. I would expect an IllegalArgumentException or a MethodNotFoundException.
The same I would expect if I invoke with null:

@Test(expected=IllegalArgumentException.class)
public void testNull() {
    MethodExpression me = factory.createMethodExpression(context,
            "${beanAA.echo2}", null , new Class[]{String.class});
    me.invoke(context, null);
}

But this throws a NullPointerException instead.
I run into this because PrimeFaces handles AjaxListeners relatively bad and expect that there is a MethodNotFoundException or an IllegalArgumentException if they call a listener method with no pram.

try {
    listener.invoke(elContext, new Object[]{});
} 
catch (MethodNotFoundException mnfe) {
    processArgListener(context, elContext, event);
} 
catch (IllegalArgumentException iae) {
    processArgListener(context, elContext, event);
}
Comment 1 Mark Thomas 2015-04-27 09:23:10 UTC
I've looked through the EL specification and I don't see the expected behaviour defined for any of these cases.

I don't think MethodNotFoundException is the right exception in this case since the method has been found. IllegalArgumentException strikes me as the better fit. I'll take a look.
Comment 2 Mark Thomas 2015-04-27 09:53:59 UTC
Thanks for the report.

This has been fixed in trunk (9.0.x), 8.0.x/trunk (for 8.0.22 onwards) and 7.0.x/trunk (for 7.0.62 onwards).
Comment 3 Konstantin Kolinko 2015-04-27 11:40:30 UTC
Reviewing r1676234

Apparently "null" src is equivalent to "no argument" or a zero-length array. [1]

1) When null is passed it should support both varargs and non-varargs cases.

E.g. calling Foo.main().

From the code of AstValue.hava I think that "paramCount > 0 && src == null" condition fails when the only argument is varargs and src is null.

2) It looks that the message in AstValue.java prints "The method [{0}] was called with [null] parameters" when null is passed as src. Wouldn't it better to print "0" instead of "null" ?




[1] http://docs.oracle.com/javaee/7/api/javax/el/MethodExpression.html#invoke%28javax.el.ELContext,%20java.lang.Object[]%29
Comment 4 Mark Thomas 2015-04-27 11:57:24 UTC
(In reply to Konstantin Kolinko from comment #3)
> Reviewing r1676234
> 
> Apparently "null" src is equivalent to "no argument" or a zero-length array.
> [1]
> 
> 1) When null is passed it should support both varargs and non-varargs cases.
> 
> E.g. calling Foo.main().

ACK. I'll add a test case for that.

> From the code of AstValue.hava I think that "paramCount > 0 && src == null"
> condition fails when the only argument is varargs and src is null.
> 
> 2) It looks that the message in AstValue.java prints "The method [{0}] was
> called with [null] parameters" when null is passed as src. Wouldn't it
> better to print "0" instead of "null" ?

I'll like to be able to differentiate between a zero length array and null.

> [1]
> http://docs.oracle.com/javaee/7/api/javax/el/MethodExpression.
> html#invoke%28javax.el.ELContext,%20java.lang.Object[]%29
Comment 5 Konstantin Kolinko 2015-04-27 12:50:44 UTC
I think MethodExpression.invoke() shall wrap IllegalArgumentException into an ELException.


It is documented that java.lang.reflect.Method.invoke() throws IllegalArgumentException when "if the number of actual and formal parameters differ" [2].

I think that our code should work as if that exception were thrown by Method.invoke(). From MethodExpression.invoke() [1] I think that an exception that is received from a Method.invoke() has to be wrapped with an ELException.


Essentially this means to move "values = convertArgs(values, m);" call into try/catch block that is around "result = m.invoke(t.base, values);" call. (lines 274 278 of AstValue.java of Tomcat 7 r1676234).


[1] http://docs.oracle.com/javaee/7/api/javax/el/MethodExpression.html#invoke%28javax.el.ELContext,%20java.lang.Object[]%29

[2] http://docs.oracle.com/javase/8/docs/api/java/lang/reflect/Method.html#invoke-java.lang.Object-java.lang.Object...-
Comment 6 Konstantin Kolinko 2015-04-27 13:46:01 UTC
(In reply to Mark Thomas from comment #4)
> (In reply to Konstantin Kolinko from comment #3)
> > Reviewing r1676234
> > 
> > 2) It looks that the message in AstValue.java prints "The method [{0}] was
> > called with [null] parameters" when null is passed as src. Wouldn't it
> > better to print "0" instead of "null" ?
> 
> I'll like to be able to differentiate between a zero length array and null.
> 

E.g. end user may call the method without any arguments,  but some higher API that translates that into a call to MethodExpression is allowed to optimize that to pass "null" instead of allocating a zero-length array of arguments.

Printing a message that says "null" may be confusing for the end user.
Comment 7 Christian Strebel 2015-04-27 14:16:56 UTC
(In reply to Konstantin Kolinko from comment #5)
> I think MethodExpression.invoke() shall wrap IllegalArgumentException into
> an ELException.
> 
> 
> It is documented that java.lang.reflect.Method.invoke() throws
> IllegalArgumentException when "if the number of actual and formal parameters
> differ" [2].
> 
> I think that our code should work as if that exception were thrown by
> Method.invoke(). From MethodExpression.invoke() [1] I think that an
> exception that is received from a Method.invoke() has to be wrapped with an
> ELException.
> 
> Essentially this means to move "values = convertArgs(values, m);" call into
> try/catch block that is around "result = m.invoke(t.base, values);" call.
> (lines 274 278 of AstValue.java of Tomcat 7 r1676234).

The problem is, the "bad" PrimeFaces AjaxBehaviorListenerImpl implementation is then broken again. Also I do not like wrapping a specific RuntimeException into an unspecific RuntimeExcpetion without additional information.
But on the other hand you are right there is already a wrapping if an IllegalAccessException or an IllegalArgumentException is thrown by Method.invoke().
I also do not really like this wrapping. The Sun/Glassfish implementation does not wrap the runtime exceptions thrown by Method.invoke() as well.
Comment 8 Mark Thomas 2015-04-27 20:44:32 UTC
The spec is unclear exactly what the required behaviour here is so (ignoring Konstantin's issue one above) I'm happy that this is an improvement. IAE with a decent error message is much better than AIOOB or NPE.

What the 'right' behaviour is is TBD. I have created https://java.net/jira/browse/EL_SPEC-24 to track this. Do add any comments you have for the EL expert group to that Jira ticket.

If you haven't already, I'd strongly recommend you raise a bug against PrimeFaces.

Until there is some clarification from the EL EG, I'm happy (once issue 1 is fixed) with throwing an IAE in this case.
Comment 9 Mark Thomas 2015-04-27 21:45:38 UTC
Issue 1 fixed in the same set of versions as comment #2