Merge pull request #8571 from Marcono1234/marcono1234/statement-expression

Java: Add `ValueDiscardingExpr`

Author: aschackmull

java/ql/lib/change-notes/2022-05-09-value-discarding-expression.md

@@ -0,0 +1,4 @@
+---
+category: feature
+---
+* The QL class `ValueDiscardingExpr` has been added, representing expressions for which the value of the expression as a whole is discarded.

java/ql/lib/semmle/code/java/Collections.qll

@@ -84,7 +84,7 @@ class CollectionMutation extends MethodAccess {
   CollectionMutation() { this.getMethod() instanceof CollectionMutator }
 
   /** Holds if the result of this call is not immediately discarded. */
-  predicate resultIsChecked() { not this.getParent() instanceof ExprStmt }
+  predicate resultIsChecked() { not this instanceof ValueDiscardingExpr }
 }
 
 /** A method that queries the contents of a collection without mutating it. */

java/ql/lib/semmle/code/java/Expr.qll

@@ -2133,3 +2133,47 @@ class Argument extends Expr {
     )
   }
 }
+
+/**
+ * An expression for which the value of the expression as a whole is discarded. Only cases
+ * of discarded values at the language level (as described by the JLS) are considered;
+ * data flow, for example to determine if an assigned variable value is ever read, is not
+ * considered. Such expressions can for example appear as part of an `ExprStmt` or as
+ * initializer of a `for` loop.
+ *
+ * For example, for the statement `i++;` the value of the increment expression, that is the
+ * old value of variable `i`, is discarded. Whereas for the statement `println(i++);` the
+ * value of the increment expression is not discarded but used as argument for the method call.
+ */
+class ValueDiscardingExpr extends Expr {
+  ValueDiscardingExpr() {
+    (
+      this = any(ExprStmt s).getExpr()
+      or
+      this = any(ForStmt s).getAnInit() and not this instanceof LocalVariableDeclExpr
+      or
+      this = any(ForStmt s).getAnUpdate()
+      or
+      // Only applies to SwitchStmt, but not to SwitchExpr, see JLS 17 section 14.11.2
+      this = any(SwitchStmt s).getACase().getRuleExpression()
+      or
+      // TODO: Workarounds for https://github.com/github/codeql/issues/3605
+      exists(LambdaExpr lambda |
+        this = lambda.getExprBody() and
+        lambda.asMethod().getReturnType() instanceof VoidType
+      )
+      or
+      exists(MemberRefExpr memberRef, Method implicitMethod, Method overridden |
+        implicitMethod = memberRef.asMethod()
+      |
+        this.getParent().(ReturnStmt).getEnclosingCallable() = implicitMethod and
+        // asMethod() has bogus method with wrong return type as result, e.g. `run(): String` (overriding `Runnable.run(): void`)
+        // Therefore need to check the overridden method
+        implicitMethod.getSourceDeclaration().overridesOrInstantiates*(overridden) and
+        overridden.getReturnType() instanceof VoidType
+      )
+    ) and
+    // Ignore if this expression is a method call with `void` as return type
+    not this.getType() instanceof VoidType
+  }
+}

java/ql/lib/semmle/code/java/Maps.qll

@@ -53,7 +53,7 @@ class MapMutation extends MethodAccess {
   MapMutation() { this.getMethod() instanceof MapMutator }
 
   /** Holds if the result of this call is not immediately discarded. */
-  predicate resultIsChecked() { not this.getParent() instanceof ExprStmt }
+  predicate resultIsChecked() { not this instanceof ValueDiscardingExpr }
 }
 
 /** A method that queries the contents of the map it belongs to without mutating it. */

java/ql/src/Likely Bugs/Statements/ReturnValueIgnored.ql

@@ -18,7 +18,7 @@ import Chaining
 
 predicate checkedMethodCall(MethodAccess ma) {
   relevantMethodCall(ma, _) and
-  not ma.getParent() instanceof ExprStmt
+  not ma instanceof ValueDiscardingExpr
 }
 
 /**

java/ql/src/Violations of Best Practice/Exception Handling/IgnoreExceptionalReturn.ql

@@ -45,7 +45,7 @@ predicate unboundedQueue(RefType t) {
 
 from MethodAccess ma, SpecialMethod m
 where
-  ma.getParent() instanceof ExprStmt and
+  ma instanceof ValueDiscardingExpr and
   m = ma.getMethod() and
   (
     m.isMethod("java.util", "Queue", "offer", 1) and not unboundedQueue(m.getDeclaringType())

java/ql/src/experimental/Security/CWE/CWE-297/IgnoredHostnameVerification.ql

@@ -21,7 +21,7 @@ private class HostnameVerificationCall extends MethodAccess {
   }
 
   /** Holds if the result of the call is not used. */
-  predicate isIgnored() { this = any(ExprStmt es).getExpr() }
+  predicate isIgnored() { this instanceof ValueDiscardingExpr }
 }
 
 from HostnameVerificationCall verification

