From 669ac2f4b43cfb2d857fbcce525227cd78151831 Mon Sep 17 00:00:00 2001 From: Dave Bartolomeo Date: Mon, 25 Mar 2019 16:19:18 -0700 Subject: [PATCH 1/3] C++: Fix FP in PointlessComparison due to preprocessor Reported by an LGTM customer here: https://discuss.lgtm.com/t/2-false-positives-in-c-for-comparison-is-always-same/1943. Even though the comparison is pointless in the preprocessor configuration in effect during extraction, it is not pointless in other preprocessor configurations. Similar to ExprHasNoEffect, we'll now exclude results in functions that contain preprocessor-excluded code. I factored the similar code already used in ExprHasNoEffect in a non-recursive version into Preprocessor.qll, leaving the recursive version in ExprHasNoEffect.ql. I believe the recursive version is too aggressive for PointerlessComparison, which does no interprocedural analysis. --- .../Arithmetic/PointlessComparison.ql | 1 + .../Likely Typos/ExprHasNoEffect.ql | 54 ++-------------- cpp/ql/src/semmle/code/cpp/Preprocessor.qll | 61 +++++++++++++++++++ .../PointlessComparison/RegressionTests.cpp | 14 +++++ 4 files changed, 82 insertions(+), 48 deletions(-) diff --git a/cpp/ql/src/Likely Bugs/Arithmetic/PointlessComparison.ql b/cpp/ql/src/Likely Bugs/Arithmetic/PointlessComparison.ql index 439b90b68a36..b68203c2a91a 100644 --- a/cpp/ql/src/Likely Bugs/Arithmetic/PointlessComparison.ql +++ b/cpp/ql/src/Likely Bugs/Arithmetic/PointlessComparison.ql @@ -31,6 +31,7 @@ from where not cmp.isInMacroExpansion() and not cmp.isFromTemplateInstantiation(_) and + not containsDisabledCode(cmp.getEnclosingFunction()) and reachablePointlessComparison(cmp, left, right, value, ss) and // a comparison between an enum and zero is always valid because whether diff --git a/cpp/ql/src/Likely Bugs/Likely Typos/ExprHasNoEffect.ql b/cpp/ql/src/Likely Bugs/Likely Typos/ExprHasNoEffect.ql index 72ee733709ee..e19767b9f76f 100644 --- a/cpp/ql/src/Likely Bugs/Likely Typos/ExprHasNoEffect.ql +++ b/cpp/ql/src/Likely Bugs/Likely Typos/ExprHasNoEffect.ql @@ -23,39 +23,12 @@ predicate accessInInitOfForStmt(Expr e) { s.getExpr() = e) } -/** - * Holds if the preprocessor branch `pbd` is on line `pbdStartLine` in file `file`. - */ -predicate pbdLocation(PreprocessorBranchDirective pbd, string file, int pbdStartLine) { - pbd.getLocation().hasLocationInfo(file, pbdStartLine, _, _, _) -} - -/** - * Holds if the body of the function `f` is on lines `fBlockStartLine` to `fBlockEndLine` in file `file`. - */ -predicate functionLocation(Function f, string file, int fBlockStartLine, int fBlockEndLine) { - f.getBlock().getLocation().hasLocationInfo(file, fBlockStartLine, _, fBlockEndLine, _) -} /** * Holds if the function `f`, or a function called by it, contains * code excluded by the preprocessor. */ -predicate containsDisabledCode(Function f) { - // `f` contains a preprocessor branch that was not taken - exists(PreprocessorBranchDirective pbd, string file, int pbdStartLine, int fBlockStartLine, int fBlockEndLine | - functionLocation(f, file, fBlockStartLine, fBlockEndLine) and - pbdLocation(pbd, file, pbdStartLine) and - pbdStartLine <= fBlockEndLine and - pbdStartLine >= fBlockStartLine and - ( - pbd.(PreprocessorBranch).wasNotTaken() or - - // an else either was not taken, or it's corresponding branch - // was not taken. - pbd instanceof PreprocessorElse - ) - ) or - +predicate containsDisabledCodeRecursive(Function f) { + containsDisabledCode(f) or // recurse into function calls exists(FunctionCall fc | fc.getEnclosingFunction() = f and @@ -63,27 +36,12 @@ predicate containsDisabledCode(Function f) { ) } - /** * Holds if the function `f`, or a function called by it, is inside a * preprocessor branch that may have code in another arm */ -predicate definedInIfDef(Function f) { - exists(PreprocessorBranchDirective pbd, string file, int pbdStartLine, int pbdEndLine, int fBlockStartLine, int fBlockEndLine | - functionLocation(f, file, fBlockStartLine, fBlockEndLine) and - pbdLocation(pbd, file, pbdStartLine) and - pbdLocation(pbd.getNext(), file, pbdEndLine) and - pbdStartLine <= fBlockStartLine and - pbdEndLine >= fBlockEndLine and - // pbd is a preprocessor branch where multiple branches exist - ( - pbd.getNext() instanceof PreprocessorElse or - pbd instanceof PreprocessorElse or - pbd.getNext() instanceof PreprocessorElif or - pbd instanceof PreprocessorElif - ) - ) or - +predicate definedInIfDefRecursive(Function f) { + definedInIfDef(f) or // recurse into function calls exists(FunctionCall fc | fc.getEnclosingFunction() = f and @@ -121,8 +79,8 @@ where // EQExprs are covered by CompareWhereAssignMeant.ql not parent instanceof PureExprInVoidContext and not peivc.getEnclosingFunction().isCompilerGenerated() and not peivc.getType() instanceof UnknownType and - not containsDisabledCode(peivc.(FunctionCall).getTarget()) and - not definedInIfDef(peivc.(FunctionCall).getTarget()) and + not containsDisabledCodeRecursive(peivc.(FunctionCall).getTarget()) and + not definedInIfDefRecursive(peivc.(FunctionCall).getTarget()) and if peivc instanceof FunctionCall then exists(Function target | target = peivc.(FunctionCall).getTarget() and diff --git a/cpp/ql/src/semmle/code/cpp/Preprocessor.qll b/cpp/ql/src/semmle/code/cpp/Preprocessor.qll index 59d25df28b22..fe8cbe0701d0 100644 --- a/cpp/ql/src/semmle/code/cpp/Preprocessor.qll +++ b/cpp/ql/src/semmle/code/cpp/Preprocessor.qll @@ -221,3 +221,64 @@ class PreprocessorPragma extends PreprocessorDirective, @ppd_pragma { class PreprocessorLine extends PreprocessorDirective, @ppd_line { override string toString() { result = "#line " + this.getHead() } } + +/** + * Holds if the preprocessor branch `pbd` is on line `pbdStartLine` in file `file`. + */ +private predicate pbdLocation(PreprocessorBranchDirective pbd, string file, int pbdStartLine) { + pbd.getLocation().hasLocationInfo(file, pbdStartLine, _, _, _) +} + +/** + * Holds if the body of the function `f` is on lines `fBlockStartLine` to `fBlockEndLine` in file `file`. + */ +private predicate functionLocation(Function f, string file, int fBlockStartLine, int fBlockEndLine) { + f.getBlock().getLocation().hasLocationInfo(file, fBlockStartLine, _, fBlockEndLine, _) +} + +/** + * Holds if the function `f` is inside a preprocessor branch that may have code in another arm. + */ +predicate definedInIfDef(Function f) { + exists(PreprocessorBranchDirective pbd, string file, int pbdStartLine, int pbdEndLine, int fBlockStartLine, + int fBlockEndLine | + functionLocation(f, file, fBlockStartLine, fBlockEndLine) and + pbdLocation(pbd, file, pbdStartLine) and + pbdLocation(pbd.getNext(), file, pbdEndLine) and + pbdStartLine <= fBlockStartLine and + pbdEndLine >= fBlockEndLine and + // pbd is a preprocessor branch where multiple branches exist + ( + pbd.getNext() instanceof PreprocessorElse or + pbd instanceof PreprocessorElse or + pbd.getNext() instanceof PreprocessorElif or + pbd instanceof PreprocessorElif + ) + ) +} + +/** + * Holds if the function `f`, or a function called by it, contains + * code excluded by the preprocessor. + */ +predicate containsDisabledCode(Function f) { + // `f` contains a preprocessor branch that was not taken + exists(PreprocessorBranchDirective pbd, string file, int pbdStartLine, int fBlockStartLine, int fBlockEndLine | + functionLocation(f, file, fBlockStartLine, fBlockEndLine) and + pbdLocation(pbd, file, pbdStartLine) and + pbdStartLine <= fBlockEndLine and + pbdStartLine >= fBlockStartLine and + ( + pbd.(PreprocessorBranch).wasNotTaken() or + + // an else either was not taken, or it's corresponding branch + // was not taken. + pbd instanceof PreprocessorElse + ) + ) or + // recurse into function calls + exists(FunctionCall fc | + fc.getEnclosingFunction() = f and + containsDisabledCode(fc.getTarget()) + ) +} diff --git a/cpp/ql/test/query-tests/Likely Bugs/Arithmetic/PointlessComparison/RegressionTests.cpp b/cpp/ql/test/query-tests/Likely Bugs/Arithmetic/PointlessComparison/RegressionTests.cpp index 45a95feac537..ce12aa7d0efc 100644 --- a/cpp/ql/test/query-tests/Likely Bugs/Arithmetic/PointlessComparison/RegressionTests.cpp +++ b/cpp/ql/test/query-tests/Likely Bugs/Arithmetic/PointlessComparison/RegressionTests.cpp @@ -66,3 +66,17 @@ int regression_test_01(unsigned long bb) { return 1; } } + +int containsIfDef(int x) { + int result = 0; + if (x > 0) { + result = 1; + } +#if _CONDITION + if (x < 0) { + result = -1; + } +#endif + + return result >= 0; +} \ No newline at end of file From f13fc42a85d53ed8ee9b62b31460ec3771aae64e Mon Sep 17 00:00:00 2001 From: Dave Bartolomeo Date: Tue, 26 Mar 2019 14:36:35 -0700 Subject: [PATCH 2/3] C++: Make recursive predicates recursive and non-recursive predicates non-recursive --- .../Arithmetic/PointlessComparison.ql | 2 +- .../Likely Bugs/Likely Typos/ExprHasNoEffect.ql | 16 ++++++++-------- cpp/ql/src/semmle/code/cpp/Preprocessor.qll | 14 ++++---------- 3 files changed, 13 insertions(+), 19 deletions(-) diff --git a/cpp/ql/src/Likely Bugs/Arithmetic/PointlessComparison.ql b/cpp/ql/src/Likely Bugs/Arithmetic/PointlessComparison.ql index b68203c2a91a..8ff5d438c430 100644 --- a/cpp/ql/src/Likely Bugs/Arithmetic/PointlessComparison.ql +++ b/cpp/ql/src/Likely Bugs/Arithmetic/PointlessComparison.ql @@ -31,7 +31,7 @@ from where not cmp.isInMacroExpansion() and not cmp.isFromTemplateInstantiation(_) and - not containsDisabledCode(cmp.getEnclosingFunction()) and + not functionContainsDisabledCode(cmp.getEnclosingFunction()) and reachablePointlessComparison(cmp, left, right, value, ss) and // a comparison between an enum and zero is always valid because whether diff --git a/cpp/ql/src/Likely Bugs/Likely Typos/ExprHasNoEffect.ql b/cpp/ql/src/Likely Bugs/Likely Typos/ExprHasNoEffect.ql index e19767b9f76f..12b95096be89 100644 --- a/cpp/ql/src/Likely Bugs/Likely Typos/ExprHasNoEffect.ql +++ b/cpp/ql/src/Likely Bugs/Likely Typos/ExprHasNoEffect.ql @@ -27,12 +27,12 @@ predicate accessInInitOfForStmt(Expr e) { * Holds if the function `f`, or a function called by it, contains * code excluded by the preprocessor. */ -predicate containsDisabledCodeRecursive(Function f) { - containsDisabledCode(f) or +predicate functionContainsDisabledCodeRecursive(Function f) { + functionContainsDisabledCode(f) or // recurse into function calls exists(FunctionCall fc | fc.getEnclosingFunction() = f and - containsDisabledCode(fc.getTarget()) + functionContainsDisabledCodeRecursive(fc.getTarget()) ) } @@ -40,12 +40,12 @@ predicate containsDisabledCodeRecursive(Function f) { * Holds if the function `f`, or a function called by it, is inside a * preprocessor branch that may have code in another arm */ -predicate definedInIfDefRecursive(Function f) { - definedInIfDef(f) or +predicate functionDefinedInIfDefRecursive(Function f) { + functionDefinedInIfDef(f) or // recurse into function calls exists(FunctionCall fc | fc.getEnclosingFunction() = f and - definedInIfDef(fc.getTarget()) + functionDefinedInIfDefRecursive(fc.getTarget()) ) } @@ -79,8 +79,8 @@ where // EQExprs are covered by CompareWhereAssignMeant.ql not parent instanceof PureExprInVoidContext and not peivc.getEnclosingFunction().isCompilerGenerated() and not peivc.getType() instanceof UnknownType and - not containsDisabledCodeRecursive(peivc.(FunctionCall).getTarget()) and - not definedInIfDefRecursive(peivc.(FunctionCall).getTarget()) and + not functionContainsDisabledCodeRecursive(peivc.(FunctionCall).getTarget()) and + not functionDefinedInIfDefRecursive(peivc.(FunctionCall).getTarget()) and if peivc instanceof FunctionCall then exists(Function target | target = peivc.(FunctionCall).getTarget() and diff --git a/cpp/ql/src/semmle/code/cpp/Preprocessor.qll b/cpp/ql/src/semmle/code/cpp/Preprocessor.qll index fe8cbe0701d0..1d1881386232 100644 --- a/cpp/ql/src/semmle/code/cpp/Preprocessor.qll +++ b/cpp/ql/src/semmle/code/cpp/Preprocessor.qll @@ -239,7 +239,7 @@ private predicate functionLocation(Function f, string file, int fBlockStartLine, /** * Holds if the function `f` is inside a preprocessor branch that may have code in another arm. */ -predicate definedInIfDef(Function f) { +predicate functionDefinedInIfDef(Function f) { exists(PreprocessorBranchDirective pbd, string file, int pbdStartLine, int pbdEndLine, int fBlockStartLine, int fBlockEndLine | functionLocation(f, file, fBlockStartLine, fBlockEndLine) and @@ -258,13 +258,12 @@ predicate definedInIfDef(Function f) { } /** - * Holds if the function `f`, or a function called by it, contains - * code excluded by the preprocessor. + * Holds if the function `f` contains code excluded by the preprocessor. */ -predicate containsDisabledCode(Function f) { +predicate functionContainsDisabledCode(Function f) { // `f` contains a preprocessor branch that was not taken exists(PreprocessorBranchDirective pbd, string file, int pbdStartLine, int fBlockStartLine, int fBlockEndLine | - functionLocation(f, file, fBlockStartLine, fBlockEndLine) and + functionLocation(f, file, fBlockStartLine, fBlockEndLine) and pbdLocation(pbd, file, pbdStartLine) and pbdStartLine <= fBlockEndLine and pbdStartLine >= fBlockStartLine and @@ -275,10 +274,5 @@ predicate containsDisabledCode(Function f) { // was not taken. pbd instanceof PreprocessorElse ) - ) or - // recurse into function calls - exists(FunctionCall fc | - fc.getEnclosingFunction() = f and - containsDisabledCode(fc.getTarget()) ) } From 127b759bad23aa7c02ec14e466e29b489f9f1cde Mon Sep 17 00:00:00 2001 From: Dave Bartolomeo Date: Tue, 26 Mar 2019 14:51:28 -0700 Subject: [PATCH 3/3] C++: Move a couple predicates into Exclusions.qll --- .../Arithmetic/PointlessComparison.ql | 1 + .../Likely Typos/ExprHasNoEffect.ql | 1 + cpp/ql/src/semmle/code/cpp/Preprocessor.qll | 55 ----------------- .../semmle/code/cpp/commons/Exclusions.qll | 60 +++++++++++++++++++ 4 files changed, 62 insertions(+), 55 deletions(-) create mode 100644 cpp/ql/src/semmle/code/cpp/commons/Exclusions.qll diff --git a/cpp/ql/src/Likely Bugs/Arithmetic/PointlessComparison.ql b/cpp/ql/src/Likely Bugs/Arithmetic/PointlessComparison.ql index 8ff5d438c430..b3669d924986 100644 --- a/cpp/ql/src/Likely Bugs/Arithmetic/PointlessComparison.ql +++ b/cpp/ql/src/Likely Bugs/Arithmetic/PointlessComparison.ql @@ -12,6 +12,7 @@ * readability */ import cpp +private import semmle.code.cpp.commons.Exclusions private import semmle.code.cpp.rangeanalysis.PointlessComparison private import semmle.code.cpp.rangeanalysis.RangeAnalysisUtils import UnsignedGEZero diff --git a/cpp/ql/src/Likely Bugs/Likely Typos/ExprHasNoEffect.ql b/cpp/ql/src/Likely Bugs/Likely Typos/ExprHasNoEffect.ql index 12b95096be89..b8a04c193149 100644 --- a/cpp/ql/src/Likely Bugs/Likely Typos/ExprHasNoEffect.ql +++ b/cpp/ql/src/Likely Bugs/Likely Typos/ExprHasNoEffect.ql @@ -11,6 +11,7 @@ * external/cwe/cwe-561 */ import cpp +private import semmle.code.cpp.commons.Exclusions class PureExprInVoidContext extends ExprInVoidContext { PureExprInVoidContext() { this.isPure() } diff --git a/cpp/ql/src/semmle/code/cpp/Preprocessor.qll b/cpp/ql/src/semmle/code/cpp/Preprocessor.qll index 1d1881386232..59d25df28b22 100644 --- a/cpp/ql/src/semmle/code/cpp/Preprocessor.qll +++ b/cpp/ql/src/semmle/code/cpp/Preprocessor.qll @@ -221,58 +221,3 @@ class PreprocessorPragma extends PreprocessorDirective, @ppd_pragma { class PreprocessorLine extends PreprocessorDirective, @ppd_line { override string toString() { result = "#line " + this.getHead() } } - -/** - * Holds if the preprocessor branch `pbd` is on line `pbdStartLine` in file `file`. - */ -private predicate pbdLocation(PreprocessorBranchDirective pbd, string file, int pbdStartLine) { - pbd.getLocation().hasLocationInfo(file, pbdStartLine, _, _, _) -} - -/** - * Holds if the body of the function `f` is on lines `fBlockStartLine` to `fBlockEndLine` in file `file`. - */ -private predicate functionLocation(Function f, string file, int fBlockStartLine, int fBlockEndLine) { - f.getBlock().getLocation().hasLocationInfo(file, fBlockStartLine, _, fBlockEndLine, _) -} - -/** - * Holds if the function `f` is inside a preprocessor branch that may have code in another arm. - */ -predicate functionDefinedInIfDef(Function f) { - exists(PreprocessorBranchDirective pbd, string file, int pbdStartLine, int pbdEndLine, int fBlockStartLine, - int fBlockEndLine | - functionLocation(f, file, fBlockStartLine, fBlockEndLine) and - pbdLocation(pbd, file, pbdStartLine) and - pbdLocation(pbd.getNext(), file, pbdEndLine) and - pbdStartLine <= fBlockStartLine and - pbdEndLine >= fBlockEndLine and - // pbd is a preprocessor branch where multiple branches exist - ( - pbd.getNext() instanceof PreprocessorElse or - pbd instanceof PreprocessorElse or - pbd.getNext() instanceof PreprocessorElif or - pbd instanceof PreprocessorElif - ) - ) -} - -/** - * Holds if the function `f` contains code excluded by the preprocessor. - */ -predicate functionContainsDisabledCode(Function f) { - // `f` contains a preprocessor branch that was not taken - exists(PreprocessorBranchDirective pbd, string file, int pbdStartLine, int fBlockStartLine, int fBlockEndLine | - functionLocation(f, file, fBlockStartLine, fBlockEndLine) and - pbdLocation(pbd, file, pbdStartLine) and - pbdStartLine <= fBlockEndLine and - pbdStartLine >= fBlockStartLine and - ( - pbd.(PreprocessorBranch).wasNotTaken() or - - // an else either was not taken, or it's corresponding branch - // was not taken. - pbd instanceof PreprocessorElse - ) - ) -} diff --git a/cpp/ql/src/semmle/code/cpp/commons/Exclusions.qll b/cpp/ql/src/semmle/code/cpp/commons/Exclusions.qll new file mode 100644 index 000000000000..9c238022cb70 --- /dev/null +++ b/cpp/ql/src/semmle/code/cpp/commons/Exclusions.qll @@ -0,0 +1,60 @@ +/** + * Common predicates used to exclude results from a query based on heuristics. + */ + +import cpp + +/** + * Holds if the preprocessor branch `pbd` is on line `pbdStartLine` in file `file`. + */ +private predicate pbdLocation(PreprocessorBranchDirective pbd, string file, int pbdStartLine) { + pbd.getLocation().hasLocationInfo(file, pbdStartLine, _, _, _) +} + +/** + * Holds if the body of the function `f` is on lines `fBlockStartLine` to `fBlockEndLine` in file `file`. + */ +private predicate functionLocation(Function f, string file, int fBlockStartLine, int fBlockEndLine) { + f.getBlock().getLocation().hasLocationInfo(file, fBlockStartLine, _, fBlockEndLine, _) +} + +/** + * Holds if the function `f` is inside a preprocessor branch that may have code in another arm. + */ +predicate functionDefinedInIfDef(Function f) { + exists(PreprocessorBranchDirective pbd, string file, int pbdStartLine, int pbdEndLine, int fBlockStartLine, + int fBlockEndLine | + functionLocation(f, file, fBlockStartLine, fBlockEndLine) and + pbdLocation(pbd, file, pbdStartLine) and + pbdLocation(pbd.getNext(), file, pbdEndLine) and + pbdStartLine <= fBlockStartLine and + pbdEndLine >= fBlockEndLine and + // pbd is a preprocessor branch where multiple branches exist + ( + pbd.getNext() instanceof PreprocessorElse or + pbd instanceof PreprocessorElse or + pbd.getNext() instanceof PreprocessorElif or + pbd instanceof PreprocessorElif + ) + ) +} + +/** + * Holds if the function `f` contains code excluded by the preprocessor. + */ +predicate functionContainsDisabledCode(Function f) { + // `f` contains a preprocessor branch that was not taken + exists(PreprocessorBranchDirective pbd, string file, int pbdStartLine, int fBlockStartLine, int fBlockEndLine | + functionLocation(f, file, fBlockStartLine, fBlockEndLine) and + pbdLocation(pbd, file, pbdStartLine) and + pbdStartLine <= fBlockEndLine and + pbdStartLine >= fBlockStartLine and + ( + pbd.(PreprocessorBranch).wasNotTaken() or + + // an else either was not taken, or it's corresponding branch + // was not taken. + pbd instanceof PreprocessorElse + ) + ) +}