From 9918f1b40c7ab8dcd98b8a5c3e6168889ad18b2c Mon Sep 17 00:00:00 2001 From: Marcono1234 Date: Sun, 25 Apr 2021 04:02:43 +0200 Subject: [PATCH] Java: Improve NullGuards.clearlyNotNullExpr() --- .../semmle/code/java/dataflow/NullGuards.qll | 30 +++++-- .../null-guards/ClearlyNotNullExpr.expected | 25 ++++++ .../null-guards/ClearlyNotNullExpr.ql | 9 ++ .../test/library-tests/null-guards/Test.java | 88 +++++++++++++++++++ .../ql/test/library-tests/null-guards/options | 1 + 5 files changed, 148 insertions(+), 5 deletions(-) create mode 100644 java/ql/test/library-tests/null-guards/ClearlyNotNullExpr.expected create mode 100644 java/ql/test/library-tests/null-guards/ClearlyNotNullExpr.ql create mode 100644 java/ql/test/library-tests/null-guards/Test.java create mode 100644 java/ql/test/library-tests/null-guards/options diff --git a/java/ql/src/semmle/code/java/dataflow/NullGuards.qll b/java/ql/src/semmle/code/java/dataflow/NullGuards.qll index 589b1f7c49f5..adfe41ffe4fe 100644 --- a/java/ql/src/semmle/code/java/dataflow/NullGuards.qll +++ b/java/ql/src/semmle/code/java/dataflow/NullGuards.qll @@ -44,21 +44,32 @@ Expr clearlyNotNullExpr(Expr reason) { or result instanceof ArrayCreationExpr and reason = result or + result instanceof FunctionalExpr and reason = result + or result instanceof TypeLiteral and reason = result or result instanceof ThisAccess and reason = result or result instanceof StringLiteral and reason = result or + // Add and AssignAdd performing String concatenation never have null result result instanceof AddExpr and result.getType() instanceof TypeString and reason = result or + result instanceof AssignAddExpr and result.getType() instanceof TypeString and reason = result + or exists(Field f | result = f.getAnAccess() and - (f.hasName("TRUE") or f.hasName("FALSE")) and + f.hasName(["TRUE", "FALSE"]) and f.getDeclaringType().hasQualifiedName("java.lang", "Boolean") and reason = result ) or + result.getType() instanceof PrimitiveType and not result instanceof TypeAccess and reason = result + or + // Ignore RValue because that is covered by SsaVariable check below and correctly + // reports the assigned value as 'reason' + result instanceof CompileTimeConstantExpr and not result instanceof RValue and reason = result + or result.(CastExpr).getExpr() = clearlyNotNullExpr(reason) or result.(AssignExpr).getSource() = clearlyNotNullExpr(reason) @@ -70,6 +81,12 @@ Expr clearlyNotNullExpr(Expr reason) { (reason = r1 or reason = r2) ) or + exists(SwitchExpr s | + s = result and + s.getAResult() = clearlyNotNullExpr(reason) and + forall(Expr resultExpr | resultExpr = s.getAResult() | resultExpr = clearlyNotNullExpr()) + ) + or exists(SsaVariable v, boolean branch, RValue rval, Guard guard | guard = directNullGuard(v, branch, false) and guard.controls(rval.getBasicBlock(), branch) and @@ -104,10 +121,13 @@ predicate clearlyNotNull(SsaVariable v, Expr reason) { clearlyNotNull(captured, reason) ) or - exists(Field f | - v.getSourceVariable().getVariable() = f and - f.isFinal() and - f.getInitializer() = clearlyNotNullExpr(reason) + exists(Variable var | + v.getSourceVariable().getVariable() = var and + var.isFinal() and + var.getAnAssignedValue() = clearlyNotNullExpr(reason) and + forall(Expr assignedValue | assignedValue = var.getAnAssignedValue() | + assignedValue = clearlyNotNullExpr() + ) ) } diff --git a/java/ql/test/library-tests/null-guards/ClearlyNotNullExpr.expected b/java/ql/test/library-tests/null-guards/ClearlyNotNullExpr.expected new file mode 100644 index 000000000000..eead22094c37 --- /dev/null +++ b/java/ql/test/library-tests/null-guards/ClearlyNotNullExpr.expected @@ -0,0 +1,25 @@ +| Test.java:12:13:12:24 | new Object(...) | Test.java:12:13:12:24 | new Object(...) | +| Test.java:13:21:13:21 | 0 | Test.java:13:21:13:21 | 0 | +| Test.java:14:13:14:16 | this | Test.java:14:13:14:16 | this | +| Test.java:15:13:15:14 | "" | Test.java:15:13:15:14 | "" | +| Test.java:17:13:17:24 | Boolean.TRUE | Test.java:17:13:17:24 | Boolean.TRUE | +| Test.java:18:13:18:25 | Boolean.FALSE | Test.java:18:13:18:25 | Boolean.FALSE | +| Test.java:20:13:20:13 | 1 | Test.java:20:13:20:13 | 1 | +| Test.java:21:13:21:21 | Float.NaN | Test.java:21:13:21:21 | Float.NaN | +| Test.java:22:13:22:20 | constant | Test.java:3:29:3:30 | "" | +| Test.java:23:13:23:23 | (...)... | Test.java:23:22:23:23 | "" | +| Test.java:24:13:24:21 | ...=... | Test.java:24:20:24:21 | "" | +| Test.java:26:13:26:54 | ...?...:... | Test.java:26:38:26:39 | "" | +| Test.java:26:13:26:54 | ...?...:... | Test.java:26:43:26:54 | new Object(...) | +| Test.java:27:13:27:21 | switch (...) | Test.java:3:29:3:30 | "" | +| Test.java:27:13:27:21 | switch (...) | Test.java:28:27:28:28 | "" | +| Test.java:34:13:34:25 | ...::... | Test.java:34:13:34:25 | ...::... | +| Test.java:35:13:35:27 | ...->... | Test.java:35:13:35:27 | ...->... | +| Test.java:41:13:41:18 | ... + ... | Test.java:41:13:41:18 | ... + ... | +| Test.java:42:13:42:20 | ... + ... | Test.java:42:13:42:20 | ... + ... | +| Test.java:43:13:43:19 | ...+=... | Test.java:43:13:43:19 | ...+=... | +| Test.java:44:13:44:32 | ... + ... | Test.java:44:13:44:32 | ... + ... | +| Test.java:51:17:51:18 | n1 | Test.java:49:13:49:22 | ... != ... | +| Test.java:59:13:59:14 | n2 | Test.java:57:14:57:15 | "" | +| Test.java:70:13:70:14 | n3 | Test.java:65:18:65:19 | "" | +| Test.java:70:13:70:14 | n3 | Test.java:67:18:67:18 | 1 | diff --git a/java/ql/test/library-tests/null-guards/ClearlyNotNullExpr.ql b/java/ql/test/library-tests/null-guards/ClearlyNotNullExpr.ql new file mode 100644 index 000000000000..ebe91a212744 --- /dev/null +++ b/java/ql/test/library-tests/null-guards/ClearlyNotNullExpr.ql @@ -0,0 +1,9 @@ +import java +import semmle.code.java.dataflow.NullGuards + +from Expr notNull, Expr reason +where + notNull = clearlyNotNullExpr(reason) and + // Restrict to ArrayInit to make results easier to read + notNull.getParent() instanceof ArrayInit +select notNull, reason diff --git a/java/ql/test/library-tests/null-guards/Test.java b/java/ql/test/library-tests/null-guards/Test.java new file mode 100644 index 000000000000..7ef8a95b5e75 --- /dev/null +++ b/java/ql/test/library-tests/null-guards/Test.java @@ -0,0 +1,88 @@ +class NonNullTest { + final String constantNull = null; + final String constant = ""; + + Object getMaybeNull() { + return null; + } + + void notNull() { + Object temp; + Object[] r = { + new Object(), + new int[0], + this, + "", + + Boolean.TRUE, + Boolean.FALSE, + + 1, + Float.NaN, + constant, + (Object) "", + temp = "", + + getMaybeNull() == null ? "" : new Object(), + switch(1) { + case 1 -> ""; + default -> constant; + } + }; + + Runnable[] functional = { + this::notNull, + () -> notNull() + }; + + String s = null; + String s2 = null; + r = new Object[] { + s + s2, + null + s, + s += s2, + (String) null + null + }; + + Object n1 = getMaybeNull(); + // Guarded by null check + if (n1 != null) { + r = new Object[] { + n1 + }; + } + + Object n2 = getMaybeNull(); + // Assigned a non-null value + n2 = ""; + r = new Object[] { + n2 + }; + + // final variable for which all assigned values are non-null + final Object n3; + if (getMaybeNull() == null) { + n3 = ""; + } else { + n3 = 1; + } + r = new Object[] { + n3 + }; + } + + void maybeNull(boolean b, int i) { + Object temp; + Object[] r = { + constantNull, + (Object) null, + temp = (String) null, + b ? "" : null, + switch(i) { + case 1 -> ""; + default -> null; + }, + null + }; + } +} diff --git a/java/ql/test/library-tests/null-guards/options b/java/ql/test/library-tests/null-guards/options new file mode 100644 index 000000000000..bb7dba26161c --- /dev/null +++ b/java/ql/test/library-tests/null-guards/options @@ -0,0 +1 @@ +//semmle-extractor-options: --javac-args -source 15 -target 15