java/ql/test/library-tests/ValueDiscardingExpr/ValueDiscardingExpr.expected

@@ -0,0 +1,16 @@
+| ValueDiscardingExpr.java:9:9:9:18 | toString(...) |
+| ValueDiscardingExpr.java:18:9:18:13 | ...=... |
+| ValueDiscardingExpr.java:19:9:19:11 | ...++ |
+| ValueDiscardingExpr.java:20:9:20:11 | ++... |
+| ValueDiscardingExpr.java:21:9:21:11 | ...-- |
+| ValueDiscardingExpr.java:22:9:22:11 | --... |
+| ValueDiscardingExpr.java:24:9:24:20 | new Object(...) |
+| ValueDiscardingExpr.java:27:9:27:28 | clone(...) |
+| ValueDiscardingExpr.java:30:14:30:38 | append(...) |
+| ValueDiscardingExpr.java:35:17:35:43 | append(...) |
+| ValueDiscardingExpr.java:55:24:55:33 | toString(...) |
+| ValueDiscardingExpr.java:74:29:74:38 | toString(...) |
+| ValueDiscardingExpr.java:76:13:76:22 | toString(...) |
+| ValueDiscardingExpr.java:90:23:90:35 | new StmtExpr(...) |
+| ValueDiscardingExpr.java:91:23:91:36 | toString(...) |
+| ValueDiscardingExpr.java:95:25:95:37 | new String[] |

java/ql/test/library-tests/ValueDiscardingExpr/ValueDiscardingExpr.java

@@ -0,0 +1,100 @@
+package StmtExpr;
+
+import java.util.function.IntConsumer;
+import java.util.function.IntFunction;
+import java.util.function.Supplier;
+
+class StmtExpr {
+    void test() {
+        toString();
+
+        // Method call with `void` return type is not a ValueDiscardingExpr
+        System.out.println("test");
+
+        // LocalVariableDeclarationStatement with init is not a ValueDiscardingExpr
+        String s = toString();
+
+        int i;
+        i = 0;
+        i++;
+        ++i;
+        i--;
+        --i;
+
+        new Object();
+        // Language specification does not permit ArrayCreationExpression at locations where its
+        // value would be discard, but the value of a method access on it can be discarded
+        new int[] {}.clone();
+
+        // for statement init can discard value
+        for (System.out.append("init");;) {
+            break;
+        }
+
+        // for statement update discards value
+        for (;; System.out.append("update")) {
+            break;
+        }
+
+        // Method call with `void` return type is not a ValueDiscardingExpr
+        for (;; System.out.println("update")) {
+            break;
+        }
+
+        // variable declaration and condition are not ValueDiscardingExpr
+        for (int i1 = 0; i1 < 10;) { }
+        for (int i1, i2 = 0; i2 < 10;) { }
+        for (;;) {
+            break;
+        }
+
+        // Not a ValueDiscardingExpr
+        for (int i2 : new int[] {1}) { }
+
+        switch(1) {
+            default -> toString(); // ValueDiscardingExpr
+        }
+
+        switch(1) {
+            // Method call with `void` return type is not a ValueDiscardingExpr
+            default -> System.out.println();
+        }
+
+        String s2 = switch(1) {
+            // Expression in SwitchExpression case rule is not a ValueDiscardingExpr
+            default -> toString();
+        };
+
+        // Expression in lambda with non-void return type is not a ValueDiscardingExpr
+        Supplier<Object> supplier1 = () -> toString();
+        Supplier<Object> supplier2 = () -> {
+            return toString();
+        };
+        // Expression in lambda with void return type is ValueDiscardingExpr
+        Runnable r1 = () -> toString();
+        Runnable r2 = () -> {
+            toString();
+        };
+        // Lambda with method call with `void` return type is not a ValueDiscardingExpr
+        Runnable r3 = () -> System.out.println();
+        Runnable r4 = () -> {
+            System.out.println();
+        };
+
+        // Method reference with non-void return type has no ValueDiscardingExpr
+        Supplier<Object> supplier3 = StmtExpr::new;
+        IntFunction<Object> f = String[]::new;
+        Supplier<Object> supplier4 = this::toString;
+
+        // Method reference with void return type has ValueDiscardingExpr in implicit method body
+        Runnable r5 = StmtExpr::new;
+        Runnable r6 = this::toString;
+        // Interestingly a method reference expression with function type (int)->void allows usage of
+        // array creation expressions, even though a regular StatementExpression would not allow it,
+        // for example the ExpressionStatement `new String[1];` is not allowed by the JLS
+        IntConsumer c = String[]::new;
+
+        // Method reference referring to method with `void` return type is not a ValueDiscardingExpr
+        Runnable r7 = System.out::println;
+    }
+}

java/ql/test/library-tests/ValueDiscardingExpr/ValueDiscardingExpr.ql

@@ -0,0 +1,4 @@
+import java
+
+from ValueDiscardingExpr e
+select e