Bug 60844 - ArrayIndexOutOfBoundsException when matching actionListener
Summary: ArrayIndexOutOfBoundsException when matching actionListener
Status: RESOLVED FIXED
Alias: None
Product: Tomcat 8
Classification: Unclassified
Component: EL (show other bugs)
Version: 8.5.x-trunk
Hardware: PC All
: P2 normal (vote)
Target Milestone: ----
Assignee: Tomcat Developers Mailing List
URL:
Keywords: PatchAvailable
Depends on:
Blocks:
 
Reported: 2017-03-10 15:50 UTC by Daniel Gray
Modified: 2017-03-23 21:12 UTC (History)
0 users



Attachments
SVN diff of fix on Apache Tomcat 8.5.x-trunk repo (1.06 KB, patch)
2017-03-10 15:50 UTC, Daniel Gray
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Gray 2017-03-10 15:50:36 UTC
Created attachment 34814 [details]
SVN diff of fix on Apache Tomcat 8.5.x-trunk repo

The bug manifested itself when I:
1. Created a custom composite control in JSF.
2. *ERRONEOUSLY* Defined an attribute with a method signature of "void actionListener(object)" -- it should have been "void actionListener(javax.faces.event.AjaxBehaviorEvent)".
3. Trigger a call on the method through JSF.

When matching the method signature, org.apache.el.util.ReflectionUtil throws an ArrayIndexOutOfBoundsException at line 210.

The bug is due to is missing check that paramValues is empty (it only checks if it is not null, and then proceeds to evaluate paramValues[i]). I have downloaded & compiled the Apache Tomcat 8.5.x trunk code, fixed the bug, and tested it by running my app inside the servlet, and it now correctly gives an error about it not finding the desired method that I have bound to the argument.

*IMPROVEMENT SUGGESTION*:

The exception or error message should be more explicit about that EL first looks for a method by the name and then the property. I got really mad after a while wondering *WHY IS IT LOOKING FOR THE PROPERTY IF I EXPLICITLY SAID IT WAS A METHOD?!?!?*, which led to the tinkering with the signature, which led to the small bug discovery. It's just that it's not obvious (from outside) what Tomcat is doing and I think it would be nice to indicate that somehow.
Comment 1 Christopher Schultz 2017-03-10 21:52:29 UTC
Thanks for the patch!

(In reply to Daniel Gray from comment #0)
> The exception or error message should be more explicit about that EL first
> looks for a method by the name and then the property.

Is this not covered by the EL specification? We would prefer not to re-document behavior which is spec-defined.
Comment 2 Daniel Gray 2017-03-10 22:45:14 UTC
(In reply to Christopher Schultz from comment #1)
> Thanks for the patch!
You're most welcome! :)

> Is this not covered by the EL specification? We would prefer not to
> re-document behavior which is spec-defined.

It probably is, but I haven't read the EL specification. Maybe I should at some point, but I don't know when I'll get around to it. In any case, I think that a lot of developers using Tomcat probably haven't either, and that making the *visible* error communicate a bit more clearly that *neither* the property or method were found, would make _somebody's_ life a tad easier...

I do, however, understand that such a seemingly small change may have many other complex implications that I'm not aware of, so I'm just suggesting it as someone more on the "user" side. :) Silver lining though, is that I found this little bugger!! :)
Comment 3 Christopher Schultz 2017-03-13 14:03:44 UTC
The error message you are likely to receive is something along the lines of "no such method or property found: actionListener". Do you think that's a good place to explain how to use a particular API? Something like "no such method of property found: actionListener (remember: methods are searched before properties)" or something to that effect? It seems a little silly to me.

When you get a FileNotFoundException, the error message doesn't tell you that the permissions of the directories all the way back to the root of the filesystem are significant and might be the reason why you can't find/read your file.
Comment 4 Mark Thomas 2017-03-23 21:12:15 UTC
Thanks for the report and the patch. I added a test case and fixed it for all currently supported versions.

I don't think the error message should be adjusted. EL resolution is non-trivial and the spec covers it in detail.

Fixed in:
- trunk for 9.0.0.M19 onwards
- 8.5.x for 8.5.13 onwards
- 8.0.x for 8.0.43 onwards
- 7.0.x for 7.0.77 onwards