From 5def44b00a3eb73a26bf97d1dd09932e7bca79e2 Mon Sep 17 00:00:00 2001 From: Marcono1234 Date: Sun, 14 Mar 2021 00:38:54 +0100 Subject: [PATCH 1/2] Java: Fix WildcardTypeAccess.hasNoBound() not considering ArrayTypeAccess --- java/ql/src/semmle/code/java/Expr.qll | 20 ++++- .../dependency/PrintAst.expected | 2 +- .../library-tests/generics/PrintAst.expected | 8 +- .../locations/TypeLocations.expected | 4 + .../locations/WildcardLocations.expected | 3 - .../locations/WildcardLocations.ql | 30 ++++++- .../library-tests/locations/locations/C.java | 8 +- .../reflection/PrintAst.expected | 4 +- .../typeaccesses/ArrayTypeAccesses.expected | 4 + .../typeaccesses/PrintAst.expected | 88 ++++++++++++++++++- .../typeaccesses/TypeAccesses.expected | 37 +++++++- .../typeaccesses/typeaccesses/TA.java | 15 ++++ 12 files changed, 204 insertions(+), 19 deletions(-) diff --git a/java/ql/src/semmle/code/java/Expr.qll b/java/ql/src/semmle/code/java/Expr.qll index 43ad6634fc15..b2b297ed9379 100755 --- a/java/ql/src/semmle/code/java/Expr.qll +++ b/java/ql/src/semmle/code/java/Expr.qll @@ -1742,11 +1742,27 @@ class WildcardTypeAccess extends Expr, @wildcardtypeaccess { /** Gets the lower bound of this wildcard type access, if any. */ Expr getLowerBound() { result.isNthChildOf(this, 1) } + /** + * Gets the bound of this wildcard type access, if any, i.e. either the upper + * or the lower bound. + */ + Expr getBound() { + result = getUpperBound() or + result = getLowerBound() + } + /** Holds if this wildcard is not bounded by any type bounds. */ - predicate hasNoBound() { not exists(TypeAccess t | t.getParent() = this) } + predicate hasNoBound() { not exists(getBound()) } /** Gets a printable representation of this expression. */ - override string toString() { result = "? ..." } + override string toString() { + if exists(getUpperBound()) + then result = "? extends ..." + else + if exists(getLowerBound()) + then result = "? super ..." + else result = "?" + } override string getAPrimaryQlClass() { result = "WildcardTypeAccess" } } diff --git a/java/ql/test/library-tests/dependency/PrintAst.expected b/java/ql/test/library-tests/dependency/PrintAst.expected index e9eeb6f75b40..73721f0e95fc 100644 --- a/java/ql/test/library-tests/dependency/PrintAst.expected +++ b/java/ql/test/library-tests/dependency/PrintAst.expected @@ -46,6 +46,6 @@ dependency/A.java: #-----| 4: (Parameters) # 28| 0: [Parameter] t # 28| 0: [TypeAccess] Collection -# 28| 0: [WildcardTypeAccess] ? ... +# 28| 0: [WildcardTypeAccess] ? extends ... # 28| 0: [TypeAccess] Number # 28| 5: [BlockStmt] stmt diff --git a/java/ql/test/library-tests/generics/PrintAst.expected b/java/ql/test/library-tests/generics/PrintAst.expected index 7c4932a6f50f..e62883cf8faa 100644 --- a/java/ql/test/library-tests/generics/PrintAst.expected +++ b/java/ql/test/library-tests/generics/PrintAst.expected @@ -29,18 +29,18 @@ generics/A.java: # 16| 0: [TypeAccess] Number # 17| 3: [FieldDeclaration] D d1, ...; # 17| -1: [TypeAccess] D -# 17| 0: [WildcardTypeAccess] ? ... +# 17| 0: [WildcardTypeAccess] ? # 18| 4: [FieldDeclaration] D d2, ...; # 18| -1: [TypeAccess] D -# 18| 0: [WildcardTypeAccess] ? ... +# 18| 0: [WildcardTypeAccess] ? extends ... # 18| 0: [TypeAccess] Object # 19| 5: [FieldDeclaration] D d3, ...; # 19| -1: [TypeAccess] D -# 19| 0: [WildcardTypeAccess] ? ... +# 19| 0: [WildcardTypeAccess] ? extends ... # 19| 0: [TypeAccess] Float # 20| 6: [FieldDeclaration] D d4, ...; # 20| -1: [TypeAccess] D -# 20| 0: [WildcardTypeAccess] ? ... +# 20| 0: [WildcardTypeAccess] ? super ... # 20| 1: [TypeAccess] Double # 21| 7: [BlockStmt] stmt # 21| 0: [ExprStmt] stmt diff --git a/java/ql/test/library-tests/locations/TypeLocations.expected b/java/ql/test/library-tests/locations/TypeLocations.expected index 516699a73d49..3f5a9b09db07 100644 --- a/java/ql/test/library-tests/locations/TypeLocations.expected +++ b/java/ql/test/library-tests/locations/TypeLocations.expected @@ -1,8 +1,12 @@ | file://:0:0:0:0 | | | file://:0:0:0:0 | ? | | file://:0:0:0:0 | ? extends Number | +| file://:0:0:0:0 | ? extends Number[] | | file://:0:0:0:0 | ? super Double | +| file://:0:0:0:0 | ? super Double[] | +| file://:0:0:0:0 | Double[] | | file://:0:0:0:0 | G[] | +| file://:0:0:0:0 | Number[] | | file://:0:0:0:0 | Object[] | | file://:0:0:0:0 | Object[][] | | file://:0:0:0:0 | boolean | diff --git a/java/ql/test/library-tests/locations/WildcardLocations.expected b/java/ql/test/library-tests/locations/WildcardLocations.expected index cecd45b03ff6..e69de29bb2d1 100644 --- a/java/ql/test/library-tests/locations/WildcardLocations.expected +++ b/java/ql/test/library-tests/locations/WildcardLocations.expected @@ -1,3 +0,0 @@ -| locations/C.java:6:7:6:7 | ? ... | -| locations/C.java:7:7:7:22 | ? ... | -| locations/C.java:8:7:8:20 | ? ... | diff --git a/java/ql/test/library-tests/locations/WildcardLocations.ql b/java/ql/test/library-tests/locations/WildcardLocations.ql index 0a5112e60c7a..a1b5d0a07bd1 100644 --- a/java/ql/test/library-tests/locations/WildcardLocations.ql +++ b/java/ql/test/library-tests/locations/WildcardLocations.ql @@ -1,4 +1,30 @@ import default +import TestUtilities.InlineExpectationsTest -from WildcardTypeAccess wta -select wta +private string getValue(WildcardTypeAccess wta) { + if wta.hasNoBound() // also makes sure that hasNoBound() is working correctly + then result = "" + else + // Also makes sure that getBound() is working correctly (and has at most + // one result) + exists(Expr bound | bound = wta.getBound() and not wta.getBound() != bound | + bound = wta.getUpperBound() and result = "u:" + bound + or + bound = wta.getLowerBound() and result = "l:" + bound + ) +} + +class WildcardTypeAccessTest extends InlineExpectationsTest { + WildcardTypeAccessTest() { this = "WildcardTypeAccessTest" } + + override string getARelevantTag() { result = ["wildcardTypeAccess"] } + + override predicate hasActualResult(Location location, string element, string tag, string value) { + exists(WildcardTypeAccess wta | + location = wta.getLocation() and + element = wta.toString() and + tag = "wildcardTypeAccess" and + value = getValue(wta) + ) + } +} diff --git a/java/ql/test/library-tests/locations/locations/C.java b/java/ql/test/library-tests/locations/locations/C.java index 3ef78d32e526..51ea521ef54e 100644 --- a/java/ql/test/library-tests/locations/locations/C.java +++ b/java/ql/test/library-tests/locations/locations/C.java @@ -3,7 +3,9 @@ import java.util.List; public class C { - List stuff; - List numbers; - List more_numbers; + List stuff; // $wildcardTypeAccess= + List numbers; // $wildcardTypeAccess=u:Number + List more_numbers; // $wildcardTypeAccess=l:Double + List numbersArr; // $wildcardTypeAccess=u:...[] + List more_numbersArr; // $wildcardTypeAccess=l:...[] } diff --git a/java/ql/test/library-tests/reflection/PrintAst.expected b/java/ql/test/library-tests/reflection/PrintAst.expected index ede39cfcde5e..069106af421a 100644 --- a/java/ql/test/library-tests/reflection/PrintAst.expected +++ b/java/ql/test/library-tests/reflection/PrintAst.expected @@ -15,7 +15,7 @@ reflection/ReflectiveAccess.java: #-----| 4: (Parameters) # 13| 0: [Parameter] classContainingAnnotation # 13| 0: [TypeAccess] Class -# 13| 0: [WildcardTypeAccess] ? ... +# 13| 0: [WildcardTypeAccess] ? # 13| 1: [Parameter] annotationClass # 13| 0: [TypeAccess] Class # 13| 0: [TypeAccess] A @@ -33,7 +33,7 @@ reflection/ReflectiveAccess.java: # 17| 5: [BlockStmt] stmt # 18| 0: [LocalVariableDeclStmt] stmt # 18| 0: [TypeAccess] Class -# 18| 0: [WildcardTypeAccess] ? ... +# 18| 0: [WildcardTypeAccess] ? # 18| 1: [LocalVariableDeclExpr] testClass # 18| 0: [MethodAccess] forName(...) # 18| -1: [TypeAccess] Class<> diff --git a/java/ql/test/library-tests/typeaccesses/ArrayTypeAccesses.expected b/java/ql/test/library-tests/typeaccesses/ArrayTypeAccesses.expected index 0fec21fe54ff..a8a6e95ebebe 100644 --- a/java/ql/test/library-tests/typeaccesses/ArrayTypeAccesses.expected +++ b/java/ql/test/library-tests/typeaccesses/ArrayTypeAccesses.expected @@ -1,2 +1,6 @@ | typeaccesses/Arrays.java:4:2:4:9 | ...[] | | typeaccesses/Arrays.java:5:2:5:25 | ...[] | +| typeaccesses/TA.java:19:43:19:50 | ...[] | +| typeaccesses/TA.java:20:41:20:48 | ...[] | +| typeaccesses/TA.java:31:80:31:82 | ...[] | +| typeaccesses/TA.java:31:124:31:137 | ...[] | diff --git a/java/ql/test/library-tests/typeaccesses/PrintAst.expected b/java/ql/test/library-tests/typeaccesses/PrintAst.expected index 6df9a49f2b2a..efe7fcf7056e 100644 --- a/java/ql/test/library-tests/typeaccesses/PrintAst.expected +++ b/java/ql/test/library-tests/typeaccesses/PrintAst.expected @@ -92,7 +92,7 @@ typeaccesses/TA.java: # 15| 11: [Method] method8 # 15| 3: [TypeAccess] TA.Inner # 15| -1: [TypeAccess] TA -# 15| 0: [WildcardTypeAccess] ? ... +# 15| 0: [WildcardTypeAccess] ? # 15| 5: [BlockStmt] stmt # 15| 0: [ReturnStmt] stmt # 15| 0: [NullLiteral] null @@ -102,3 +102,89 @@ typeaccesses/TA.java: # 16| 5: [BlockStmt] stmt # 16| 0: [ReturnStmt] stmt # 16| 0: [NullLiteral] null +# 18| 13: [Method] method10 +# 18| 3: [TypeAccess] void +#-----| 4: (Parameters) +# 18| 0: [Parameter] l +# 18| 0: [TypeAccess] ArrayList +# 18| 0: [WildcardTypeAccess] ? extends ... +# 18| 0: [TypeAccess] Number +# 18| 5: [BlockStmt] stmt +# 19| 14: [Method] method11 +# 19| 3: [TypeAccess] void +#-----| 4: (Parameters) +# 19| 0: [Parameter] l +# 19| 0: [TypeAccess] ArrayList +# 19| 0: [WildcardTypeAccess] ? extends ... +# 19| 0: [ArrayTypeAccess] ...[] +# 19| 0: [TypeAccess] Number +# 19| 5: [BlockStmt] stmt +# 20| 15: [Method] method12 +# 20| 3: [TypeAccess] void +#-----| 4: (Parameters) +# 20| 0: [Parameter] l +# 20| 0: [TypeAccess] ArrayList +# 20| 0: [WildcardTypeAccess] ? super ... +# 20| 1: [ArrayTypeAccess] ...[] +# 20| 0: [TypeAccess] Number +# 20| 5: [BlockStmt] stmt +# 22| 16: [Interface] I1 +# 23| 17: [Interface] I2 +# 24| 18: [Class] Inner2 +#-----| -2: (Generic Parameters) +# 24| 0: [TypeVariable] T +# 24| 0: [TypeAccess] I1 +# 24| 1: [TypeAccess] I2 +# 25| 19: [Class] Inner3 +#-----| -2: (Generic Parameters) +# 25| 0: [TypeVariable] T +# 25| 0: [TypeAccess] I2 +# 25| 1: [TypeAccess] I1 +# 26| 20: [Class] Inner4 +#-----| -2: (Generic Parameters) +# 26| 0: [TypeVariable] T +# 26| 0: [TypeAccess] Object +# 26| 1: [TypeAccess] I2 +# 26| 2: [TypeAccess] I1 +# 27| 21: [Method] method13 +#-----| 2: (Generic Parameters) +# 27| 0: [TypeVariable] T +# 27| 0: [TypeAccess] I1 +# 27| 1: [TypeAccess] I2 +# 27| 3: [TypeAccess] void +# 27| 5: [BlockStmt] stmt +# 28| 22: [Method] method14 +#-----| 2: (Generic Parameters) +# 28| 0: [TypeVariable] T +# 28| 0: [TypeAccess] I2 +# 28| 1: [TypeAccess] I1 +# 28| 3: [TypeAccess] void +# 28| 5: [BlockStmt] stmt +# 29| 23: [Method] method15 +#-----| 2: (Generic Parameters) +# 29| 0: [TypeVariable] T +# 29| 0: [TypeAccess] Object +# 29| 1: [TypeAccess] I2 +# 29| 2: [TypeAccess] I1 +# 29| 3: [TypeAccess] void +# 29| 5: [BlockStmt] stmt +# 31| 24: [Method] method16 +#-----| 2: (Generic Parameters) +# 31| 0: [TypeVariable] T +# 31| 0: [TypeAccess] Number +# 31| 1: [TypeAccess] Runnable +# 31| 1: [TypeVariable] R +# 31| 0: [TypeAccess] T +# 31| 2: [TypeVariable] S +# 31| 0: [TypeAccess] ArrayList +# 31| 0: [WildcardTypeAccess] ? super ... +# 31| 1: [ArrayTypeAccess] ...[] +# 31| 0: [TypeAccess] R +# 31| 3: [TypeAccess] ArrayList[]>> +# 31| 0: [WildcardTypeAccess] ? super ... +# 31| 1: [TypeAccess] ArrayList[]> +# 31| 0: [WildcardTypeAccess] ? extends ... +# 31| 0: [ArrayTypeAccess] ...[] +# 31| 0: [TypeAccess] ArrayList +# 31| 0: [TypeAccess] T +# 31| 5: [BlockStmt] stmt diff --git a/java/ql/test/library-tests/typeaccesses/TypeAccesses.expected b/java/ql/test/library-tests/typeaccesses/TypeAccesses.expected index a4fbf14dfdc0..85393739312c 100644 --- a/java/ql/test/library-tests/typeaccesses/TypeAccesses.expected +++ b/java/ql/test/library-tests/typeaccesses/TypeAccesses.expected @@ -33,4 +33,39 @@ | typeaccesses/TA.java:15:9:15:10 | TA | | typeaccesses/TA.java:15:9:15:19 | TA.Inner | | typeaccesses/TA.java:16:9:16:26 | List | -| typeaccesses/TA.java:16:24:16:25 | TA | \ No newline at end of file +| typeaccesses/TA.java:16:24:16:25 | TA | +| typeaccesses/TA.java:18:9:18:12 | void | +| typeaccesses/TA.java:18:23:18:49 | ArrayList | +| typeaccesses/TA.java:18:43:18:48 | Number | +| typeaccesses/TA.java:19:9:19:12 | void | +| typeaccesses/TA.java:19:23:19:51 | ArrayList | +| typeaccesses/TA.java:19:43:19:48 | Number | +| typeaccesses/TA.java:20:9:20:12 | void | +| typeaccesses/TA.java:20:23:20:49 | ArrayList | +| typeaccesses/TA.java:20:41:20:46 | Number | +| typeaccesses/TA.java:24:25:24:26 | I1 | +| typeaccesses/TA.java:24:30:24:31 | I2 | +| typeaccesses/TA.java:25:25:25:26 | I2 | +| typeaccesses/TA.java:25:30:25:31 | I1 | +| typeaccesses/TA.java:26:25:26:30 | Object | +| typeaccesses/TA.java:26:34:26:35 | I2 | +| typeaccesses/TA.java:26:39:26:40 | I1 | +| typeaccesses/TA.java:27:20:27:21 | I1 | +| typeaccesses/TA.java:27:25:27:26 | I2 | +| typeaccesses/TA.java:27:29:27:32 | void | +| typeaccesses/TA.java:28:20:28:21 | I2 | +| typeaccesses/TA.java:28:25:28:26 | I1 | +| typeaccesses/TA.java:28:29:28:32 | void | +| typeaccesses/TA.java:29:20:29:25 | Object | +| typeaccesses/TA.java:29:29:29:30 | I2 | +| typeaccesses/TA.java:29:34:29:35 | I1 | +| typeaccesses/TA.java:29:38:29:41 | void | +| typeaccesses/TA.java:31:20:31:25 | Number | +| typeaccesses/TA.java:31:29:31:36 | Runnable | +| typeaccesses/TA.java:31:49:31:49 | T | +| typeaccesses/TA.java:31:62:31:83 | ArrayList | +| typeaccesses/TA.java:31:80:31:80 | R | +| typeaccesses/TA.java:31:86:31:139 | ArrayList[]>> | +| typeaccesses/TA.java:31:104:31:138 | ArrayList[]> | +| typeaccesses/TA.java:31:124:31:135 | ArrayList | +| typeaccesses/TA.java:31:134:31:134 | T | diff --git a/java/ql/test/library-tests/typeaccesses/typeaccesses/TA.java b/java/ql/test/library-tests/typeaccesses/typeaccesses/TA.java index 8fa0ff91291f..52c8c20c0e29 100644 --- a/java/ql/test/library-tests/typeaccesses/typeaccesses/TA.java +++ b/java/ql/test/library-tests/typeaccesses/typeaccesses/TA.java @@ -14,4 +14,19 @@ public > void method4() { } class Inner> { } public TA.Inner method8() { return null; } public java.util.List method9() { return null; } + + public void method10(ArrayList l) { } + public void method11(ArrayList l) { } + public void method12(ArrayList l) { } + + public interface I1 { } + public interface I2 { } + class Inner2 { } + class Inner3 { } + class Inner4 { } + public void method13() { } + public void method14() { } + public void method15() { } + + public > ArrayList[]>> method16() { } } From 87364b3f75ce5c7f206302f57a60fcd24f56f122 Mon Sep 17 00:00:00 2001 From: Marcono1234 Date: Mon, 22 Mar 2021 01:27:09 +0100 Subject: [PATCH 2/2] Address review feedback --- java/ql/src/semmle/code/java/Expr.qll | 4 ++-- java/ql/test/library-tests/locations/WildcardLocations.ql | 2 +- java/ql/test/library-tests/typeaccesses/PrintAst.expected | 2 ++ java/ql/test/library-tests/typeaccesses/typeaccesses/TA.java | 4 +++- 4 files changed, 8 insertions(+), 4 deletions(-) diff --git a/java/ql/src/semmle/code/java/Expr.qll b/java/ql/src/semmle/code/java/Expr.qll index b2b297ed9379..37ebdb749a80 100755 --- a/java/ql/src/semmle/code/java/Expr.qll +++ b/java/ql/src/semmle/code/java/Expr.qll @@ -1743,8 +1743,8 @@ class WildcardTypeAccess extends Expr, @wildcardtypeaccess { Expr getLowerBound() { result.isNthChildOf(this, 1) } /** - * Gets the bound of this wildcard type access, if any, i.e. either the upper - * or the lower bound. + * Gets the bound of this wildcard type access, if any. That is, either + * the upper or the lower bound. */ Expr getBound() { result = getUpperBound() or diff --git a/java/ql/test/library-tests/locations/WildcardLocations.ql b/java/ql/test/library-tests/locations/WildcardLocations.ql index a1b5d0a07bd1..7576cf8ce7c4 100644 --- a/java/ql/test/library-tests/locations/WildcardLocations.ql +++ b/java/ql/test/library-tests/locations/WildcardLocations.ql @@ -17,7 +17,7 @@ private string getValue(WildcardTypeAccess wta) { class WildcardTypeAccessTest extends InlineExpectationsTest { WildcardTypeAccessTest() { this = "WildcardTypeAccessTest" } - override string getARelevantTag() { result = ["wildcardTypeAccess"] } + override string getARelevantTag() { result = "wildcardTypeAccess" } override predicate hasActualResult(Location location, string element, string tag, string value) { exists(WildcardTypeAccess wta | diff --git a/java/ql/test/library-tests/typeaccesses/PrintAst.expected b/java/ql/test/library-tests/typeaccesses/PrintAst.expected index efe7fcf7056e..25f879824a09 100644 --- a/java/ql/test/library-tests/typeaccesses/PrintAst.expected +++ b/java/ql/test/library-tests/typeaccesses/PrintAst.expected @@ -188,3 +188,5 @@ typeaccesses/TA.java: # 31| 0: [TypeAccess] ArrayList # 31| 0: [TypeAccess] T # 31| 5: [BlockStmt] stmt +# 32| 0: [ReturnStmt] stmt +# 32| 0: [NullLiteral] null diff --git a/java/ql/test/library-tests/typeaccesses/typeaccesses/TA.java b/java/ql/test/library-tests/typeaccesses/typeaccesses/TA.java index 52c8c20c0e29..dcd0bc40844f 100644 --- a/java/ql/test/library-tests/typeaccesses/typeaccesses/TA.java +++ b/java/ql/test/library-tests/typeaccesses/typeaccesses/TA.java @@ -28,5 +28,7 @@ public void method13() { } public void method14() { } public void method15() { } - public > ArrayList[]>> method16() { } + public > ArrayList[]>> method16() { + return null; + } }