Merge remote-tracking branch 'upstream/master' into DefaultTaintTracking-Configuration

Author: jbj

change-notes/1.24/analysis-cpp.md

@@ -20,6 +20,7 @@ The following changes in version 1.24 affect C/C++ analysis in all applications.
 | Memory may not be freed (`cpp/memory-may-not-be-freed`) | More true positive results | This query now identifies a wider variety of buffer allocations using the `semmle.code.cpp.models.interfaces.Allocation` library. |
 | Mismatching new/free or malloc/delete (`cpp/new-free-mismatch`) | Fewer false positive results | Fixed false positive results in template code. |
 | Missing return statement (`cpp/missing-return`) | Fewer false positive results | Functions containing `asm` statements are no longer highlighted by this query. |
+| Missing return statement (`cpp/missing-return`) | More accurate locations | Locations reported by this query are now more accurate in some cases. |
 | No space for zero terminator (`cpp/no-space-for-terminator`) | More correct results | String arguments to formatting functions are now (usually) expected to be null terminated strings. |
 | Hard-coded Japanese era start date (`cpp/japanese-era/exact-era-date`) |  | This query is no longer run on LGTM. |
 | No space for zero terminator (`cpp/no-space-for-terminator`) | Fewer false positive results | This query has been modified to be more conservative when identifying which pointers point to null-terminated strings.  This approach produces fewer, more accurate results. |
@@ -45,6 +46,7 @@ The following changes in version 1.24 affect C/C++ analysis in all applications.
     `StackVariableReachability`. The functionality is the same.
 * The models library models `strlen` in more detail, and includes common variations such as `wcslen`.
 * The models library models `gets` and similar functions.
+* The models library now partially models `std::string`.
 * The taint tracking library (`semmle.code.cpp.dataflow.TaintTracking`) has had
   the following improvements:
   * The library now models data flow through `strdup` and similar functions.

config/identical-files.json

@@ -242,6 +242,13 @@
     "csharp/ql/src/semmle/code/csharp/ir/implementation/raw/gvn/ValueNumbering.qll",
     "csharp/ql/src/semmle/code/csharp/ir/implementation/unaliased_ssa/gvn/ValueNumbering.qll"
   ],
+  "C++ IR PrintValueNumbering": [
+    "cpp/ql/src/semmle/code/cpp/ir/implementation/raw/gvn/PrintValueNumbering.qll",
+    "cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/gvn/PrintValueNumbering.qll",
+    "cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/gvn/PrintValueNumbering.qll",
+    "csharp/ql/src/semmle/code/csharp/ir/implementation/raw/gvn/PrintValueNumbering.qll",
+    "csharp/ql/src/semmle/code/csharp/ir/implementation/unaliased_ssa/gvn/PrintValueNumbering.qll"
+  ],
   "C++ IR ConstantAnalysis": [
     "cpp/ql/src/semmle/code/cpp/ir/implementation/raw/constant/ConstantAnalysis.qll",
     "cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/constant/ConstantAnalysis.qll",

cpp/ql/src/jsf/4.13 Functions/AV Rule 114.ql

@@ -30,7 +30,13 @@ predicate functionsMissingReturnStmt(Function f, ControlFlowNode blame) {
   ) and
   exists(ReturnStmt s |
     f.getAPredecessor() = s and
-    blame = s.getAPredecessor()
+    (
+      blame = s.getAPredecessor() and
+      count(blame.getASuccessor()) = 1
+      or
+      blame = s and
+      exists(ControlFlowNode pred | pred = s.getAPredecessor() | count(pred.getASuccessor()) != 1)
+    )
   )
 }
 

cpp/ql/src/semmle/code/cpp/exprs/Expr.qll

@@ -2,6 +2,7 @@ import semmle.code.cpp.Element
 private import semmle.code.cpp.Enclosing
 private import semmle.code.cpp.internal.ResolveClass
 private import semmle.code.cpp.internal.AddressConstantExpression
