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, ma...@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
signature.asc
Description: OpenPGP digital signature
