Merge remote-tracking branch 'upstream/master' into mergeback-2018-10-11

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

@@ -4,13 +4,13 @@
 <qhelp>
 
 <overview>
-  <p>This query indicates that an <code>HRESULT</code> is being cast to a boolean type or vice versa.</p>
-  <p>The typical success value (<code>S_OK</code>) of an <code>HRESULT</code> equals 0. However, 0 indicates failure for a boolean type.</p>
-  <p>Casting an <code>HRESULT</code> to a boolean type and then using it in a test expression will yield an incorrect result.</p>
+  <p>This query indicates that an <code>HRESULT</code> is being cast to a Boolean type or vice versa.</p>
+  <p>The typical success value (<code>S_OK</code>) of an <code>HRESULT</code> equals 0. However, 0 indicates failure for a Boolean type.</p>
+  <p>Casting an <code>HRESULT</code> to a Boolean type and then using it in a test expression will yield an incorrect result.</p>
 </overview>
 
 <recommendation>
-<p>To check if a call that returns an HRESULT succeeded use the <code>FAILED</code> macro.</p>
+<p>To check if a call that returns an <code>HRESULT</code> succeeded use the <code>FAILED</code> macro.</p>
 </recommendation>
 
 <example>

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

@@ -1,8 +1,6 @@
 /**
- * @name Cast between semantically different integer types: HRESULT to/from a Boolean type
- * @description Cast between semantically different integer types: HRESULT to/from a Boolean type.
- *              Boolean types indicate success by a non-zero value, whereas success (S_OK) in HRESULT is indicated by a value of 0. 
- *              Casting an HRESULT to/from a Boolean type and then using it in a test expression will yield an incorrect result.
+ * @name Cast between HRESULT and a Boolean type
+ * @description Casting an HRESULT to/from a Boolean type and then using it in a test expression will yield an incorrect result because success (S_OK) in HRESULT is indicated by a value of 0.
  * @kind problem
  * @id cpp/hresult-boolean-conversion
  * @problem.severity error
@@ -68,4 +66,4 @@ where exists
     )
     and not isHresultBooleanConverted(e1)
   )
-select e1, msg
+select e1, msg

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

@@ -1,4 +1,5 @@
 import cpp
+import semmle.code.cpp.dataflow.DataFlow
 
 /**
  * Holds if `sizeof(s)` occurs as part of the parameter of a dynamic
@@ -47,6 +48,7 @@ predicate memberMayBeVarSize(Class c, MemberVariable v) {
 /**
  * Get the size in bytes of the buffer pointed to by an expression (if this can be determined). 
  */
+language[monotonicAggregates]
 int getBufferSize(Expr bufferExpr, Element why) {
   exists(Variable bufferVar | bufferVar = bufferExpr.(VariableAccess).getTarget() |
     (
@@ -58,6 +60,10 @@ int getBufferSize(Expr bufferExpr, Element why) {
       // buffer is an initialized array
       //  e.g. int buffer[] = {1, 2, 3};
       why = bufferVar.getInitializer().getExpr() and
+      (
+        why instanceof AggregateLiteral or
+        why instanceof StringLiteral
+      ) and
       result = why.(Expr).getType().(ArrayType).getSize() and
       not exists(bufferVar.getType().getUnspecifiedType().(ArrayType).getSize())
     ) or exists(Class parentClass, VariableAccess parentPtr |
@@ -71,19 +77,25 @@ int getBufferSize(Expr bufferExpr, Element why) {
         bufferVar.getType().getSize() -
         parentClass.getSize()
     )
-  ) or exists(Expr def |
-    // buffer is assigned with an allocation
-    definitionUsePair(_, def, bufferExpr) and
-    exprDefinition(_, def, why) and
-    isFixedSizeAllocationExpr(why, result)
-  ) or exists(Expr def, Expr e, Element why2 |
-    // buffer is assigned with another buffer
-    definitionUsePair(_, def, bufferExpr) and
-    exprDefinition(_, def, e) and
-    result = getBufferSize(e, why2) and
-    (
+  ) or (
+    // buffer is a fixed size dynamic allocation
+    isFixedSizeAllocationExpr(bufferExpr, result) and
+    why = bufferExpr
+  ) or (
+    // dataflow (all sources must be the same size)
+    result = min(Expr def |
+      DataFlow::localFlowStep(DataFlow::exprNode(def), DataFlow::exprNode(bufferExpr)) |
+      getBufferSize(def, _)
+    ) and result = max(Expr def |
+      DataFlow::localFlowStep(DataFlow::exprNode(def), DataFlow::exprNode(bufferExpr)) |
+      getBufferSize(def, _)
+    ) and
+
+    // find reason
+    exists(Expr def |
+      DataFlow::localFlowStep(DataFlow::exprNode(def), DataFlow::exprNode(bufferExpr)) |
       why = def or
-      why = why2
+      exists(getBufferSize(def, why))
     )
   ) or exists(Type bufferType |
     // buffer is the address of a variable