Merge pull request #1165 from dave-bartolomeo/dave/CompareFP

C++: Fix FP in PointlessComparison due to preprocessor

Author: geoffw0

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

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

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
+    )
+  )
+}

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;
+}