Merge branch 'master' into taintedmalloc

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/Likely Bugs/Memory Management/AllocaInLoop.qhelp

@@ -0,0 +1,42 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+
+
+<overview>
+<p> The <code>alloca</code> macro allocates memory by expanding the current stack frame. Invoking
+<code>alloca</code> within a loop may lead to a stack overflow because the memory is not released
+until the function returns. 
+</p>
+
+</overview>
+<recommendation>
+
+<p>
+Consider invoking <code>alloca</code> once outside the loop, or using <code>malloc</code> or
+<code>new</code> to allocate memory on the heap if the allocation must be done inside the loop.
+</p>
+
+</recommendation>
+<example>
+<p>The variable <code>path</code> is allocated inside a loop with <code>alloca</code>. Consequently,
+storage for all copies of the path is present in the stack frame until the end of the function.
+</p>
+
+<sample src="AllocaInLoopBad.cpp" />
+
+<p>In the revised example, <code>path</code> is allocated with <code>malloc</code> and freed at the
+end of the loop.
+</p>
+
+<sample src="AllocaInLoopGood.cpp" />
+
+
+</example>
+<references>
+
+  <li>Linux Programmer's Manual: <a href="http://man7.org/linux/man-pages/man3/alloca.3.html">ALLOCA(3)</a>.</li>
+
+</references>
+</qhelp>

cpp/ql/src/Likely Bugs/Memory Management/AllocaInLoopBad.cpp

@@ -0,0 +1,8 @@
+char *dir_path;
+char **dir_entries;
+int count;
+
+for (int i = 0; i < count; i++) {
+  char *path = (char*)alloca(strlen(dir_path) + strlen(dir_entry[i]) + 2);
+  // use path
+}

cpp/ql/src/Likely Bugs/Memory Management/AllocaInLoopGood.cpp

@@ -0,0 +1,9 @@
+char *dir_path;
+char **dir_entries;
+int count;
+
+for (int i = 0; i < count; i++) {
+  char *path = (char*)malloc(strlen(dir_path) + strlen(dir_entry[i]) + 2);
+  // use path
+  free(path);
+}

cpp/ql/src/jsf/4.07 Header Files/AV Rule 35.qhelp

@@ -68,6 +68,9 @@ some are after the final <code>#endif</code>. All three of these things must be
 <li>
   <a href="http://www.cplusplus.com/forum/articles/10627/">Headers and Includes: Why and How</a>
 </li>
+<li>
+  <a href="https://gcc.gnu.org/onlinedocs/cppinternals/Guard-Macros.html">The Multiple-Include Optimization</a>
+</li>
 
 
 </references>

cpp/ql/src/semmle/code/cpp/commons/Alloc.qll

@@ -39,7 +39,16 @@ predicate allocationFunction(Function f)
       name = "MmAllocateNodePagesForMdlEx" or
       name = "MmMapLockedPagesWithReservedMapping" or
       name = "MmMapLockedPages" or
-      name = "MmMapLockedPagesSpecifyCache"
+      name = "MmMapLockedPagesSpecifyCache" or
+      name = "LocalAlloc" or
+      name = "LocalReAlloc" or
+      name = "GlobalAlloc" or
+      name = "GlobalReAlloc" or
+      name = "HeapAlloc" or
+      name = "HeapReAlloc" or
+      name = "VirtualAlloc" or
+      name = "CoTaskMemAlloc" or
+      name = "CoTaskMemRealloc"
     )
   )
 }
@@ -81,7 +90,17 @@ predicate freeFunction(Function f, int argNum)
       (name = "MmFreeMappingAddress" and argNum = 0) or
       (name = "MmFreePagesFromMdl" and argNum = 0) or
       (name = "MmUnmapReservedMapping" and argNum = 0) or
-      (name = "MmUnmapLockedPages" and argNum = 0)
+      (name = "MmUnmapLockedPages" and argNum = 0) or
+      (name = "LocalFree" and argNum = 0) or
+      (name = "GlobalFree" and argNum = 0) or
+      (name = "HeapFree" and argNum = 2) or
+      (name = "VirtualFree" and argNum = 0) or
+      (name = "CoTaskMemFree" and argNum = 0) or
+      (name = "SysFreeString" and argNum = 0) or
+      (name = "LocalReAlloc" and argNum = 0) or
+      (name = "GlobalReAlloc" and argNum = 0) or
+      (name = "HeapReAlloc" and argNum = 2) or
+      (name = "CoTaskMemRealloc" and argNum = 0)
     )
   )
 }

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/src/semmle/code/cpp/rangeanalysis/NanAnalysis.qll

@@ -0,0 +1,41 @@
+import cpp
+private import semmle.code.cpp.rangeanalysis.RangeSSA
+
+/**
+ * Holds if `guard` won't return the value `polarity` when either
+ * operand is NaN.
+ */
+predicate nanExcludingComparison(ComparisonOperation guard, boolean polarity) {
+  polarity = true and
+  (
+    guard instanceof LTExpr or
+    guard instanceof LEExpr or
+    guard instanceof GTExpr or
+    guard instanceof GEExpr or
+    guard instanceof EQExpr
+  )
+  or
+  polarity = false and
+  guard instanceof NEExpr
+}
+
+/**
+ * Holds if `v` is a use of an SSA definition in `def` which cannot be NaN,
+ * by virtue of the guard in `def`.
+ */
+private predicate excludesNan(RangeSsaDefinition def, VariableAccess v) {
+  exists(VariableAccess inCond, ComparisonOperation guard, boolean branch, LocalScopeVariable lsv |
+    def.isGuardPhi(inCond, guard, branch) and
+    inCond.getTarget() = lsv and
+    v = def.getAUse(lsv) and
+    guard.getAnOperand() = inCond and
+    nanExcludingComparison(guard, branch)
+  )
+}
+
+/**
+ * A variable access which cannot be NaN.
+ */
+class NonNanVariableAccess extends VariableAccess {
+  NonNanVariableAccess() { excludesNan(_, this) }
+}

