From 17627e4a070ce7dc2ce97c8cf31512c722e7bd1c Mon Sep 17 00:00:00 2001 From: Rasmus Lerchedahl Petersen Date: Mon, 29 Jan 2024 21:04:42 +0100 Subject: [PATCH 1/3] java: start reducing noise - create test - check for statements being executed often --- .../src/Performance/ConcatenationInLoops.ql | 17 ++++++++-- .../ConcatenationInLoops.expected | 1 + .../ConcatenationInLoops.java | 34 +++++++++++++++++++ .../ConcatenationInLoops.qlref | 1 + 4 files changed, 50 insertions(+), 3 deletions(-) create mode 100644 java/ql/test/query-tests/ConcatenationInLoops/ConcatenationInLoops.expected create mode 100644 java/ql/test/query-tests/ConcatenationInLoops/ConcatenationInLoops.java create mode 100644 java/ql/test/query-tests/ConcatenationInLoops/ConcatenationInLoops.qlref diff --git a/java/ql/src/Performance/ConcatenationInLoops.ql b/java/ql/src/Performance/ConcatenationInLoops.ql index e4280678e4be..9b5b846f15ed 100644 --- a/java/ql/src/Performance/ConcatenationInLoops.ql +++ b/java/ql/src/Performance/ConcatenationInLoops.ql @@ -49,10 +49,21 @@ predicate declaredInLoop(LocalVariableDecl v, LoopStmt loop) { exists(EnhancedForStmt for | for = loop | for.getVariable().getVariable() = v) } +/** + * Holds if `e` is executed often in loop `loop`. + * + * Checks that `e` is not in a block that breaks out of the loop. + * + * A more principled way would be to check for loops in the control flow graph. + */ +predicate executedOften(Assignment e, LoopStmt loop) { + e.getEnclosingStmt().getEnclosingStmt*() = loop and + not loop.(ForStmt).getInit(_) = e.getParent*() and + not exists(BreakStmt b | b.getEnclosingStmt() = e.getEnclosingStmt().getEnclosingStmt()) +} + from Assignment a, Variable v where useAndDef(a, v) and - exists(LoopStmt loop | a.getEnclosingStmt().getEnclosingStmt*() = loop | - not declaredInLoop(v, loop) - ) + exists(LoopStmt loop | executedOften(a, loop) | not declaredInLoop(v, loop)) 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..2e7422a76b96 --- /dev/null +++ b/java/ql/test/query-tests/ConcatenationInLoops/ConcatenationInLoops.expected @@ -0,0 +1 @@ +| ConcatenationInLoops.java:7:4:7:10 | ...+=... | The string s 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..eb4b0981013c --- /dev/null +++ b/java/ql/test/query-tests/ConcatenationInLoops/ConcatenationInLoops.java @@ -0,0 +1,34 @@ +public class ConcatenationInLoops { + public static void main(String[] args) { + // Random r = 42; + long start = System.currentTimeMillis(); + String s = ""; + for (int i = 0; i < 65536; i++) + s += 42;//r.nextInt(2); + 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; // Will be executed many times. + break; + } + } +} \ 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 From 835dba4e8a22efb02dc7682b24377ffec65075dc Mon Sep 17 00:00:00 2001 From: Rasmus Lerchedahl Petersen Date: Mon, 19 Feb 2024 09:03:35 +0100 Subject: [PATCH 2/3] java: add cfg-based loop detection - add test for recursive function - update test expectations --- java/ql/src/Performance/ConcatenationInLoops.ql | 16 +++++++++++++++- .../ConcatenationInLoops.expected | 3 ++- .../ConcatenationInLoops.java | 9 +++++++++ 3 files changed, 26 insertions(+), 2 deletions(-) diff --git a/java/ql/src/Performance/ConcatenationInLoops.ql b/java/ql/src/Performance/ConcatenationInLoops.ql index 9b5b846f15ed..847a4b0b6b31 100644 --- a/java/ql/src/Performance/ConcatenationInLoops.ql +++ b/java/ql/src/Performance/ConcatenationInLoops.ql @@ -62,8 +62,22 @@ predicate executedOften(Assignment e, LoopStmt loop) { not exists(BreakStmt b | b.getEnclosingStmt() = e.getEnclosingStmt().getEnclosingStmt()) } +predicate executedOftenCFG(Assignment a) { + a.getDest().getType() instanceof TypeString and + exists(ControlFlowNode n | a.getControlFlowNode() = n | n.getASuccessor+() = n) +} + +ControlFlowNode test(ControlFlowNode n) { + exists(Assignment a | a.getControlFlowNode() = n | a.getDest().getType() instanceof TypeString) and + result = n.getASuccessor+() +} + from Assignment a, Variable v where useAndDef(a, v) and - exists(LoopStmt loop | executedOften(a, loop) | not declaredInLoop(v, loop)) + ( + exists(LoopStmt loop | executedOften(a, loop) | not declaredInLoop(v, loop)) + or + executedOftenCFG(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 index 2e7422a76b96..66cf85fe921e 100644 --- a/java/ql/test/query-tests/ConcatenationInLoops/ConcatenationInLoops.expected +++ b/java/ql/test/query-tests/ConcatenationInLoops/ConcatenationInLoops.expected @@ -1 +1,2 @@ -| ConcatenationInLoops.java:7:4:7:10 | ...+=... | The string s is built-up in a loop: use string buffer. | +| 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. | diff --git a/java/ql/test/query-tests/ConcatenationInLoops/ConcatenationInLoops.java b/java/ql/test/query-tests/ConcatenationInLoops/ConcatenationInLoops.java index eb4b0981013c..b4bf630cbdb3 100644 --- a/java/ql/test/query-tests/ConcatenationInLoops/ConcatenationInLoops.java +++ b/java/ql/test/query-tests/ConcatenationInLoops/ConcatenationInLoops.java @@ -1,4 +1,6 @@ public class ConcatenationInLoops { + private String cs = ""; + public static void main(String[] args) { // Random r = 42; long start = System.currentTimeMillis(); @@ -31,4 +33,11 @@ public static void main(String[] args) { break; } } + + public void test(int bound) { + if (bound > 0) { + cs += "a"; + test(bound - 1); + } + } } \ No newline at end of file From 6ab53d95dcad525d0c23f7c99c6c8e36ef4dd775 Mon Sep 17 00:00:00 2001 From: Rasmus Lerchedahl Petersen Date: Mon, 26 Feb 2024 11:02:48 +0100 Subject: [PATCH 3/3] java: Use semantic reasoning - Use a deep callgraph traversal to identify loops. This should be checked for performance issues and possibly be restriceted/optimised. - Add inline expectation test. Currently dupicating the query code. It should be refactored so that the code can be reused. --- .../src/Performance/ConcatenationInLoops.ql | 38 +++------ .../ConcatenationInLoops.expected | 1 + .../ConcatenationInLoops.java | 6 +- .../ConcatenationInLoops/test.expected | 2 + .../query-tests/ConcatenationInLoops/test.ql | 82 +++++++++++++++++++ 5 files changed, 98 insertions(+), 31 deletions(-) create mode 100644 java/ql/test/query-tests/ConcatenationInLoops/test.expected create mode 100644 java/ql/test/query-tests/ConcatenationInLoops/test.ql diff --git a/java/ql/src/Performance/ConcatenationInLoops.ql b/java/ql/src/Performance/ConcatenationInLoops.ql index 847a4b0b6b31..fd0ce673a1df 100644 --- a/java/ql/src/Performance/ConcatenationInLoops.ql +++ b/java/ql/src/Performance/ConcatenationInLoops.ql @@ -40,44 +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() - ) - or - exists(EnhancedForStmt for | for = loop | for.getVariable().getVariable() = v) -} - /** * Holds if `e` is executed often in loop `loop`. - * - * Checks that `e` is not in a block that breaks out of the loop. - * - * A more principled way would be to check for loops in the control flow graph. */ -predicate executedOften(Assignment e, LoopStmt loop) { - e.getEnclosingStmt().getEnclosingStmt*() = loop and - not loop.(ForStmt).getInit(_) = e.getParent*() and - not exists(BreakStmt b | b.getEnclosingStmt() = e.getEnclosingStmt().getEnclosingStmt()) -} - -predicate executedOftenCFG(Assignment a) { +predicate executedOften(Assignment a) { a.getDest().getType() instanceof TypeString and - exists(ControlFlowNode n | a.getControlFlowNode() = n | n.getASuccessor+() = n) + exists(ControlFlowNode n | a.getControlFlowNode() = n | getADeepSuccessor(n) = n) } -ControlFlowNode test(ControlFlowNode n) { - exists(Assignment a | a.getControlFlowNode() = n | a.getDest().getType() instanceof TypeString) and +/** 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) + ) } from Assignment a, Variable v where useAndDef(a, v) and - ( - exists(LoopStmt loop | executedOften(a, loop) | not declaredInLoop(v, loop)) - or - executedOftenCFG(a) - ) + 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 index 66cf85fe921e..12be89018cd7 100644 --- a/java/ql/test/query-tests/ConcatenationInLoops/ConcatenationInLoops.expected +++ b/java/ql/test/query-tests/ConcatenationInLoops/ConcatenationInLoops.expected @@ -1,2 +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 index b4bf630cbdb3..22e46398ed05 100644 --- a/java/ql/test/query-tests/ConcatenationInLoops/ConcatenationInLoops.java +++ b/java/ql/test/query-tests/ConcatenationInLoops/ConcatenationInLoops.java @@ -6,7 +6,7 @@ public static void main(String[] args) { long start = System.currentTimeMillis(); String s = ""; for (int i = 0; i < 65536; i++) - s += 42;//r.nextInt(2); + s += 42; // $ loopConcat=s System.out.println(System.currentTimeMillis() - start); // This prints roughly 4500. // r = 42; @@ -29,14 +29,14 @@ public static void main(String[] args) { for (int i = 0; i < 65536; i++) for (int j = 0; i < 65536; i++) if (j > 10) { - s += 42; // Will be executed many times. + s += 42; // $ loopConcat=s break; } } public void test(int bound) { if (bound > 0) { - cs += "a"; + cs += "a"; // $ loopConcat=cs test(bound - 1); } } 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