+private import semmle.code.cpp.models.implementations.Allocation
 
 /**
  * A C/C++ expression.
@@ -804,8 +805,10 @@ class NewOrNewArrayExpr extends Expr, @any_new_expr {
    * call the constructor of `T` but will not allocate memory.
    */
   Expr getPlacementPointer() {
-    isStandardPlacementNewAllocator(this.getAllocator()) and
-    result = this.getAllocatorCall().getArgument(1)
+    result =
+      this
+          .getAllocatorCall()
+          .getArgument(this.getAllocator().(OperatorNewAllocationFunction).getPlacementArgument())
   }
 }
 
@@ -1194,12 +1197,6 @@ private predicate convparents(Expr child, int idx, Element parent) {
   )
 }
 
-private predicate isStandardPlacementNewAllocator(Function operatorNew) {
-  operatorNew.getName().matches("operator new%") and
-  operatorNew.getNumberOfParameters() = 2 and
-  operatorNew.getParameter(1).getType() instanceof VoidPointerType
-}
-
 // Pulled out for performance. See QL-796.
 private predicate hasNoConversions(Expr e) { not e.hasConversion() }
 

cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/gvn/PrintValueNumbering.qll

@@ -0,0 +1,17 @@
+private import internal.ValueNumberingImports
+private import ValueNumbering
+
+/**
+ * Provides additional information about value numbering in IR dumps.
+ */
+class ValueNumberPropertyProvider extends IRPropertyProvider {
+  override string getInstructionProperty(Instruction instr, string key) {
+    exists(ValueNumber vn |
+      vn = valueNumber(instr) and
+      key = "valnum" and
+      if strictcount(vn.getAnInstruction()) > 1
+      then result = vn.getDebugString()
+      else result = "unique"
+    )
+  }
+}

cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/gvn/ValueNumbering.qll

@@ -1,21 +1,6 @@
 private import internal.ValueNumberingInternal
 private import internal.ValueNumberingImports
 
-/**
- * Provides additional information about value numbering in IR dumps.
- */
-class ValueNumberPropertyProvider extends IRPropertyProvider {
-  override string getInstructionProperty(Instruction instr, string key) {
-    exists(ValueNumber vn |
-      vn = valueNumber(instr) and
-      key = "valnum" and
-      if strictcount(vn.getAnInstruction()) > 1
-      then result = vn.getDebugString()
-      else result = "unique"
-    )
-  }
-}
-
 /**
  * The value number assigned to a particular set of instructions that produce equivalent results.
  */

cpp/ql/src/semmle/code/cpp/ir/implementation/raw/gvn/PrintValueNumbering.qll

@@ -0,0 +1,17 @@
+private import internal.ValueNumberingImports
+private import ValueNumbering
+
+/**
+ * Provides additional information about value numbering in IR dumps.
+ */
+class ValueNumberPropertyProvider extends IRPropertyProvider {
+  override string getInstructionProperty(Instruction instr, string key) {
+    exists(ValueNumber vn |
+      vn = valueNumber(instr) and
+      key = "valnum" and
+      if strictcount(vn.getAnInstruction()) > 1
+      then result = vn.getDebugString()
+      else result = "unique"
+    )
+  }
+}

cpp/ql/src/semmle/code/cpp/ir/implementation/raw/gvn/ValueNumbering.qll

@@ -1,21 +1,6 @@
 private import internal.ValueNumberingInternal
 private import internal.ValueNumberingImports
 
-/**
- * Provides additional information about value numbering in IR dumps.
- */
-class ValueNumberPropertyProvider extends IRPropertyProvider {
-  override string getInstructionProperty(Instruction instr, string key) {
-    exists(ValueNumber vn |
-      vn = valueNumber(instr) and
-      key = "valnum" and
-      if strictcount(vn.getAnInstruction()) > 1
-      then result = vn.getDebugString()
-      else result = "unique"
-    )
-  }
-}
-
 /**
  * The value number assigned to a particular set of instructions that produce equivalent results.
  */

cpp/ql/src/semmle/code/cpp/ir/implementation/raw/internal/TranslatedCall.qll

