Merge pull request #1101 from robertbrignull/merge/rc/1.20

Merge rc/1.20 => master

Author: jbj

change-notes/1.20/analysis-cpp.md

@@ -8,38 +8,39 @@
 
 | **Query**                   | **Tags**  | **Purpose**                                                        |
 |-----------------------------|-----------|--------------------------------------------------------------------|
-| Use of string copy function in a condition (`cpp/string-copy-return-value-as-boolean`) | correctness | This query identifies calls to string copy functions used in conditions, where it's likely that a different function was intended to be called. |
-| Lossy function result cast (`cpp/lossy-function-result-cast`) | correctness | Finds function calls whose result type is a floating point type, which are implicitly cast to an integral type.  Newly available but not displayed by default on LGTM. |
 | Array argument size mismatch (`cpp/array-arg-size-mismatch`) | reliability | Finds function calls where the size of an array being passed is smaller than the array size of the declared parameter.  Newly displayed on LGTM. |
-| Returning stack-allocated memory (`cpp/return-stack-allocated-memory`) | reliability, external/cwe/cwe-825 | Finds functions that may return a pointer or reference to stack-allocated memory. This query existed already but has been rewritten from scratch to make the error rate low enough for use on LGTM. Displayed by default. |
+| Lossy function result cast (`cpp/lossy-function-result-cast`) | correctness | Finds function calls whose result type is a floating point type, which are implicitly cast to an integral type.  Newly available on LGTM but results not displayed by default. |
+| Returning stack-allocated memory (`cpp/return-stack-allocated-memory`) | reliability, external/cwe/cwe-825 | Finds functions that may return a pointer or reference to stack-allocated memory. This query existed already but has been rewritten from scratch to make the error rate low enough for use on LGTM. Results displayed by default. |
+| Use of string copy function in a condition (`cpp/string-copy-return-value-as-boolean`) | correctness | This query identifies calls to string copy functions used in conditions, where it's likely that a different function was intended to be called. Results are displayed by default on LGTM. |
 
 ## Changes to existing queries
 
 | **Query**                  | **Expected impact**    | **Change**                                                       |
 |----------------------------|------------------------|------------------------------------------------------------------|
-| Array argument size mismatch (`cpp/array-arg-size-mismatch`) | Fewer false positives | An exception has been added to this query for variable sized arrays. |
+| Array argument size mismatch (`cpp/array-arg-size-mismatch`) | Fewer false positive results | An exception has been added to this query for variable sized arrays. |
 | Call to memory access function may overflow buffer (`cpp/overflow-buffer`) | More correct results | This query now recognizes calls to `RtlCopyMemoryNonTemporal` and `RtlSecureZeroMemory`. |
-| Returning stack-allocated memory (`cpp/return-stack-allocated-memory`) | More correct results | Many more stack allocated expressions are now recognized. |
-| Suspicious add with sizeof (`cpp/suspicious-add-sizeof`) | Fewer false positives | Pointer arithmetic on `char * const` expressions (and other variations of `char *`) are now correctly excluded from the results. |
-| Suspicious pointer scaling (`cpp/suspicious-pointer-scaling`) | Fewer false positives | False positives involving types that are not uniquely named in the snapshot have been fixed. |
 | Call to memory access function may overflow buffer (`cpp/overflow-buffer`) | More correct results | Calls to `fread` are now examined by this query. |
 | Lossy function result cast (`cpp/lossy-function-result-cast`) | Fewer false positive results | The whitelist of rounding functions built into this query has been expanded. |
 | Memory is never freed (`cpp/memory-never-freed`) | More correct results | Support for more Microsoft-specific memory allocation/de-allocation functions has been added. |
 | Memory may not be freed (`cpp/memory-may-not-be-freed`) | More correct results | Support for more Microsoft-specific memory allocation/de-allocation functions has been added. |
