From 600b6e5f3aeefcce2841c36824d967f74f6824e8 Mon Sep 17 00:00:00 2001 From: Marcono1234 Date: Sat, 2 Oct 2021 23:39:50 +0200 Subject: [PATCH] Java: Deprecate `PrimitiveType.getADefaultValue()` --- ...2021-10-02-getADefaultValue-deprecation.md | 2 + java/ql/lib/semmle/code/java/Type.qll | 6 +- .../Dead Code/DeadStoreOfLocal.ql | 21 +++- java/ql/test/query-tests/UnreadLocal/A.java | 95 +++++++++++++++++++ .../UnreadLocal/DeadStoreOfLocal.expected | 10 ++ 5 files changed, 128 insertions(+), 6 deletions(-) create mode 100644 java/change-notes/2021-10-02-getADefaultValue-deprecation.md diff --git a/java/change-notes/2021-10-02-getADefaultValue-deprecation.md b/java/change-notes/2021-10-02-getADefaultValue-deprecation.md new file mode 100644 index 000000000000..07e896d4d95a --- /dev/null +++ b/java/change-notes/2021-10-02-getADefaultValue-deprecation.md @@ -0,0 +1,2 @@ +lgtm,codescanning +* The predicate `PrimitiveType.getADefaultValue()` has been deprecated for removal in a future version because its behavior is misleading. diff --git a/java/ql/lib/semmle/code/java/Type.qll b/java/ql/lib/semmle/code/java/Type.qll index 5395e855701d..ccdcf7fc58f7 100755 --- a/java/ql/lib/semmle/code/java/Type.qll +++ b/java/ql/lib/semmle/code/java/Type.qll @@ -947,6 +947,10 @@ class PrimitiveType extends Type, @primitive { } /** + * DEPRECATED: This predicate will be removed in a future version because + * its behavior is misleading and it does not find all literals with default + * value, see [GitHub issue #6615](https://github.com/github/codeql/issues/6615). + * * Gets a default value for this primitive type, as assigned by the compiler * for variables that are declared but not initialized explicitly. * Typically zero for numeric and character types and `false` for `boolean`. @@ -955,7 +959,7 @@ class PrimitiveType extends Type, @primitive { * considered to be default values of all other numeric types, even if they * require an explicit cast. */ - Literal getADefaultValue() { + deprecated Literal getADefaultValue() { getName() = "boolean" and result.getLiteral() = "false" or getName() = "char" and diff --git a/java/ql/src/Violations of Best Practice/Dead Code/DeadStoreOfLocal.ql b/java/ql/src/Violations of Best Practice/Dead Code/DeadStoreOfLocal.ql index c2dd74b57457..bb19301a3d41 100644 --- a/java/ql/src/Violations of Best Practice/Dead Code/DeadStoreOfLocal.ql +++ b/java/ql/src/Violations of Best Practice/Dead Code/DeadStoreOfLocal.ql @@ -27,13 +27,24 @@ predicate flowStep(Expr decl, Expr init) { decl.(CastExpr).getExpr() = init } +predicate isDefaultValueLiteral(Literal l, Type t) { + t instanceof RefType and l instanceof NullLiteral + or + // Checking here for primitive suffices to make sure that literals below are valid default + t instanceof PrimitiveType and + ( + l.(BooleanLiteral).getBooleanValue() = false or + l.(CharacterLiteral).getValue() = 0.toUnicode() or + l.(DoubleLiteral).getDoubleValue() = 0 or + l.(FloatingPointLiteral).getFloatValue() = 0 or + l.(IntegerLiteral).getIntValue() = 0 or + l.(LongLiteral).getValue() = "0" + ) +} + predicate excludedInit(Type t, Expr decl) { exists(Expr init | flowStep(decl, init) | - // The `null` literal for reference types. - t instanceof RefType and init instanceof NullLiteral - or - // The default value for primitive types. - init = t.(PrimitiveType).getADefaultValue() + isDefaultValueLiteral(init, t) or // The expression `-1` for integral types. t instanceof IntegralType and minusOne(init) diff --git a/java/ql/test/query-tests/UnreadLocal/A.java b/java/ql/test/query-tests/UnreadLocal/A.java index 5591df08634c..d03380109859 100644 --- a/java/ql/test/query-tests/UnreadLocal/A.java +++ b/java/ql/test/query-tests/UnreadLocal/A.java @@ -71,6 +71,101 @@ public void ex4() { if (valid) return; } + public void use(Object o) { } + + public void badNonDefault() { + String s1 = ""; + s1 = "a"; + use(s1); + + String s2 = "null"; + s2 = "a"; + use(s2); + + Object o = false; // `false` is not the default for Object + o = true; + use(o); + + boolean b = true; + b = false; + use(b); + + char c1 = '\1'; + c1 = '1'; + use(c1); + + char c2 = '0'; // is not \0 + c2 = '1'; + use(c2); + + double d = 1d; + d = 0d; + use(d); + + float f = 1f; + f = 0f; + use(f); + + int i = 1; + i = 0; + use(i); + + long l = 1L; + l = 0L; + use(l); + } + + // Ignore if stored value is default value + public void goodDefault() { + String s = null; + s = "a"; + use(s); + + boolean b = false; + b = true; + use(b); + + char c1 = '\0'; + c1 = '1'; + use(c1); + + char c2 = 0; + c2 = 1; + use(c2); + + double d1 = 0d; + d1 = 1d; + use(d1); + + double d2 = '\0'; + d2 = 1; + use(d2); + + float f1 = 0f; + f1 = 1f; + use(f1); + + float f2 = 0; + f2 = 1; + use(f2); + + int i1 = 0; + i1 = 1; + use(i1); + + int i2 = '\0'; + i2 = '1'; + use(i2); + + long l1 = 0L; + l1 = 1L; + use(l1); + + long l2 = 0; + l2 = 1; + use(l2); + } + // ensure extraction of java.lang.RuntimeException public void noop() throws RuntimeException { } } diff --git a/java/ql/test/query-tests/UnreadLocal/DeadStoreOfLocal.expected b/java/ql/test/query-tests/UnreadLocal/DeadStoreOfLocal.expected index 40d26f3084c3..2e0c5d7b6195 100644 --- a/java/ql/test/query-tests/UnreadLocal/DeadStoreOfLocal.expected +++ b/java/ql/test/query-tests/UnreadLocal/DeadStoreOfLocal.expected @@ -3,3 +3,13 @@ | A.java:40:14:40:16 | ++... | This assignment to y is useless: the value is always overwritten before it is read. | | A.java:55:13:55:17 | ...=... | This assignment to x is useless: the value is always overwritten before it is read. | | A.java:64:17:64:29 | ...=... | This assignment to valid is useless: the value is always overwritten before it is read. | +| A.java:77:16:77:22 | s1 | This assignment to s1 is useless: the value is always overwritten before it is read. | +| A.java:81:16:81:26 | s2 | This assignment to s2 is useless: the value is always overwritten before it is read. | +| A.java:85:16:85:24 | o | This assignment to o is useless: the value is always overwritten before it is read. | +| A.java:89:17:89:24 | b | This assignment to b is useless: the value is always overwritten before it is read. | +| A.java:93:14:93:22 | c1 | This assignment to c1 is useless: the value is always overwritten before it is read. | +| A.java:97:14:97:21 | c2 | This assignment to c2 is useless: the value is always overwritten before it is read. | +| A.java:101:16:101:21 | d | This assignment to d is useless: the value is always overwritten before it is read. | +| A.java:105:15:105:20 | f | This assignment to f is useless: the value is always overwritten before it is read. | +| A.java:109:13:109:17 | i | This assignment to i is useless: the value is always overwritten before it is read. | +| A.java:113:14:113:19 | l | This assignment to l is useless: the value is always overwritten before it is read. |