From 460eddd7812e2b50592d0679c549ad5f207921d9 Mon Sep 17 00:00:00 2001 From: erik-krogh Date: Tue, 16 Aug 2022 08:55:49 +0200 Subject: [PATCH 1/3] add ql/override-any --- .../performance/VarUnusedInDisjunctQuery.qll | 27 +++++++++++++ .../performance/VarUnusedInDisjunct.ql | 27 +------------ ql/ql/src/queries/style/OverrideAny.ql | 40 +++++++++++++++++++ 3 files changed, 68 insertions(+), 26 deletions(-) create mode 100644 ql/ql/src/codeql_ql/performance/VarUnusedInDisjunctQuery.qll create mode 100644 ql/ql/src/queries/style/OverrideAny.ql diff --git a/ql/ql/src/codeql_ql/performance/VarUnusedInDisjunctQuery.qll b/ql/ql/src/codeql_ql/performance/VarUnusedInDisjunctQuery.qll new file mode 100644 index 00000000000..503ad6db3ad --- /dev/null +++ b/ql/ql/src/codeql_ql/performance/VarUnusedInDisjunctQuery.qll @@ -0,0 +1,27 @@ +import ql + +/** + * Holds if we assume `t` is a small type, and + * variables of this type are therefore not an issue in cartesian products. + */ +predicate isSmallType(Type t) { + t.getName() = "string" // DataFlow::Configuration and the like + or + exists(NewType newType | newType = t.getDeclaration() | + forex(NewTypeBranch branch | branch = newType.getABranch() | branch.getArity() = 0) + ) + or + t.getName() = "boolean" + or + exists(NewType newType | newType = t.getDeclaration() | + forex(NewTypeBranch branch | branch = newType.getABranch() | + isSmallType(branch.getReturnType()) + ) + ) + or + exists(NewTypeBranch branch | t = branch.getReturnType() | + forall(Type param | param = branch.getParameterType(_) | isSmallType(param)) + ) + or + isSmallType(t.getASuperType()) +} diff --git a/ql/ql/src/queries/performance/VarUnusedInDisjunct.ql b/ql/ql/src/queries/performance/VarUnusedInDisjunct.ql index c26b47554fe..2d85b872153 100644 --- a/ql/ql/src/queries/performance/VarUnusedInDisjunct.ql +++ b/ql/ql/src/queries/performance/VarUnusedInDisjunct.ql @@ -10,6 +10,7 @@ */ import ql +import codeql_ql.performance.VarUnusedInDisjunctQuery /** * Holds if `node` bind `var` in a (transitive) child node. @@ -48,32 +49,6 @@ predicate alwaysBindsVar(VarDef var, AstNode node) { exists(IfFormula ifForm | ifForm = node | alwaysBindsVar(var, ifForm.getCondition())) } -/** - * Holds if we assume `t` is a small type, and - * variables of this type are therefore not an issue in cartesian products. - */ -predicate isSmallType(Type t) { - t.getName() = "string" // DataFlow::Configuration and the like - or - exists(NewType newType | newType = t.getDeclaration() | - forex(NewTypeBranch branch | branch = newType.getABranch() | branch.getArity() = 0) - ) - or - t.getName() = "boolean" - or - exists(NewType newType | newType = t.getDeclaration() | - forex(NewTypeBranch branch | branch = newType.getABranch() | - isSmallType(branch.getReturnType()) - ) - ) - or - exists(NewTypeBranch branch | t = branch.getReturnType() | - forall(Type param | param = branch.getParameterType(_) | isSmallType(param)) - ) - or - isSmallType(t.getASuperType()) -} - /** * Holds if `pred` is inlined. */ diff --git a/ql/ql/src/queries/style/OverrideAny.ql b/ql/ql/src/queries/style/OverrideAny.ql new file mode 100644 index 00000000000..0c7d82fd3c8 --- /dev/null +++ b/ql/ql/src/queries/style/OverrideAny.ql @@ -0,0 +1,40 @@ +/** + * @name Override with unmentioned parameter + * @description A predicate that overrides the default behavior but doesn't mention a parameter is suspicious. + * @kind problem + * @problem.severity warning + * @id ql/override-any + * @precision very-high + */ + +import ql +import codeql_ql.performance.VarUnusedInDisjunctQuery + +AstNode param(Predicate pred, string name, Type t) { + result = pred.getParameter(_) and + result.(VarDecl).getName() = name and + result.(VarDecl).getType() = t + or + result = pred.getReturnTypeExpr() and + name = "result" and + t = pred.getReturnType() +} + +predicate hasAccess(Predicate pred, string name) { + exists(param(pred, name, _).(VarDecl).getAnAccess()) + or + name = "result" and + exists(param(pred, name, _)) and + exists(ResultAccess res | res.getEnclosingPredicate() = pred) +} + +from Predicate pred, AstNode param, string name, Type paramType +where + pred.hasAnnotation("override") and + param = param(pred, name, paramType) and + not hasAccess(pred, name) and + not pred.getBody() instanceof NoneCall and + exists(pred.getBody()) and + not isSmallType(pred.getParent().(Class).getType()) and + not isSmallType(paramType) +select pred, "Override predicate doesn't mention $@.", param, name From 107cbb29b1e53611b1c069fcefb161426f488e6a Mon Sep 17 00:00:00 2001 From: erik-krogh Date: Tue, 8 Nov 2022 20:34:58 +0100 Subject: [PATCH 2/3] guide users towards using exists(variable) in ql/override-any --- ql/ql/src/queries/style/OverrideAny.ql | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/ql/ql/src/queries/style/OverrideAny.ql b/ql/ql/src/queries/style/OverrideAny.ql index 0c7d82fd3c8..f00db8eb0ee 100644 --- a/ql/ql/src/queries/style/OverrideAny.ql +++ b/ql/ql/src/queries/style/OverrideAny.ql @@ -37,4 +37,5 @@ where exists(pred.getBody()) and not isSmallType(pred.getParent().(Class).getType()) and not isSmallType(paramType) -select pred, "Override predicate doesn't mention $@.", param, name +select pred, "Override predicate doesn't mention $@. Maybe mention it in a 'exists(" + name + ")'?", + param, name From c1727ba005ff77b49f5f367dc6b646fd4ed6ba0e Mon Sep 17 00:00:00 2001 From: erik-krogh Date: Tue, 8 Nov 2022 20:35:07 +0100 Subject: [PATCH 3/3] lower precision to high in ql/override-any --- ql/ql/src/queries/style/OverrideAny.ql | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ql/ql/src/queries/style/OverrideAny.ql b/ql/ql/src/queries/style/OverrideAny.ql index f00db8eb0ee..ed3a762f0e9 100644 --- a/ql/ql/src/queries/style/OverrideAny.ql +++ b/ql/ql/src/queries/style/OverrideAny.ql @@ -4,7 +4,7 @@ * @kind problem * @problem.severity warning * @id ql/override-any - * @precision very-high + * @precision high */ import ql