@@ -206,7 +206,44 @@ abstract class TranslatedCall extends TranslatedExpr {
 
   predicate hasPreciseSideEffect() { exists(getSideEffects()) }
 
-  TranslatedSideEffects getSideEffects() { result.getCall() = expr }
+  final TranslatedSideEffects getSideEffects() { result.getExpr() = expr }
+}
+
+abstract class TranslatedSideEffects extends TranslatedElement {
+  abstract Expr getExpr();
+
+  final override Locatable getAST() { result = getExpr() }
+
+  final override Function getFunction() { result = getExpr().getEnclosingFunction() }
+
+  override TranslatedElement getChild(int i) {
+    result =
+      rank[i + 1](TranslatedSideEffect tse, int isWrite, int index |
+        (
+          tse.getCall() = getExpr() and
+          tse.getArgumentIndex() = index and
+          if tse.isWrite() then isWrite = 1 else isWrite = 0
+        )
+      |
+        tse order by isWrite, index
+      )
+  }
+
+  final override Instruction getChildSuccessor(TranslatedElement te) {
+    exists(int i |
+      getChild(i) = te and
+      if exists(getChild(i + 1))
+      then result = getChild(i + 1).getFirstInstruction()
+      else result = getParent().getChildSuccessor(this)
+    )
+  }
+
+  /**
+   * Gets the `TranslatedFunction` containing this expression.
+   */
+  final TranslatedFunction getEnclosingFunction() {
+    result = getTranslatedFunction(getExpr().getEnclosingFunction())
+  }
 }
 
 /**
@@ -308,56 +345,27 @@ class TranslatedStructorCall extends TranslatedFunctionCall {
   override predicate hasQualifier() { any() }
 }
 
-class TranslatedSideEffects extends TranslatedElement, TTranslatedSideEffects {
-  Call expr;
-
-  TranslatedSideEffects() { this = TTranslatedSideEffects(expr) }
+class TranslatedAllocationSideEffects extends TranslatedSideEffects,
+  TTranslatedAllocationSideEffects {
+  AllocationExpr expr;
 
-  override string toString() { result = "(side effects  for " + expr.toString() + ")" }
+  TranslatedAllocationSideEffects() { this = TTranslatedAllocationSideEffects(expr) }
 
-  override Locatable getAST() { result = expr }
+  final override AllocationExpr getExpr() { result = expr }
 
-  Call getCall() { result = expr }
+  override string toString() { result = "(allocation side effects for " + expr.toString() + ")" }
 
-  override TranslatedElement getChild(int i) {
-    result =
-      rank[i + 1](TranslatedSideEffect tse, int isWrite, int index |
-        (
-          tse.getCall() = getCall() and
-          tse.getArgumentIndex() = index and
-          if tse.isWrite() then isWrite = 1 else isWrite = 0
-        )
-      |
-        tse order by isWrite, index
-      )
-  }
-
-  override Instruction getChildSuccessor(TranslatedElement te) {
-    exists(int i |
-      getChild(i) = te and
-      if exists(getChild(i + 1))
-      then result = getChild(i + 1).getFirstInstruction()
-      else result = getParent().getChildSuccessor(this)
-    )
-  }
+  override Instruction getFirstInstruction() { result = getInstruction(OnlyInstructionTag()) }
 
   override predicate hasInstruction(Opcode opcode, InstructionTag tag, CppType type) {
-    expr.getTarget() instanceof AllocationFunction and
     opcode instanceof Opcode::InitializeDynamicAllocation and
     tag = OnlyInstructionTag() and
     type = getUnknownType()
   }
 
-  override Instruction getFirstInstruction() {
-    if expr.getTarget() instanceof AllocationFunction
-    then result = getInstruction(OnlyInstructionTag())
-    else result = getChild(0).getFirstInstruction()
-  }
-
   override Instruction getInstructionSuccessor(InstructionTag tag, EdgeKind kind) {
     tag = OnlyInstructionTag() and
     kind = gotoEdge() and
-    expr.getTarget() instanceof AllocationFunction and
     if exists(getChild(0))
     then result = getChild(0).getFirstInstruction()
     else result = getParent().getChildSuccessor(this)
@@ -371,23 +379,34 @@ class TranslatedSideEffects extends TranslatedElement, TTranslatedSideEffects {
 
   override Instruction getPrimaryInstructionForSideEffect(InstructionTag tag) {
     tag = OnlyInstructionTag() and
-    result = getTranslatedExpr(expr).getInstruction(CallTag())
+    if expr instanceof NewOrNewArrayExpr
+    then result = getTranslatedAllocatorCall(expr).getInstruction(CallTag())
+    else result = getTranslatedExpr(expr).getInstruction(CallTag())
   }
+}
 
-  /**
-   * Gets the `TranslatedFunction` containing this expression.
-   */
-  final TranslatedFunction getEnclosingFunction() {
-    result = getTranslatedFunction(expr.getEnclosingFunction())
-  }
+class TranslatedCallSideEffects extends TranslatedSideEffects, TTranslatedCallSideEffects {
+  Call expr;
 
-  /**
-   * Gets the `Function` containing this expression.
-   */
-  override Function getFunction() { result = expr.getEnclosingFunction() }
+  TranslatedCallSideEffects() { this = TTranslatedCallSideEffects(expr) }
+
+  override string toString() { result = "(side effects  for " + expr.toString() + ")" }
+
+  override Call getExpr() { result = expr }
+
+  override predicate hasInstruction(Opcode opcode, InstructionTag tag, CppType type) { none() }
+
+  override Instruction getFirstInstruction() { result = getChild(0).getFirstInstruction() }
+
+  override Instruction getInstructionSuccessor(InstructionTag tag, EdgeKind kind) { none() }
+
+  override Instruction getPrimaryInstructionForSideEffect(InstructionTag tag) {
+    tag = OnlyInstructionTag() and
+    result = getTranslatedExpr(expr).getInstruction(CallTag())
+  }
 }
 