-| Unused static variable (`cpp/unused-static-variable`) | Fewer false positive results | Variables with the attribute `unused` are now excluded from the query. |
-| Resource not released in destructor (`cpp/resource-not-released-in-destructor`) | Fewer false positive results | Fix false positives where a resource is released via a virtual method call, function pointer, or lambda. |
+| Mismatching new/free or malloc/delete (`cpp/new-free-mismatch`) | More correct results | Data flow through global variables for this query has been improved. |
 | 'new[]' array freed with 'delete' (`cpp/new-array-delete-mismatch`) | More correct results | Data flow through global variables for this query has been improved. |
 | 'new' object freed with 'delete[]' (`cpp/new-delete-array-mismatch`) | More correct results | Data flow through global variables for this query has been improved. |
-| Mismatching new/free or malloc/delete (`cpp/new-free-mismatch`) | More correct results | Data flow through global variables for this query has been improved. |
+| Potential buffer overflow (`cpp/potential-buffer-overflow`) | Deprecated | This query has been deprecated. Use Potentially overrunning write (`cpp/overrunning-write`) and Potentially overrunning write with float to string conversion (`cpp/overrunning-write-with-float`) instead. |
+| Resource not released in destructor (`cpp/resource-not-released-in-destructor`) | Fewer false positive results | The query no longer highlights code that releases a resource via a virtual method call, function pointer, or lambda. |
+| Returning stack-allocated memory (`cpp/return-stack-allocated-memory`) | More correct results | Many more stack allocated expressions are now recognized. |
+| Suspicious add with sizeof (`cpp/suspicious-add-sizeof`) | Fewer false positive results | Pointer arithmetic on `char * const` expressions (and other variations of `char *`) are now correctly excluded from the results. |
+| Suspicious pointer scaling (`cpp/suspicious-pointer-scaling`) | Fewer false positive results | False positive results involving types that are not uniquely named in the snapshot have been fixed. |
+| Unused static variable (`cpp/unused-static-variable`) | Fewer false positive results | Variables with the attribute `unused` are now excluded from the query. |
 | Use of inherently dangerous function (`cpp/potential-buffer-overflow`) | Cleaned up | This query no longer catches uses of `gets`, and has been renamed 'Potential buffer overflow'. |
 | Use of potentially dangerous function (`cpp/potentially-dangerous-function`) | More correct results | This query now catches uses of `gets`. |
-| Potential buffer overflow (`cpp/potential-buffer-overflow`) | Deprecated | This query has been deprecated. Use Potentially overrunning write (`cpp/overrunning-write`) and Potentially overrunning write with float to string conversion (`cpp/overrunning-write-with-float`) instead. |
+
 
 ## Changes to QL libraries
 
 * The `semmle.code.cpp.dataflow.DataFlow` library now supports _definition by reference_ via output parameters of known functions.
     * Data flows through `memcpy` and `memmove` by default.
-    * Custom flow into or out of arguments assigned by reference can be modelled with the new class `DataFlow::DefinitionByReferenceNode`.
+    * Custom flow into or out of arguments assigned by reference can be modeled with the new class `DataFlow::DefinitionByReferenceNode`.
     * The data flow library adds flow through library functions that are modeled in `semmle.code.cpp.models.interfaces.DataFlow`. Queries can add subclasses of `DataFlowFunction` to specify additional flow.
 * There is a new `Namespace.isInline()` predicate, which holds if the namespace was declared as `inline namespace`.
 * The `Expr.isConstant()` predicate now also holds for _address constant expressions_, which are addresses that will be constant after the program has been linked. These address constants do not have a result for `Expr.getValue()`.

config/identical-files.json

@@ -35,10 +35,10 @@
     "cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/IRVariable.qll",
     "cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/IRVariable.qll"
   ],
