From ebc24e5f517efbc9d082d438f22818b0eeaf5d4b Mon Sep 17 00:00:00 2001 From: henrib Date: Wed, 28 Nov 2018 18:07:05 +0100 Subject: [PATCH 1/3] JEXL-279: refined logic to better capture null variables dereferencing in interpreter --- RELEASE-NOTES.txt | 1 + .../org/apache/commons/jexl3/JexlInfo.java | 3 +- .../commons/jexl3/internal/Interpreter.java | 15 ++++---- .../internal/introspection/MethodKey.java | 8 +++-- src/site/xdoc/changes.xml | 3 ++ .../apache/commons/jexl3/Issues200Test.java | 35 +++++++++++++++++++ 6 files changed, 55 insertions(+), 10 deletions(-) diff --git a/RELEASE-NOTES.txt b/RELEASE-NOTES.txt index 9ef3f8b2d..f49554f64 100644 --- a/RELEASE-NOTES.txt +++ b/RELEASE-NOTES.txt @@ -60,6 +60,7 @@ New Features in 3.2: Bugs Fixed in 3.2: ================== +* JEXL-279: Null variables property access do not throw exceptions * JEXL-278: Ambiguous exceptions should point to actual statement ambiguity * JEXL-272: Dereferencing null property not reported on method call * JEXL-271: Hoisted variable is lost when currying lambda diff --git a/src/main/java/org/apache/commons/jexl3/JexlInfo.java b/src/main/java/org/apache/commons/jexl3/JexlInfo.java index c0abe07c2..19a4a72fc 100644 --- a/src/main/java/org/apache/commons/jexl3/JexlInfo.java +++ b/src/main/java/org/apache/commons/jexl3/JexlInfo.java @@ -90,7 +90,8 @@ public JexlInfo() { if (!className.equals(cname)) { // go deeper if called from jexl implementation classes if (className.startsWith(pkgname + ".internal.") - || className.startsWith(pkgname + ".Jexl")) { + || className.startsWith(pkgname + ".Jexl") + || className.startsWith(pkgname + ".parser")) { cname = className; } else { break; diff --git a/src/main/java/org/apache/commons/jexl3/internal/Interpreter.java b/src/main/java/org/apache/commons/jexl3/internal/Interpreter.java index 11402f310..76d3b6800 100644 --- a/src/main/java/org/apache/commons/jexl3/internal/Interpreter.java +++ b/src/main/java/org/apache/commons/jexl3/internal/Interpreter.java @@ -1117,6 +1117,13 @@ protected Object visit(ASTReference node, Object data) { } else { antish = false; } + } else if (objectNode instanceof ASTArrayAccess) { + if (object == null) { + ptyNode = objectNode; + break; + } else { + antish = false; + } } // attempt to evaluate the property within the object (visit(ASTIdentifierAccess node)) object = objectNode.jjtAccept(this, object); @@ -1129,14 +1136,10 @@ protected Object visit(ASTReference node, Object data) { JexlNode first = node.jjtGetChild(0); if (first instanceof ASTIdentifier) { ASTIdentifier afirst = (ASTIdentifier) first; - if (afirst.getSymbol() < 0) { - ant = new StringBuilder(afirst.getName()); - } else { - break main; - } + ant = new StringBuilder(afirst.getName()); } else { ptyNode = objectNode; - break main; + break; } } for (; v <= c; ++v) { diff --git a/src/main/java/org/apache/commons/jexl3/internal/introspection/MethodKey.java b/src/main/java/org/apache/commons/jexl3/internal/introspection/MethodKey.java index b0c3cae77..229534d7b 100644 --- a/src/main/java/org/apache/commons/jexl3/internal/introspection/MethodKey.java +++ b/src/main/java/org/apache/commons/jexl3/internal/introspection/MethodKey.java @@ -383,9 +383,11 @@ private static boolean isInvocationConvertible( /* Primitive conversion check. */ if (formal.isPrimitive()) { Class[] clist = strict ? STRICT_CONVERTIBLES.get(formal) : CONVERTIBLES.get(formal); - for(int c = 0; c < clist.length; ++c) { - if (actual == clist[c]) { - return true; + if (clist != null) { + for (int c = 0; c < clist.length; ++c) { + if (actual == clist[c]) { + return true; + } } } return false; diff --git a/src/site/xdoc/changes.xml b/src/site/xdoc/changes.xml index e2316d4ee..a4869d733 100644 --- a/src/site/xdoc/changes.xml +++ b/src/site/xdoc/changes.xml @@ -26,6 +26,9 @@ + + Null variables property access do not throw exceptions + Ambiguous exceptions should point to actual statement ambiguity diff --git a/src/test/java/org/apache/commons/jexl3/Issues200Test.java b/src/test/java/org/apache/commons/jexl3/Issues200Test.java index b949da8dd..8d520aab6 100644 --- a/src/test/java/org/apache/commons/jexl3/Issues200Test.java +++ b/src/test/java/org/apache/commons/jexl3/Issues200Test.java @@ -631,4 +631,39 @@ public void test278() throws Exception { Assert.assertEquals(src, ctls[i], value); } } + + @Test + public void test279() throws Exception { + Object result; + JexlScript script; + JexlContext ctxt = new MapContext(); + ctxt.set("y.z", null); + ctxt.set("z", null); + String[] srcs = new String[]{ + "var x = null; x[0];", + "var x = null; x.0;", + "z[0]", + "z.0", + "y.z[0]", + "y.z.0", + }; + for(boolean strict : new boolean[]{ true, false} ) { + JexlEngine jexl = new JexlBuilder().safe(false).strict(strict).create(); + for (String src : srcs) { + script = jexl.createScript(src); + try { + result = script.execute(ctxt); + if (strict) { + Assert.fail("should have failed: " + src); + } + // not reachable + Assert.assertNull("non-null result ?!", result); + } catch (JexlException xany) { + if (!strict) { + Assert.fail(src + ", should not have thrown " + xany); + } + } + } + } + } } From 51a58ed28ce0a530c7854aa863849e47de294356 Mon Sep 17 00:00:00 2001 From: henrib Date: Mon, 17 Dec 2018 16:10:55 +0100 Subject: [PATCH 2/3] JEXL-281: refined logic in MethodKey.isVarArgs avoiding costly class hierarchy walk --- .../jexl3/internal/introspection/MethodExecutor.java | 10 ++++------ .../jexl3/internal/introspection/MethodKey.java | 2 +- 2 files changed, 5 insertions(+), 7 deletions(-) diff --git a/src/main/java/org/apache/commons/jexl3/internal/introspection/MethodExecutor.java b/src/main/java/org/apache/commons/jexl3/internal/introspection/MethodExecutor.java index 61a45e7ac..17a66e16e 100644 --- a/src/main/java/org/apache/commons/jexl3/internal/introspection/MethodExecutor.java +++ b/src/main/java/org/apache/commons/jexl3/internal/introspection/MethodExecutor.java @@ -70,13 +70,11 @@ private MethodExecutor(Class c, java.lang.reflect.Method m, MethodKey k) { super(c, m, k); int vastart = -1; Class vaclass = null; - if (method != null) { - Class[] formal = method.getParameterTypes(); + if (MethodKey.isVarArgs(method)) { // if the last parameter is an array, the method is considered as vararg - if (formal != null && MethodKey.isVarArgs(method)) { - vastart = formal.length - 1; - vaclass = formal[vastart].getComponentType(); - } + Class[] formal = method.getParameterTypes(); + vastart = formal.length - 1; + vaclass = formal[vastart].getComponentType(); } vaStart = vastart; vaClass = vaclass; diff --git a/src/main/java/org/apache/commons/jexl3/internal/introspection/MethodKey.java b/src/main/java/org/apache/commons/jexl3/internal/introspection/MethodKey.java index 229534d7b..1977d14c6 100644 --- a/src/main/java/org/apache/commons/jexl3/internal/introspection/MethodKey.java +++ b/src/main/java/org/apache/commons/jexl3/internal/introspection/MethodKey.java @@ -201,7 +201,7 @@ public static boolean isVarArgs(final Method method) { } // before climbing up the hierarchy, verify that the last parameter is an array final Class[] ptypes = method.getParameterTypes(); - if (ptypes.length > 0 && ptypes[ptypes.length - 1].getComponentType() == null) { + if (ptypes.length == 0 || ptypes[ptypes.length - 1].getComponentType() == null) { return false; } final String mname = method.getName(); From e7d212e423cd409bdd049864e77b0521ea5e4c10 Mon Sep 17 00:00:00 2001 From: henrib Date: Mon, 17 Dec 2018 16:42:49 +0100 Subject: [PATCH 3/3] JEXL-281: releases notes and changes --- RELEASE-NOTES.txt | 1 + src/site/xdoc/changes.xml | 3 +++ 2 files changed, 4 insertions(+) diff --git a/RELEASE-NOTES.txt b/RELEASE-NOTES.txt index f49554f64..67ddfea66 100644 --- a/RELEASE-NOTES.txt +++ b/RELEASE-NOTES.txt @@ -60,6 +60,7 @@ New Features in 3.2: Bugs Fixed in 3.2: ================== +* JEXL-281: MethodExecutor incorrectly tests for empty parameters list * JEXL-279: Null variables property access do not throw exceptions * JEXL-278: Ambiguous exceptions should point to actual statement ambiguity * JEXL-272: Dereferencing null property not reported on method call diff --git a/src/site/xdoc/changes.xml b/src/site/xdoc/changes.xml index a4869d733..3dfadb1be 100644 --- a/src/site/xdoc/changes.xml +++ b/src/site/xdoc/changes.xml @@ -26,6 +26,9 @@ + + MethodExecutor incorrectly tests for empty parameters list + Null variables property access do not throw exceptions