-class TranslatedStructorCallSideEffects extends TranslatedSideEffects {
+class TranslatedStructorCallSideEffects extends TranslatedCallSideEffects {
   TranslatedStructorCallSideEffects() { getParent().(TranslatedStructorCall).hasQualifier() }
 
   override predicate hasInstruction(Opcode opcode, InstructionTag tag, CppType t) {

cpp/ql/src/semmle/code/cpp/ir/implementation/raw/internal/TranslatedElement.qll

@@ -442,11 +442,22 @@ newtype TTranslatedElement =
   // The declaration/initialization part of a `ConditionDeclExpr`
   TTranslatedConditionDecl(ConditionDeclExpr expr) { not ignoreExpr(expr) } or
   // The side effects of a `Call`
-  TTranslatedSideEffects(Call expr) {
-    exists(TTranslatedArgumentSideEffect(expr, _, _, _)) or
-    expr instanceof ConstructorCall or
-    expr.getTarget() instanceof AllocationFunction
-  } or // A precise side effect of an argument to a `Call`
+  TTranslatedCallSideEffects(Call expr) {
+    // Exclude allocations such as `malloc` (which happen to also be function calls).
+    // Both `TranslatedCallSideEffects` and `TranslatedAllocationSideEffects` generate
+    // the same side effects for its children as they both extend the `TranslatedSideEffects`
+    // class.
+    // Note: We can separate allocation side effects and call side effects into two
+    // translated elements as no call can be both a `ConstructorCall` and an `AllocationExpr`.
+    not expr instanceof AllocationExpr and
+    (
+      exists(TTranslatedArgumentSideEffect(expr, _, _, _)) or
+      expr instanceof ConstructorCall
+    )
+  } or
+  // The side effects of an allocation, i.e. `new`, `new[]` or `malloc`
+  TTranslatedAllocationSideEffects(AllocationExpr expr) or
+  // A precise side effect of an argument to a `Call`
   TTranslatedArgumentSideEffect(Call call, Expr expr, int n, boolean isWrite) {
     (
       expr = call.getArgument(n).getFullyConverted()