-  "C++ IR FunctionIR": [
-    "cpp/ql/src/semmle/code/cpp/ir/implementation/raw/FunctionIR.qll",
-    "cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/FunctionIR.qll",
-    "cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/FunctionIR.qll"
+  "C++ IR IRFunction": [
+    "cpp/ql/src/semmle/code/cpp/ir/implementation/raw/IRFunction.qll",
+    "cpp/ql/src/semmle/code/cpp/ir/implementation/unaliased_ssa/IRFunction.qll",
+    "cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/IRFunction.qll"
   ],
   "C++ IR Operand": [
     "cpp/ql/src/semmle/code/cpp/ir/implementation/raw/Operand.qll",

cpp/ql/src/Critical/NewDelete.qll

@@ -80,6 +80,7 @@ private predicate allocReaches0(Expr e, Expr alloc, string kind) {
   ) or exists(Variable v |
     // alloc via a global
     allocReachesVariable(v, alloc, kind) and
+    strictcount(VariableAccess va | va.getTarget() = v) <= 50 and // avoid very expensive cases 
     e.(VariableAccess).getTarget() = v
   )
 }

cpp/ql/src/semmle/code/cpp/dataflow/internal/FlowVar.qll

@@ -237,15 +237,8 @@ module FlowVar_internal {
 
     override VariableAccess getAnAccess() {
       exists(SubBasicBlock reached |
-        reached = getAReachedBlockVarSBB(this)
-      |
+        reached = getAReachedBlockVarSBB(this) and
         variableAccessInSBB(v, reached, result)
-        or
-        // Allow flow into a `VariableAccess` that is used as definition by
-        // reference. This flow is blocked by `getAReachedBlockVarSBB` because
-        // flow should not propagate past that.
-        result = reached.getASuccessor().(VariableAccess) and
-        blockVarDefinedByReference(result, v, _)
       )
     }
 
@@ -420,6 +413,12 @@ module FlowVar_internal {
     va.getTarget() = v and
     va = sbb.getANode() and
     not overwrite(va, _)
+    or
+    // Allow flow into a `VariableAccess` that is used as definition by
+    // reference. This flow is blocked by `getAReachedBlockVarSBB` because
+    // flow should not propagate past that.
+    va = sbb.getASuccessor().(VariableAccess) and
+    blockVarDefinedByReference(va, v, _)
   }
 
   /**

cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/IR.qll

@@ -1,4 +1,4 @@
-import FunctionIR
+import IRFunction
 import Instruction
 import IRBlock
 import IRVariable

cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/IRBlock.qll

@@ -63,8 +63,8 @@ class IRBlockBase extends TIRBlock {
     result = getInstructionCount(this)
   }
 
-  final FunctionIR getEnclosingFunctionIR() {
-    result = getFirstInstruction(this).getEnclosingFunctionIR()
+  final IRFunction getEnclosingIRFunction() {
+    result = getFirstInstruction(this).getEnclosingIRFunction()
   }
 
   final Function getEnclosingFunction() {
@@ -116,7 +116,7 @@ class IRBlock extends IRBlockBase {
    * Holds if this block is reachable from the entry point of its function
    */
   final predicate isReachableFromFunctionEntry() {
-    this = getEnclosingFunctionIR().getEntryBlock() or
+    this = getEnclosingIRFunction().getEntryBlock() or
     getAPredecessor().isReachableFromFunctionEntry()
   }
 }

…mplementation/aliased_ssa/FunctionIR.qll …mplementation/aliased_ssa/IRFunction.qllcpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/FunctionIR.qll renamed to cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/IRFunction.qll cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/FunctionIR.qll renamed to cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/IRFunction.qll

@@ -2,19 +2,19 @@ private import internal.IRInternal
 import Instruction
 import cpp
 
-private newtype TFunctionIR =
-  MkFunctionIR(Function func) {
+private newtype TIRFunction =
+  MkIRFunction(Function func) {
     Construction::functionHasIR(func)
   }
 
 /**
  * Represents the IR for a function.
  */
-class FunctionIR extends TFunctionIR {
+class IRFunction extends TIRFunction {
   Function func;
 
-  FunctionIR() {
-    this = MkFunctionIR(func)
+  IRFunction() {
+    this = MkIRFunction(func)
   }
 
   final string toString() {
@@ -40,33 +40,33 @@ class FunctionIR extends TFunctionIR {
    */
   pragma[noinline]
   final EnterFunctionInstruction getEnterFunctionInstruction() {
-    result.getEnclosingFunctionIR() = this
+    result.getEnclosingIRFunction() = this
   }
 
   /**
    * Gets the exit point for this function.
    */
   pragma[noinline]
   final ExitFunctionInstruction getExitFunctionInstruction() {
-    result.getEnclosingFunctionIR() = this
+    result.getEnclosingIRFunction() = this
   }
 
   pragma[noinline]
   final UnmodeledDefinitionInstruction getUnmodeledDefinitionInstruction() {
-    result.getEnclosingFunctionIR() = this
+    result.getEnclosingIRFunction() = this
   }
 
   pragma[noinline]
   final UnmodeledUseInstruction getUnmodeledUseInstruction() {
-    result.getEnclosingFunctionIR() = this
+    result.getEnclosingIRFunction() = this
   }
 
   /**
    * Gets the single return instruction for this function.
    */
   pragma[noinline]
   final ReturnInstruction getReturnInstruction() {
-    result.getEnclosingFunctionIR() = this
+    result.getEnclosingIRFunction() = this
   }
 
   /**
@@ -75,7 +75,7 @@ class FunctionIR extends TFunctionIR {
    */
   pragma[noinline]
   final IRReturnVariable getReturnVariable() {
-    result.getEnclosingFunctionIR() = this
+    result.getEnclosingIRFunction() = this
   }
 
   /**
@@ -90,13 +90,13 @@ class FunctionIR extends TFunctionIR {
    * Gets all instructions in this function.
    */
   final Instruction getAnInstruction() {
-    result.getEnclosingFunctionIR() = this
+    result.getEnclosingIRFunction() = this
   }
 
   /**
    * Gets all blocks in this function.
    */
   final IRBlock getABlock() {
-    result.getEnclosingFunctionIR() = this
+    result.getEnclosingIRFunction() = this
   }
 }

cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/IRVariable.qll

@@ -1,5 +1,5 @@
 private import internal.IRInternal
-import FunctionIR
+import IRFunction
 import cpp
 import semmle.code.cpp.ir.implementation.TempVariableTag
 private import semmle.code.cpp.ir.internal.IRUtilities
@@ -48,7 +48,7 @@ abstract class IRVariable extends TIRVariable {
   /**
    * Gets the IR for the function that references this variable.
    */
-  final FunctionIR getEnclosingFunctionIR() {
+  final IRFunction getEnclosingIRFunction() {
     result.getFunction() = func
   }
 

cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/Instruction.qll

@@ -1,5 +1,5 @@
 private import internal.IRInternal
-import FunctionIR
+import IRFunction
 import IRBlock
 import IRVariable
 import Operand
@@ -15,7 +15,7 @@ module InstructionSanity {
   /**
    * Holds if the instruction `instr` should be expected to have an operand
    * with operand tag `tag`. Only holds for singleton operand tags. Tags with
-   * parameters, such as `PhiOperand` and `PositionalArgumentOperand` are handled
+   * parameters, such as `PhiInputOperand` and `PositionalArgumentOperand` are handled
    * separately in `unexpectedOperand`.
    */
   private predicate expectsOperand(Instruction instr, OperandTag tag) {
@@ -87,7 +87,7 @@ module InstructionSanity {
    */
   query predicate missingPhiOperand(PhiInstruction instr, IRBlock pred) {
     pred = instr.getBlock().getAPredecessor() and
-    not exists(PhiOperand operand |
+    not exists(PhiInputOperand operand |
       operand = instr.getAnOperand() and
       operand.getPredecessorBlock() = pred
     )
@@ -153,7 +153,7 @@ module InstructionSanity {
   query predicate operandAcrossFunctions(Operand operand, Instruction instr, Instruction defInstr) {
     operand.getUseInstruction() = instr and
     operand.getDefinitionInstruction() = defInstr and
-    instr.getEnclosingFunctionIR() != defInstr.getEnclosingFunctionIR()
+    instr.getEnclosingIRFunction() != defInstr.getEnclosingIRFunction()
   }
 
   /**
@@ -174,10 +174,10 @@ module InstructionSanity {
    *
    * This check ensures we don't have too _few_ back edges.
    */
-  query predicate containsLoopOfForwardEdges(FunctionIR f) {
+  query predicate containsLoopOfForwardEdges(IRFunction f) {
     exists(IRBlock block |
       forwardEdge+(block, block) and
-      block.getEnclosingFunctionIR() = f
+      block.getEnclosingIRFunction() = f
     )
   }
 
@@ -190,7 +190,7 @@ module InstructionSanity {
    * This check ensures we don't have too _many_ back edges.
    */
   query predicate lostReachability(IRBlock block) {
-    exists(FunctionIR f, IRBlock entry |
+    exists(IRFunction f, IRBlock entry |
       entry = f.getEntryBlock() and
       entry.getASuccessor+() = block and
       not forwardEdge+(entry, block) and
@@ -373,14 +373,14 @@ class Instruction extends Construction::TInstruction {
    * Gets the function that contains this instruction.
    */
   final Function getEnclosingFunction() {
-    result = getEnclosingFunctionIR().getFunction()
+    result = getEnclosingIRFunction().getFunction()
   }
 
   /**
-   * Gets the FunctionIR object that contains the IR for this instruction.
+   * Gets the IRFunction object that contains the IR for this instruction.
    */
-  final FunctionIR getEnclosingFunctionIR() {
-    result = Construction::getInstructionEnclosingFunctionIR(this)
+  final IRFunction getEnclosingIRFunction() {
+    result = Construction::getInstructionEnclosingIRFunction(this)
   }
 
   /**
@@ -1622,15 +1622,22 @@ class PhiInstruction extends Instruction {
     result instanceof PhiMemoryAccess
   }
 
+  /**
+   * Gets all of the instruction's `PhiInputOperand`s, representing the values that flow from each predecessor block.
+   */
+  final PhiInputOperand getAnInputOperand() {
+    result = this.getAnOperand()
+  }
+
   /**
    * Gets an instruction that defines the input to one of the operands of this
    * instruction. It's possible for more than one operand to have the same
    * defining instruction, so this predicate will have the same number of
-   * results as `getAnOperand()` or fewer.
+   * results as `getAnInputOperand()` or fewer.
    */
   pragma[noinline]
-  final Instruction getAnOperandDefinitionInstruction() {
-    result = this.getAnOperand().(PhiOperand).getDefinitionInstruction()
+  final Instruction getAnInput() {
+    result = this.getAnInputOperand().getDefinitionInstruction()
   }
 }
 

cpp/ql/src/semmle/code/cpp/ir/implementation/aliased_ssa/Operand.qll

@@ -25,8 +25,8 @@ class Operand extends TOperand {
     result = getUseInstruction().getLocation()
   }
 
-  final FunctionIR getEnclosingFunctionIR() {
-    result = getUseInstruction().getEnclosingFunctionIR()
+  final IRFunction getEnclosingIRFunction() {
+    result = getUseInstruction().getEnclosingIRFunction()
   }
 
   /**
@@ -379,12 +379,12 @@ class SideEffectOperand extends TypedOperand {
 /**
  * An operand of a `PhiInstruction`.
  */
-class PhiOperand extends MemoryOperand, TPhiOperand {
+class PhiInputOperand extends MemoryOperand, TPhiOperand {
   PhiInstruction useInstr;
   Instruction defInstr;
   IRBlock predecessorBlock;
 
-  PhiOperand() {
+  PhiInputOperand() {
     this = TPhiOperand(useInstr, defInstr, predecessorBlock)
   }