You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by ma...@apache.org on 2015/04/28 01:01:16 UTC
svn commit: r1676393 - in /tomcat/trunk:
java/org/apache/el/parser/AstValue.java
test/org/apache/el/TestMethodExpressionImpl.java
Author: markt
Date: Mon Apr 27 23:01:16 2015
New Revision: 1676393
URL: http://svn.apache.org/r1676393
Log:
Add some comments to clarify behaviour.
Review by schultz re object allocation
Modified:
tomcat/trunk/java/org/apache/el/parser/AstValue.java
tomcat/trunk/test/org/apache/el/TestMethodExpressionImpl.java
Modified: tomcat/trunk/java/org/apache/el/parser/AstValue.java
URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/el/parser/AstValue.java?rev=1676393&r1=1676392&r2=1676393&view=diff
==============================================================================
--- tomcat/trunk/java/org/apache/el/parser/AstValue.java (original)
+++ tomcat/trunk/java/org/apache/el/parser/AstValue.java Mon Apr 27 23:01:16 2015
@@ -41,6 +41,9 @@ import org.apache.el.util.ReflectionUtil
*/
public final class AstValue extends SimpleNode {
+ private static final Object[] EMPTY_ARRAY = new Object[0];
+ private static final Object[] ARRAY_OF_SINGLE_NULL = new Object[1];
+
protected static class Target {
protected Object base;
@@ -263,7 +266,8 @@ public final class AstValue extends Simp
private Object[] convertArgs(EvaluationContext ctx, Object[] src, Method m) {
Class<?>[] types = m.getParameterTypes();
if (types.length == 0) {
- return new Object[0];
+ // Treated as if parameters have been provided so src is ignored
+ return EMPTY_ARRAY;
}
int paramCount = types.length;
@@ -271,23 +275,24 @@ public final class AstValue extends Simp
if (m.isVarArgs() && paramCount > 1 && (src == null || paramCount > src.length) ||
!m.isVarArgs() && (paramCount > 0 && src == null ||
src != null && src.length != paramCount)) {
- String inputParamCount = null;
+ String srcCount = null;
if (src != null) {
- inputParamCount = Integer.toString(src.length);
+ srcCount = Integer.toString(src.length);
}
String msg;
if (m.isVarArgs()) {
msg = MessageFactory.get("error.invoke.tooFewParams",
- m.getName(), inputParamCount, Integer.toString(paramCount));
+ m.getName(), srcCount, Integer.toString(paramCount));
} else {
msg = MessageFactory.get("error.invoke.wrongParams",
- m.getName(), inputParamCount, Integer.toString(paramCount));
+ m.getName(), srcCount, Integer.toString(paramCount));
}
throw new IllegalArgumentException(msg);
}
if (src == null) {
- return new Object[1];
+ // Must be a varargs method with a single parameter.
+ return ARRAY_OF_SINGLE_NULL;
}
Object[] dest = new Object[paramCount];
Modified: tomcat/trunk/test/org/apache/el/TestMethodExpressionImpl.java
URL: http://svn.apache.org/viewvc/tomcat/trunk/test/org/apache/el/TestMethodExpressionImpl.java?rev=1676393&r1=1676392&r2=1676393&view=diff
==============================================================================
--- tomcat/trunk/test/org/apache/el/TestMethodExpressionImpl.java (original)
+++ tomcat/trunk/test/org/apache/el/TestMethodExpressionImpl.java Mon Apr 27 23:01:16 2015
@@ -467,6 +467,15 @@ public class TestMethodExpressionImpl {
@Test
+ public void testBug53792d() {
+ MethodExpression me = factory.createMethodExpression(context,
+ "#{beanB.sayHello().length()}", null, new Class<?>[] {});
+ Integer result = (Integer) me.invoke(context, new Object[] { "foo" });
+ assertEquals(beanB.sayHello().length(), result.intValue());
+ }
+
+
+ @Test
public void testBug56797a() {
MethodExpression me = factory.createMethodExpression(context,
"${beanAA.echo1('Hello World!')}", null , null);
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1676393 - in /tomcat/trunk:
java/org/apache/el/parser/AstValue.java
test/org/apache/el/TestMethodExpressionImpl.java
Posted by Christopher Schultz <ch...@christopherschultz.net>.
Mark,
On 4/28/15 5:26 PM, Mark Thomas wrote:
> On 28/04/2015 22:21, Christopher Schultz wrote:
>> Mark,
>>
>> On 4/27/15 7:01 PM, markt@apache.org wrote:
>>> Author: markt
>>> Date: Mon Apr 27 23:01:16 2015
>>> New Revision: 1676393
>>>
>>> URL: http://svn.apache.org/r1676393
>>> Log:
>>> Add some comments to clarify behaviour.
>>> Review by schultz re object allocation
>>>
>>> Modified:
>>> tomcat/trunk/java/org/apache/el/parser/AstValue.java
>>> tomcat/trunk/test/org/apache/el/TestMethodExpressionImpl.java
>>>
>>> Modified: tomcat/trunk/java/org/apache/el/parser/AstValue.java
>>> URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/el/parser/AstValue.java?rev=1676393&r1=1676392&r2=1676393&view=diff
>>> ==============================================================================
>>> --- tomcat/trunk/java/org/apache/el/parser/AstValue.java (original)
>>> +++ tomcat/trunk/java/org/apache/el/parser/AstValue.java Mon Apr 27 23:01:16 2015
>>> @@ -41,6 +41,9 @@ import org.apache.el.util.ReflectionUtil
>>> */
>>> public final class AstValue extends SimpleNode {
>>>
>>> + private static final Object[] EMPTY_ARRAY = new Object[0];
>>> + private static final Object[] ARRAY_OF_SINGLE_NULL = new Object[1];
>>> +
>>> protected static class Target {
>>> protected Object base;
>>>
>>> @@ -263,7 +266,8 @@ public final class AstValue extends Simp
>>> private Object[] convertArgs(EvaluationContext ctx, Object[] src, Method m) {
>>> Class<?>[] types = m.getParameterTypes();
>>> if (types.length == 0) {
>>> - return new Object[0];
>>> + // Treated as if parameters have been provided so src is ignored
>>> + return EMPTY_ARRAY;
>>> }
>>>
>>> int paramCount = types.length;
>>> @@ -271,23 +275,24 @@ public final class AstValue extends Simp
>>> if (m.isVarArgs() && paramCount > 1 && (src == null || paramCount > src.length) ||
>>> !m.isVarArgs() && (paramCount > 0 && src == null ||
>>> src != null && src.length != paramCount)) {
>>> - String inputParamCount = null;
>>> + String srcCount = null;
>>> if (src != null) {
>>> - inputParamCount = Integer.toString(src.length);
>>> + srcCount = Integer.toString(src.length);
>>> }
>>> String msg;
>>> if (m.isVarArgs()) {
>>> msg = MessageFactory.get("error.invoke.tooFewParams",
>>> - m.getName(), inputParamCount, Integer.toString(paramCount));
>>> + m.getName(), srcCount, Integer.toString(paramCount));
>>> } else {
>>> msg = MessageFactory.get("error.invoke.wrongParams",
>>> - m.getName(), inputParamCount, Integer.toString(paramCount));
>>> + m.getName(), srcCount, Integer.toString(paramCount));
>>> }
>>> throw new IllegalArgumentException(msg);
>>> }
>>>
>>> if (src == null) {
>>> - return new Object[1];
>>> + // Must be a varargs method with a single parameter.
>>> + return ARRAY_OF_SINGLE_NULL;
>>
>> Is this safe? I'm not sure of the scope of this array, but client code
>> could potentially mutate the contents. We fully expect
>> ARRAY_OF_SINGLE_NULL[0] to be == null, but some other code could change
>> it and it would probably cause a huge mess.
>>
>> I like the use of a flyweight here, but if that object ever gets out of
>> our control, it could be poisoned.
>
> Untrusted apps are a minority use case but they do exist. I think you
> are right and I'll use a new array every time.
I wasn't sure how far this object would go, but I guess it might
ultimately end up as the parameter to a method controlled by the
application.
There might be a security vulnerability there, like triggering a call to
an evil method to trigger that array to be passed to it, and then
changing the value. Then calling an important method with a null
argument to maybe pass some argument-checking or something. I can't
think of a use-case off the top of my head but someone more creative
than I might be able to.
Since it would affect other web applications running on the same
container, it wouldn't just be dangerous to a single web app.
This is a relatively unlikely case, so I think object-allocation isn't
too much of a concern.
-chris
Re: svn commit: r1676393 - in /tomcat/trunk: java/org/apache/el/parser/AstValue.java
test/org/apache/el/TestMethodExpressionImpl.java
Posted by Mark Thomas <ma...@apache.org>.
On 28/04/2015 22:21, Christopher Schultz wrote:
> Mark,
>
> On 4/27/15 7:01 PM, markt@apache.org wrote:
>> Author: markt
>> Date: Mon Apr 27 23:01:16 2015
>> New Revision: 1676393
>>
>> URL: http://svn.apache.org/r1676393
>> Log:
>> Add some comments to clarify behaviour.
>> Review by schultz re object allocation
>>
>> Modified:
>> tomcat/trunk/java/org/apache/el/parser/AstValue.java
>> tomcat/trunk/test/org/apache/el/TestMethodExpressionImpl.java
>>
>> Modified: tomcat/trunk/java/org/apache/el/parser/AstValue.java
>> URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/el/parser/AstValue.java?rev=1676393&r1=1676392&r2=1676393&view=diff
>> ==============================================================================
>> --- tomcat/trunk/java/org/apache/el/parser/AstValue.java (original)
>> +++ tomcat/trunk/java/org/apache/el/parser/AstValue.java Mon Apr 27 23:01:16 2015
>> @@ -41,6 +41,9 @@ import org.apache.el.util.ReflectionUtil
>> */
>> public final class AstValue extends SimpleNode {
>>
>> + private static final Object[] EMPTY_ARRAY = new Object[0];
>> + private static final Object[] ARRAY_OF_SINGLE_NULL = new Object[1];
>> +
>> protected static class Target {
>> protected Object base;
>>
>> @@ -263,7 +266,8 @@ public final class AstValue extends Simp
>> private Object[] convertArgs(EvaluationContext ctx, Object[] src, Method m) {
>> Class<?>[] types = m.getParameterTypes();
>> if (types.length == 0) {
>> - return new Object[0];
>> + // Treated as if parameters have been provided so src is ignored
>> + return EMPTY_ARRAY;
>> }
>>
>> int paramCount = types.length;
>> @@ -271,23 +275,24 @@ public final class AstValue extends Simp
>> if (m.isVarArgs() && paramCount > 1 && (src == null || paramCount > src.length) ||
>> !m.isVarArgs() && (paramCount > 0 && src == null ||
>> src != null && src.length != paramCount)) {
>> - String inputParamCount = null;
>> + String srcCount = null;
>> if (src != null) {
>> - inputParamCount = Integer.toString(src.length);
>> + srcCount = Integer.toString(src.length);
>> }
>> String msg;
>> if (m.isVarArgs()) {
>> msg = MessageFactory.get("error.invoke.tooFewParams",
>> - m.getName(), inputParamCount, Integer.toString(paramCount));
>> + m.getName(), srcCount, Integer.toString(paramCount));
>> } else {
>> msg = MessageFactory.get("error.invoke.wrongParams",
>> - m.getName(), inputParamCount, Integer.toString(paramCount));
>> + m.getName(), srcCount, Integer.toString(paramCount));
>> }
>> throw new IllegalArgumentException(msg);
>> }
>>
>> if (src == null) {
>> - return new Object[1];
>> + // Must be a varargs method with a single parameter.
>> + return ARRAY_OF_SINGLE_NULL;
>
> Is this safe? I'm not sure of the scope of this array, but client code
> could potentially mutate the contents. We fully expect
> ARRAY_OF_SINGLE_NULL[0] to be == null, but some other code could change
> it and it would probably cause a huge mess.
>
> I like the use of a flyweight here, but if that object ever gets out of
> our control, it could be poisoned.
Untrusted apps are a minority use case but they do exist. I think you
are right and I'll use a new array every time.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1676393 - in /tomcat/trunk:
java/org/apache/el/parser/AstValue.java
test/org/apache/el/TestMethodExpressionImpl.java
Posted by Christopher Schultz <ch...@christopherschultz.net>.
Mark,
On 4/27/15 7:01 PM, markt@apache.org wrote:
> Author: markt
> Date: Mon Apr 27 23:01:16 2015
> New Revision: 1676393
>
> URL: http://svn.apache.org/r1676393
> Log:
> Add some comments to clarify behaviour.
> Review by schultz re object allocation
>
> Modified:
> tomcat/trunk/java/org/apache/el/parser/AstValue.java
> tomcat/trunk/test/org/apache/el/TestMethodExpressionImpl.java
>
> Modified: tomcat/trunk/java/org/apache/el/parser/AstValue.java
> URL: http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/el/parser/AstValue.java?rev=1676393&r1=1676392&r2=1676393&view=diff
> ==============================================================================
> --- tomcat/trunk/java/org/apache/el/parser/AstValue.java (original)
> +++ tomcat/trunk/java/org/apache/el/parser/AstValue.java Mon Apr 27 23:01:16 2015
> @@ -41,6 +41,9 @@ import org.apache.el.util.ReflectionUtil
> */
> public final class AstValue extends SimpleNode {
>
> + private static final Object[] EMPTY_ARRAY = new Object[0];
> + private static final Object[] ARRAY_OF_SINGLE_NULL = new Object[1];
> +
> protected static class Target {
> protected Object base;
>
> @@ -263,7 +266,8 @@ public final class AstValue extends Simp
> private Object[] convertArgs(EvaluationContext ctx, Object[] src, Method m) {
> Class<?>[] types = m.getParameterTypes();
> if (types.length == 0) {
> - return new Object[0];
> + // Treated as if parameters have been provided so src is ignored
> + return EMPTY_ARRAY;
> }
>
> int paramCount = types.length;
> @@ -271,23 +275,24 @@ public final class AstValue extends Simp
> if (m.isVarArgs() && paramCount > 1 && (src == null || paramCount > src.length) ||
> !m.isVarArgs() && (paramCount > 0 && src == null ||
> src != null && src.length != paramCount)) {
> - String inputParamCount = null;
> + String srcCount = null;
> if (src != null) {
> - inputParamCount = Integer.toString(src.length);
> + srcCount = Integer.toString(src.length);
> }
> String msg;
> if (m.isVarArgs()) {
> msg = MessageFactory.get("error.invoke.tooFewParams",
> - m.getName(), inputParamCount, Integer.toString(paramCount));
> + m.getName(), srcCount, Integer.toString(paramCount));
> } else {
> msg = MessageFactory.get("error.invoke.wrongParams",
> - m.getName(), inputParamCount, Integer.toString(paramCount));
> + m.getName(), srcCount, Integer.toString(paramCount));
> }
> throw new IllegalArgumentException(msg);
> }
>
> if (src == null) {
> - return new Object[1];
> + // Must be a varargs method with a single parameter.
> + return ARRAY_OF_SINGLE_NULL;
Is this safe? I'm not sure of the scope of this array, but client code
could potentially mutate the contents. We fully expect
ARRAY_OF_SINGLE_NULL[0] to be == null, but some other code could change
it and it would probably cause a huge mess.
I like the use of a flyweight here, but if that object ever gets out of
our control, it could be poisoned.
Thanks,
-chris