cpp/ql/src/semmle/code/cpp/rangeanalysis/SimpleRangeAnalysis.qll

@@ -45,6 +45,7 @@ import cpp
 private import RangeAnalysisUtils
 import RangeSSA
 import SimpleRangeAnalysisCached
+private import NanAnalysis
 
 /**
  * This fixed set of lower bounds is used when the lower bounds of an
@@ -993,6 +994,25 @@ predicate unanalyzableDefBounds(
   ub = varMaxVal(v)
 }
 
+/**
+ * Holds if in the `branch` branch of a guard `guard` involving `v`,
+ * we know that `v` is not NaN, and therefore it is safe to make range
+ * inferences about `v`.
+ */
+bindingset[guard, v, branch]
+predicate nonNanGuardedVariable(ComparisonOperation guard, VariableAccess v, boolean branch) {
+  v.getType().getUnspecifiedType() instanceof IntegralType
+  or
+  v.getType().getUnspecifiedType() instanceof FloatingPointType and v instanceof NonNanVariableAccess
+  or
+  // The reason the following case is here is to ensure that when we say
+  // `if (x > 5) { ...then... } else { ...else... }`
+  // it is ok to conclude that `x > 5` in the `then`, (though not safe
+  // to conclude that x <= 5 in `else`) even if we had no prior
+  // knowledge of `x` not being `NaN`.
+  nanExcludingComparison(guard, branch)
+}
+
 /**
  * If the guard is a comparison of the form `p*v + q <CMP> r`, then this
  * predicate uses the bounds information for `r` to compute a lower bound
@@ -1004,10 +1024,12 @@ predicate lowerBoundFromGuard(
 ) {
   exists (float childLB, RelationStrictness strictness
   | boundFromGuard(guard, v, childLB, true, strictness, branch)
-  | if (strictness = Nonstrict() or
-        not (v.getType().getUnspecifiedType() instanceof IntegralType))
-      then lb = childLB
-      else lb = childLB+1)
+  | if nonNanGuardedVariable(guard, v, branch)
+    then (if (strictness = Nonstrict() or
+            not (v.getType().getUnspecifiedType() instanceof IntegralType))
+          then lb = childLB
+          else lb = childLB+1)
+    else lb = varMinVal(v.getTarget()))
 }
 
 /**
@@ -1021,10 +1043,12 @@ predicate upperBoundFromGuard(
 ) {
   exists (float childUB, RelationStrictness strictness
   | boundFromGuard(guard, v, childUB, false, strictness, branch)
-  | if (strictness = Nonstrict() or
-        not (v.getType().getUnspecifiedType() instanceof IntegralType))
-      then ub = childUB
-      else ub = childUB-1)
+  | if nonNanGuardedVariable(guard, v, branch)
+    then (if (strictness = Nonstrict() or
+            not (v.getType().getUnspecifiedType() instanceof IntegralType))
+          then ub = childUB
+          else ub = childUB-1)
+    else ub = varMaxVal(v.getTarget()))
 }
 
 /**

cpp/ql/test/query-tests/Likely Bugs/Arithmetic/PointlessComparison/PointlessComparison.c

@@ -265,3 +265,30 @@ int negative_zero(double dbl) {
   }
   return 0;
 }
+
+int nan(double x) {
+  if (x < 0.0) {
+    return 100;
+  }
+  else if (x >= 0.0) { // GOOD [x could be NaN]
+    return 200;
+  }
+  else {
+    return 300;
+  }
+}
+
+int nan2(double x) {
+  if (x == x) {
+    // If x compares with anything at all, it's not NaN
+    if (x < 0.0) {
+      return 100;
+    }
+    else if (x >= 0.0) { // BAD [Always true]
+      return 200;
+    }
+    else {
+      return 300;
+    }
+  }
+}

cpp/ql/test/query-tests/Likely Bugs/Arithmetic/PointlessComparison/PointlessComparison.expected

@@ -32,5 +32,6 @@
 | PointlessComparison.c:129:12:129:16 | ... > ... | Comparison is always false because a <= 3. |
 | PointlessComparison.c:197:7:197:11 | ... < ... | Comparison is always false because x >= 0. |
 | PointlessComparison.c:264:12:264:22 | ... >= ... | Comparison is always true because dbl >= 0 and -0 >= - .... |
+| PointlessComparison.c:287:14:287:21 | ... >= ... | Comparison is always true because x >= 0. |
 | RegressionTests.cpp:57:7:57:22 | ... <= ... | Comparison is always true because * ... <= 4294967295. |
 | Templates.cpp:9:10:9:24 | ... <= ... | Comparison is always true because local <= 32767. |

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