diff --git a/cpp/ql/src/Likely Bugs/Arithmetic/PointlessComparison.ql b/cpp/ql/src/Likely Bugs/Arithmetic/PointlessComparison.ql index 439b90b68a36..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 @@ -31,6 +32,7 @@ from where not cmp.isInMacroExpansion() and not cmp.isFromTemplateInstantiation(_) 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 72ee733709ee..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() } @@ -23,71 +24,29 @@ 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 functionContainsDisabledCodeRecursive(Function f) { + functionContainsDisabledCode(f) or // recurse into function calls exists(FunctionCall fc | fc.getEnclosingFunction() = f and - containsDisabledCode(fc.getTarget()) + functionContainsDisabledCodeRecursive(fc.getTarget()) ) } - /** * 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 functionDefinedInIfDefRecursive(Function f) { + functionDefinedInIfDef(f) or // recurse into function calls exists(FunctionCall fc | fc.getEnclosingFunction() = f and - definedInIfDef(fc.getTarget()) + functionDefinedInIfDefRecursive(fc.getTarget()) ) } @@ -121,8 +80,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 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/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 + ) + ) +} 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