diff --git a/java/ql/src/Performance/ConcatenationInLoops.ql b/java/ql/src/Performance/ConcatenationInLoops.ql index e4280678e4be..fd0ce673a1df 100644 --- a/java/ql/src/Performance/ConcatenationInLoops.ql +++ b/java/ql/src/Performance/ConcatenationInLoops.ql @@ -40,19 +40,26 @@ predicate useAndDef(Assignment a, Variable v) { ) } -predicate declaredInLoop(LocalVariableDecl v, LoopStmt loop) { - exists(LocalVariableDeclExpr e | - e.getVariable() = v and - e.getEnclosingStmt().getEnclosingStmt*() = loop.getBody() - ) +/** + * Holds if `e` is executed often in loop `loop`. + */ +predicate executedOften(Assignment a) { + a.getDest().getType() instanceof TypeString and + exists(ControlFlowNode n | a.getControlFlowNode() = n | getADeepSuccessor(n) = n) +} + +/** Gets a sucessor of `n`, also following function calls. */ +ControlFlowNode getADeepSuccessor(ControlFlowNode n) { + result = n.getASuccessor+() or - exists(EnhancedForStmt for | for = loop | for.getVariable().getVariable() = v) + exists(Call c, ControlFlowNode callee | c.(Expr).getControlFlowNode() = n.getASuccessor+() | + callee = c.getCallee().getBody().getControlFlowNode() and + result = getADeepSuccessor(callee) + ) } from Assignment a, Variable v where useAndDef(a, v) and - exists(LoopStmt loop | a.getEnclosingStmt().getEnclosingStmt*() = loop | - not declaredInLoop(v, loop) - ) + executedOften(a) select a, "The string " + v.getName() + " is built-up in a loop: use string buffer." diff --git a/java/ql/test/query-tests/ConcatenationInLoops/ConcatenationInLoops.expected b/java/ql/test/query-tests/ConcatenationInLoops/ConcatenationInLoops.expected new file mode 100644 index 000000000000..12be89018cd7 --- /dev/null +++ b/java/ql/test/query-tests/ConcatenationInLoops/ConcatenationInLoops.expected @@ -0,0 +1,3 @@ +| ConcatenationInLoops.java:9:4:9:10 | ...+=... | The string s is built-up in a loop: use string buffer. | +| ConcatenationInLoops.java:32:6:32:12 | ...+=... | The string s is built-up in a loop: use string buffer. | +| ConcatenationInLoops.java:39:4:39:12 | ...+=... | The string cs is built-up in a loop: use string buffer. | diff --git a/java/ql/test/query-tests/ConcatenationInLoops/ConcatenationInLoops.java b/java/ql/test/query-tests/ConcatenationInLoops/ConcatenationInLoops.java new file mode 100644 index 000000000000..22e46398ed05 --- /dev/null +++ b/java/ql/test/query-tests/ConcatenationInLoops/ConcatenationInLoops.java @@ -0,0 +1,43 @@ +public class ConcatenationInLoops { + private String cs = ""; + + public static void main(String[] args) { + // Random r = 42; + long start = System.currentTimeMillis(); + String s = ""; + for (int i = 0; i < 65536; i++) + s += 42; // $ loopConcat=s + System.out.println(System.currentTimeMillis() - start); // This prints roughly 4500. + + // r = 42; + start = System.currentTimeMillis(); + StringBuilder sb = new StringBuilder(); + for (int i = 0; i < 65536; i++) + sb.append(42);//r.nextInt(2)); + s = sb.toString(); + System.out.println(System.currentTimeMillis() - start); // This prints 5. + + + String s2 = ""; + for (int i = 0; i < 65536; i++) + if (i > 10) { + s += 42; // Will only be executed once. + break; + } + + String s3 = ""; + for (int i = 0; i < 65536; i++) + for (int j = 0; i < 65536; i++) + if (j > 10) { + s += 42; // $ loopConcat=s + break; + } + } + + public void test(int bound) { + if (bound > 0) { + cs += "a"; // $ loopConcat=cs + test(bound - 1); + } + } +} \ No newline at end of file diff --git a/java/ql/test/query-tests/ConcatenationInLoops/ConcatenationInLoops.qlref b/java/ql/test/query-tests/ConcatenationInLoops/ConcatenationInLoops.qlref new file mode 100644 index 000000000000..0afb156f926d --- /dev/null +++ b/java/ql/test/query-tests/ConcatenationInLoops/ConcatenationInLoops.qlref @@ -0,0 +1 @@ +Performance/ConcatenationInLoops.ql \ No newline at end of file diff --git a/java/ql/test/query-tests/ConcatenationInLoops/test.expected b/java/ql/test/query-tests/ConcatenationInLoops/test.expected new file mode 100644 index 000000000000..8ec8033d086e --- /dev/null +++ b/java/ql/test/query-tests/ConcatenationInLoops/test.expected @@ -0,0 +1,2 @@ +testFailures +failures diff --git a/java/ql/test/query-tests/ConcatenationInLoops/test.ql b/java/ql/test/query-tests/ConcatenationInLoops/test.ql new file mode 100644 index 000000000000..dd35dc5e63ce --- /dev/null +++ b/java/ql/test/query-tests/ConcatenationInLoops/test.ql @@ -0,0 +1,82 @@ +import java +import semmle.code.java.dataflow.DataFlow +import TestUtilities.InlineExpectationsTest + +module TheQuery { + import semmle.code.java.Type + import semmle.code.java.Expr + import semmle.code.java.Statement + import semmle.code.java.JDK + + /** + * An assignment of the form + * + * ``` + * v = ... + ... v ... + * ``` + * or + * + * ``` + * v += ... + * ``` + * where `v` is a simple variable (and not, for example, + * an array element). + */ + predicate useAndDef(Assignment a, Variable v) { + a.getDest() = v.getAnAccess() and + v.getType() instanceof TypeString and + ( + a instanceof AssignAddExpr + or + exists(VarAccess use | use.getVariable() = v | use.getParent*() = a.getSource()) and + a.getSource() instanceof AddExpr + ) + } + + /** + * Holds if `e` is executed often in loop `loop`. + */ + predicate executedOften(Assignment a) { + a.getDest().getType() instanceof TypeString and + exists(ControlFlowNode n | a.getControlFlowNode() = n | getADeepSuccessor(n) = n) + } + + /** Gets a sucessor of `n`, also following function calls. */ + ControlFlowNode getADeepSuccessor(ControlFlowNode n) { + result = n.getASuccessor+() + or + exists(Call c, ControlFlowNode callee | c.(Expr).getControlFlowNode() = n.getASuccessor+() | + callee = c.getCallee().getBody().getControlFlowNode() and + result = getADeepSuccessor(callee) + ) + } + + predicate queryResult(Assignment a) { + exists(Variable v | + useAndDef(a, v) and + executedOften(a) + ) + } +} + +// module Config implements DataFlow::ConfigSig { +// predicate isSource(DataFlow::Node n) { n.asExpr().(MethodCall).getMethod().hasName("source") } +// predicate isSink(DataFlow::Node n) { +// exists(MethodCall ma | ma.getMethod().hasName("sink") | n.asExpr() = ma.getAnArgument()) +// } +// } +// module Flow = DataFlow::Global; +module HasFlowTest implements TestSig { + string getARelevantTag() { result = "loopConcat" } + + predicate hasActualResult(Location location, string element, string tag, string value) { + tag = "loopConcat" and + exists(Assignment a | TheQuery::queryResult(a) | + location = a.getLocation() and + element = a.toString() and + value = a.getDest().toString() + ) + } +} + +import MakeTest