Merge master into next.

master as of dc3c5a684c
Version numbers resolved in favour of `next`.
C++ expected output file updated to accept test output.

cpp/ql/src/Best Practices/Likely Errors/EmptyBlock.ql

@@ -29,6 +29,10 @@ class AffectedFile extends File {
   }
 }
 
+/**
+ * A block, or an element we might find textually within a block that is
+ * not a child of it in the AST.
+ */
 class BlockOrNonChild extends Element {
   BlockOrNonChild() {
     ( this instanceof Block
@@ -68,6 +72,9 @@ class BlockOrNonChild extends Element {
   }
 }
 
+/**
+ * A block that contains a non-child element.
+ */
 predicate emptyBlockContainsNonchild(Block b) {
   emptyBlock(_, b) and
   exists(BlockOrNonChild c, AffectedFile file |
@@ -78,7 +85,27 @@ predicate emptyBlockContainsNonchild(Block b) {
   )
 }
 
+/**
+ * A block that is entirely on one line, which also contains a comment.  Chances
+ * are the comment is intended to refer to the block.
+ */
+predicate lineComment(Block b) {
+  emptyBlock(_, b) and
+  exists(Location bLocation, File f, int line |
+    bLocation = b.getLocation() and
+    f = bLocation.getFile() and
+    line = bLocation.getStartLine() and
+    line = bLocation.getEndLine() and
+    exists(Comment c, Location cLocation |
+      cLocation = c.getLocation() and
+      cLocation.getFile() = f and
+      cLocation.getStartLine() = line
+    )
+  )
+}
+
 from ControlStructure s, Block eb
 where emptyBlock(s, eb)
   and not emptyBlockContainsNonchild(eb)
+  and not lineComment(eb)
 select eb, "Empty block without comment"

cpp/ql/src/Security/CWE/CWE-253/HResultBooleanConversion.ql

@@ -48,6 +48,7 @@ where exists
     ctls.getControllingExpr() = e1
     and e1.getType().(TypedefType).hasName("HRESULT")
     and not isHresultBooleanConverted(e1)
+    and not ctls instanceof SwitchStmt // not controlled by a boolean condition
     and msg = "Direct usage of a type " + e1.getType().toString() + " as a conditional expression"
   ) 
   or

cpp/ql/src/jsf/4.10 Classes/AV Rule 82.ql

@@ -25,7 +25,7 @@ predicate pointerThis(Expr e) {
   // `f(...)`
   // (includes `this = ...`, where `=` is overloaded so a `FunctionCall`)
   exists(FunctionCall fc | fc = e and callOnThis(fc) |
-    exists(fc.getTarget().getBlock()) implies returnsPointerThis(fc.getTarget())
+    returnsPointerThis(fc.getTarget())
   ) or
 
   // `this = ...` (where `=` is not overloaded, so an `AssignExpr`) 
@@ -38,22 +38,33 @@ predicate dereferenceThis(Expr e) {
   // `f(...)`
   // (includes `*this = ...`, where `=` is overloaded so a `FunctionCall`)
   exists(FunctionCall fc | fc = e and callOnThis(fc) |
-    exists(fc.getTarget().getBlock()) implies returnsDereferenceThis(fc.getTarget())
+    returnsDereferenceThis(fc.getTarget())
   ) or
 
   // `*this = ...` (where `=` is not overloaded, so an `AssignExpr`) 
   dereferenceThis(e.(AssignExpr).getLValue())
 }
 
+/**
+ * Holds if all `return` statements in `f` return `this`, possibly indirectly.
+ * This includes functions whose body is not in the database.
+ */
 predicate returnsPointerThis(Function f) {
-  forex(ReturnStmt s | s.getEnclosingFunction() = f |
+  f.getType().getUnspecifiedType() instanceof PointerType and
+  forall(ReturnStmt s | s.getEnclosingFunction() = f and reachable(s) |
     // `return this`
     pointerThis(s.getExpr())
   )
 }
 
+/**
+ * Holds if all `return` statements in `f` return a reference to `*this`,
+ * possibly indirectly. This includes functions whose body is not in the
+ * database.
+ */
 predicate returnsDereferenceThis(Function f) {
-  forex(ReturnStmt s | s.getEnclosingFunction() = f |
+  f.getType().getUnspecifiedType() instanceof ReferenceType and
+  forall(ReturnStmt s | s.getEnclosingFunction() = f and reachable(s) |
     // `return *this`
     dereferenceThis(s.getExpr())
   )
@@ -72,7 +83,6 @@ predicate assignOperatorWithWrongType(Operator op, string msg) {
 predicate assignOperatorWithWrongResult(Operator op, string msg) {
   op.hasName("operator=")
   and not returnsDereferenceThis(op)
-  and exists(op.getBlock())
   and not op.getType() instanceof VoidType
   and not assignOperatorWithWrongType(op, _)
   and msg = "Assignment operator in class " + op.getDeclaringType().getName() + " does not return a reference to *this."

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

@@ -1,29 +1,6 @@
 import cpp
 import semmle.code.cpp.dataflow.DataFlow
 
-/**
- * Holds if `sizeof(s)` occurs as part of the parameter of a dynamic
- * memory allocation (`malloc`, `realloc`, etc.), except if `sizeof(s)`
- * only ever occurs as the immediate parameter to allocations.
- *
- * For example, holds for `s` if it occurs as
- * ```
- * malloc(sizeof(s) + 100 * sizeof(char))
- * ```
- * but not if it only ever occurs as
- * ```
- * malloc(sizeof(s))
- * ```
-*/
-private predicate isDynamicallyAllocatedWithDifferentSize(Class s) {
-  exists(SizeofTypeOperator sof |
-    sof.getTypeOperand().getUnspecifiedType() = s |
-    // Check all ancestor nodes except the immediate parent for
-    // allocations.
-    isStdLibAllocationExpr(sof.getParent().(Expr).getParent+())
-  )
-}
-
 /**
  * Holds if `v` is a member variable of `c` that looks like it might be variable sized in practice.  For
  * example:
@@ -34,15 +11,40 @@ private predicate isDynamicallyAllocatedWithDifferentSize(Class s) {
  * };
  * ```
  * This requires that `v` is an array of size 0 or 1, and `v` is the last member of `c`.  In addition,
- * there must be at least one instance where a `c` pointer is allocated with additional space. 
+ * there must be at least one instance where a `c` pointer is allocated with additional space.  For
+ * example, holds for `c` if it occurs as
+ * ```
+ * malloc(sizeof(c) + 100 * sizeof(char))
+ * ```
+ * but not if it only ever occurs as
+ * ```
+ * malloc(sizeof(c))
+ * ``` 
  */
 predicate memberMayBeVarSize(Class c, MemberVariable v) {
   exists(int i |
+    // `v` is the last field in `c`
     i = max(int j | c.getCanonicalMember(j) instanceof Field | j) and
     v = c.getCanonicalMember(i) and
-    v.getType().getUnspecifiedType().(ArrayType).getSize() <= 1
-  ) and
-  isDynamicallyAllocatedWithDifferentSize(c)
+
+    // v is an array of size at most 1
+    v.getType().getUnspecifiedType().(ArrayType).getArraySize() <= 1
+  ) and (
+    exists(SizeofOperator so |
+      // `sizeof(c)` is taken
+      so.(SizeofTypeOperator).getTypeOperand().getUnspecifiedType() = c or
+      so.(SizeofExprOperator).getExprOperand().getType().getUnspecifiedType() = c |
+
+      // arithmetic is performed on the result
+      so.getParent*() instanceof AddExpr
+    ) or exists(AddressOfExpr aoe |
+      // `&(c.v)` is taken
+      aoe.getAddressable() = v
+    ) or exists(BuiltInOperationOffsetOf oo |
+      // `offsetof(c, v)` using a builtin
+      oo.getAChild().(VariableAccess).getTarget() = v
+    )
+  )
 }
 
 /**
@@ -81,19 +83,21 @@ int getBufferSize(Expr bufferExpr, Element why) {
     // buffer is a fixed size dynamic allocation
     isFixedSizeAllocationExpr(bufferExpr, result) and
     why = bufferExpr
-  ) or (
+  ) or exists(DataFlow::ExprNode bufferExprNode |
     // dataflow (all sources must be the same size)
+    bufferExprNode = DataFlow::exprNode(bufferExpr) and
+ 
     result = min(Expr def |
-      DataFlow::localFlowStep(DataFlow::exprNode(def), DataFlow::exprNode(bufferExpr)) |
+      DataFlow::localFlowStep(DataFlow::exprNode(def), bufferExprNode) |
       getBufferSize(def, _)
     ) and result = max(Expr def |
-      DataFlow::localFlowStep(DataFlow::exprNode(def), DataFlow::exprNode(bufferExpr)) |
+      DataFlow::localFlowStep(DataFlow::exprNode(def), bufferExprNode) |
       getBufferSize(def, _)
     ) and
 
     // find reason
     exists(Expr def |
-      DataFlow::localFlowStep(DataFlow::exprNode(def), DataFlow::exprNode(bufferExpr)) |
+      DataFlow::localFlowStep(DataFlow::exprNode(def), bufferExprNode) |
       why = def or
       exists(getBufferSize(def, why))
     )

cpp/ql/src/semmle/code/cpp/controlflow/SSAUtils.qll

@@ -124,11 +124,17 @@ cached library class SSAHelper extends int {
      * Modern Compiler Implementation by Andrew Appel.
      */
     private predicate frontier_phi_node(LocalScopeVariable v, BasicBlock b) {
-        exists(BasicBlock x | dominanceFrontier(x, b) and ssa_defn(v, _, x, _))
+        exists(BasicBlock x | dominanceFrontier(x, b) and ssa_defn_rec(v, x))
         /* We can also eliminate those nodes where the variable is not live on any incoming edge */
         and live_at_start_of_bb(v, b)
     }
 
+    private predicate ssa_defn_rec(LocalScopeVariable v, BasicBlock b) {
+        phi_node(v, b)
+        or
+        variableUpdate(v, _, b, _)
+    }
+  
     /**
      * Holds if `v` is defined, for the purpose of SSA, at `node`, which is at
      * position `index` in block `b`. This includes definitions from phi nodes.

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

@@ -293,6 +293,61 @@ predicate analyzableDef(RangeSsaDefinition def, LocalScopeVariable v) {
   assignmentDef(def, v, _) or defDependsOnDef(def, v, _, _)
 }
 
+/**
+ * Computes a normal form of `x` where -0.0 has changed to +0.0. This can be
+ * needed on the lesser side of a floating-point comparison or on both sides of
+ * a floating point equality because QL does not follow IEEE in floating-point
+ * comparisons but instead defines -0.0 to be less than and distinct from 0.0.
+ */
+bindingset[x]
+private float normalizeFloatUp(float x) {
+  result = x + 0.0
+}
+
+/**
+ * Computes `x + y`, rounded towards +Inf. This is the general case where both
+ * `x` and `y` may be large numbers.
+ */
+bindingset[x, y]
+private float addRoundingUp(float x, float y) {
+  if normalizeFloatUp((x + y) - x) < y or normalizeFloatUp((x + y) - y) < x
+  then result = (x + y).nextUp()
+  else result = (x + y)
+}
+
+/**
+ * Computes `x + y`, rounded towards -Inf. This is the general case where both
+ * `x` and `y` may be large numbers.
+ */
+bindingset[x, y]
+private float addRoundingDown(float x, float y) {
+  if (x + y) - x > normalizeFloatUp(y) or (x + y) - y > normalizeFloatUp(x)
+  then result = (x + y).nextDown()
+  else result = (x + y)
+}
+
+/**
+ * Computes `x + small`, rounded towards +Inf, where `small` is a small
+ * constant.
+ */
+bindingset[x, small]
+private float addRoundingUpSmall(float x, float small) {
+  if (x + small) - x < small
+  then result = (x + small).nextUp()
+  else result = (x + small)
+}
+
+/**
+ * Computes `x + small`, rounded towards -Inf, where `small` is a small
+ * constant.
+ */
+bindingset[x, small]
+private float addRoundingDownSmall(float x, float small) {
+  if (x + small) - x > small
+  then result = (x + small).nextDown()
+  else result = (x + small)
+}
+
 /**
  * Gets the truncated lower bounds of the fully converted expression.
  */
@@ -470,13 +525,13 @@ float getLowerBoundsImpl(Expr expr) {
   | expr = addExpr and
     xLow = getFullyConvertedLowerBounds(addExpr.getLeftOperand()) and
     yLow = getFullyConvertedLowerBounds(addExpr.getRightOperand()) and
-    result = xLow+yLow)
+    result = addRoundingDown(xLow, yLow))
   or
   exists (SubExpr subExpr, float xLow, float yHigh
   | expr = subExpr and
     xLow = getFullyConvertedLowerBounds(subExpr.getLeftOperand()) and
     yHigh = getFullyConvertedUpperBounds(subExpr.getRightOperand()) and
-    result = xLow-yHigh)
+    result = addRoundingDown(xLow, -yHigh))
   or
   exists (PrefixIncrExpr incrExpr, float xLow
   | expr = incrExpr and
@@ -486,7 +541,7 @@ float getLowerBoundsImpl(Expr expr) {
   exists (PrefixDecrExpr decrExpr, float xLow
   | expr = decrExpr and
     xLow = getFullyConvertedLowerBounds(decrExpr.getOperand()) and
-    result = xLow-1)
+    result = addRoundingDownSmall(xLow, -1))
   or
   // `PostfixIncrExpr` and `PostfixDecrExpr` return the value of their
   // operand. The incrementing/decrementing behavior is handled in
@@ -592,18 +647,18 @@ float getUpperBoundsImpl(Expr expr) {
   | expr = addExpr and
     xHigh = getFullyConvertedUpperBounds(addExpr.getLeftOperand()) and
     yHigh = getFullyConvertedUpperBounds(addExpr.getRightOperand()) and
-    result = xHigh+yHigh)
+    result = addRoundingUp(xHigh, yHigh))
   or
   exists (SubExpr subExpr, float xHigh, float yLow
   | expr = subExpr and
     xHigh = getFullyConvertedUpperBounds(subExpr.getLeftOperand()) and
     yLow = getFullyConvertedLowerBounds(subExpr.getRightOperand()) and
-    result = xHigh-yLow)
+    result = addRoundingUp(xHigh, -yLow))
   or
   exists (PrefixIncrExpr incrExpr, float xHigh
   | expr = incrExpr and
     xHigh = getFullyConvertedUpperBounds(incrExpr.getOperand()) and
-    result = xHigh+1)
+    result = addRoundingUpSmall(xHigh, 1))
   or
   exists (PrefixDecrExpr decrExpr, float xHigh
   | expr = decrExpr and
@@ -796,7 +851,7 @@ float getDefLowerBoundsImpl(RangeSsaDefinition def, LocalScopeVariable v) {
     assignAdd.getLValue() = nextDef.getAUse(v) and
     lhsLB = getDefLowerBounds(nextDef, v) and
     rhsLB = getFullyConvertedLowerBounds(assignAdd.getRValue()) and
-    result = lhsLB + rhsLB)
+    result = addRoundingDown(lhsLB, rhsLB))
   or
   exists (
     AssignSubExpr assignSub, RangeSsaDefinition nextDef, float lhsLB, float rhsUB
@@ -804,7 +859,7 @@ float getDefLowerBoundsImpl(RangeSsaDefinition def, LocalScopeVariable v) {
     assignSub.getLValue() = nextDef.getAUse(v) and
     lhsLB = getDefLowerBounds(nextDef, v) and
     rhsUB = getFullyConvertedUpperBounds(assignSub.getRValue()) and
-    result = lhsLB - rhsUB)
+    result = addRoundingDown(lhsLB, -rhsUB))
   or
   exists (IncrementOperation incr, float newLB
   | def = incr and
@@ -816,7 +871,7 @@ float getDefLowerBoundsImpl(RangeSsaDefinition def, LocalScopeVariable v) {
   | def = decr and
     decr.getOperand() = v.getAnAccess() and
     newLB = getFullyConvertedLowerBounds(decr.getOperand()) and
-    result = newLB-1)
+    result = addRoundingDownSmall(newLB, -1))
   or
   // Phi nodes.
   result = getPhiLowerBounds(v, def)
@@ -839,7 +894,7 @@ float getDefUpperBoundsImpl(RangeSsaDefinition def, LocalScopeVariable v) {
     assignAdd.getLValue() = nextDef.getAUse(v) and
     lhsUB = getDefUpperBounds(nextDef, v) and
     rhsUB = getFullyConvertedUpperBounds(assignAdd.getRValue()) and
-    result = lhsUB + rhsUB)
+    result = addRoundingUp(lhsUB, rhsUB))
   or
   exists (
     AssignSubExpr assignSub, RangeSsaDefinition nextDef, float lhsUB, float rhsLB
@@ -847,13 +902,13 @@ float getDefUpperBoundsImpl(RangeSsaDefinition def, LocalScopeVariable v) {
     assignSub.getLValue() = nextDef.getAUse(v) and
     lhsUB = getDefUpperBounds(nextDef, v) and
     rhsLB = getFullyConvertedLowerBounds(assignSub.getRValue()) and
-    result = lhsUB - rhsLB)
+    result = addRoundingUp(lhsUB, -rhsLB))
   or
   exists (IncrementOperation incr, float newUB
   | def = incr and
     incr.getOperand() = v.getAnAccess() and
     newUB = getFullyConvertedUpperBounds(incr.getOperand()) and
-    result = newUB+1)
+    result = addRoundingUpSmall(newUB, 1))
   or
   exists (DecrementOperation decr, float newUB
   | def = decr and