Merge branch 'main' into rdmarsh2/cpp/explicit-conversion-perf

Author: Robert Marsh

.devcontainer/devcontainer.json

@@ -4,6 +4,6 @@
     "slevesque.vscode-zipexplorer"
   ],
   "settings": {
-    "codeQL.experimentalBqrsParsing": true
+    "codeQL.runningQueries.memory": 2048
   }
 }

.github/workflows/labeler.yml

@@ -0,0 +1,11 @@
+name: "Pull Request Labeler"
+on:
+- pull_request_target
+
+jobs:
+  triage:
+    runs-on: ubuntu-latest
+    steps:
+    - uses: actions/labeler@v2
+      with:
+        repo-token: "${{ secrets.GITHUB_TOKEN }}"

change-notes/1.26/analysis-cpp.md

@@ -13,6 +13,7 @@ The following changes in version 1.26 affect C/C++ analysis in all applications.
 
 | **Query**                  | **Expected impact**    | **Change**                                                       |
 |----------------------------|------------------------|------------------------------------------------------------------|
+| Declaration hides parameter (`cpp/declaration-hides-parameter`) | Fewer false positive results | False positives involving template functions have been fixed. |
 | Inconsistent direction of for loop (`cpp/inconsistent-loop-direction`) | Fewer false positive results | The query now accounts for intentional wrapping of an unsigned loop counter. |
 | Overflow in uncontrolled allocation size (`cpp/uncontrolled-allocation-size`) | | The precision of this query has been decreased from "high" to "medium". As a result, the query is still run but results are no longer displayed on LGTM by default. |
 | Comparison result is always the same (`cpp/constant-comparison`) | More correct results | Bounds on expressions involving multiplication can now be determined in more cases. |

change-notes/1.26/analysis-csharp.md

@@ -21,6 +21,10 @@ The following changes in version 1.26 affect C# analysis in all applications.
 * Partial method bodies are extracted. Previously, partial method bodies were skipped completely.
 * Inferring the lengths of implicitely sized arrays is fixed. Previously, multidimensional arrays were always extracted with the same length for
 each dimension. With the fix, the array sizes `2` and `1` are extracted for `new int[,]{{1},{2}}`. Previously `2` and `2` were extracted.
+* The extractor is now assembly-insensitive by default. This means that two entities with the same
+  fully-qualified name are now mapped to the same entity in the resulting database, regardless of
+  whether they belong to different assemblies. Assembly sensitivity can be reenabled by passing
+  `--assemblysensitivetrap` to the extractor.
 
 ## Changes to libraries
 

change-notes/1.26/analysis-javascript.md

@@ -26,9 +26,12 @@
 
 | **Query**                      | **Expected impact**          | **Change**                                                                |
 |--------------------------------|------------------------------|---------------------------------------------------------------------------|
+| Potentially unsafe external link (`js/unsafe-external-link`) | Fewer results | This query no longer flags URLs constructed using a template system where only the hash or query part of the URL is dynamic. |
 | Incomplete URL substring sanitization (`js/incomplete-url-substring-sanitization`) | More results | This query now recognizes additional URLs when the substring check is an inclusion check. |
 | Ambiguous HTML id attribute (`js/duplicate-html-id`) | Results no longer shown | Precision tag reduced to "low". The query is no longer run by default. |
 | Unused loop iteration variable (`js/unused-loop-variable`) | Fewer results | This query no longer flags variables in a destructuring array assignment that are not the last variable in the destructed array. |
+| Unsafe jQuery plugin (`js/unsafe-jquery-plugin`) | More results | This query now detects more unsafe uses of nested option properties. |
 
 
 ## Changes to libraries
+* The predicate `TypeAnnotation.hasQualifiedName` now works in more cases when the imported library was not present during extraction.

cpp/ql/src/Best Practices/Hiding/DeclarationHidesParameter.ql

@@ -11,20 +11,35 @@
 
 import cpp
 
+/**
+ * Gets the template that a function `f` is constructed from, or just `f` if it
+ * is not from a template instantiation.
+ */
+Function getConstructedFrom(Function f) {
+  f.isConstructedFrom(result)
+  or
+  not f.isConstructedFrom(_) and
+  result = f
+}
+
 /**
  * Gets the parameter of `f` with name `name`, which has to come from the
  * _definition_ of `f` and not a prototype declaration.
  * We also exclude names from functions that have multiple definitions.
  * This should not happen in a single application but since we
  * have a system wide view it is likely to happen for instance for
  * the main function.
+ *
+ * Note: we use `getConstructedFrom` to ensure that we look at template
+ * functions rather than their instantiations. We get better results this way
+ * as the instantiation is artificial and may have inherited parameter names
+ * from the declaration rather than the definition.
  */
 ParameterDeclarationEntry functionParameterNames(Function f, string name) {
   exists(FunctionDeclarationEntry fe |
     result.getFunctionDeclarationEntry() = fe and
-    fe.getFunction() = f and
+    getConstructedFrom(f).getDefinition() = fe and
     fe.getLocation() = f.getDefinitionLocation() and
-    result.getFile() = fe.getFile() and // Work around CPP-331
     strictcount(f.getDefinitionLocation()) = 1 and
     result.getName() = name
   )

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

@@ -45,6 +45,16 @@ predicate dereferenceThis(Expr e) {
   or
   // `*this = ...` (where `=` is not overloaded, so an `AssignExpr`)
   dereferenceThis(e.(AssignExpr).getLValue())
+  or
+  // `e ? ... : ... `
+  exists(ConditionalExpr cond |
+    cond = e and
+    dereferenceThis(cond.getThen()) and
+    dereferenceThis(cond.getElse())
+  )
+  or
+  // `..., ... `
+  dereferenceThis(e.(CommaExpr).getRightOperand())
 }
 
 /**

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

@@ -1275,3 +1275,31 @@ class SpaceshipExpr extends BinaryOperation, @spaceshipexpr {
 
   override string getOperator() { result = "<=>" }
 }
+
+/**
+ * A C/C++ `co_await` expression.
+ * ```
+ * co_await foo();
+ * ```
+ */
+class CoAwaitExpr extends UnaryOperation, @co_await {
+  override string getAPrimaryQlClass() { result = "CoAwaitExpr" }
+
+  override string getOperator() { result = "co_await" }
+
+  override int getPrecedence() { result = 16 }
+}
+
+/**
+ * A C/C++ `co_yield` expression.
+ * ```
+ * co_yield 1;
+ * ```
+ */
+class CoYieldExpr extends UnaryOperation, @co_yield {
+  override string getAPrimaryQlClass() { result = "CoYieldExpr" }
+
+  override string getOperator() { result = "co_yield" }
+
+  override int getPrecedence() { result = 2 }
+}

cpp/ql/src/semmle/code/cpp/ir/dataflow/internal/DataFlowUtil.qll

@@ -11,6 +11,7 @@ private import semmle.code.cpp.ir.IR
 private import semmle.code.cpp.controlflow.IRGuards
 private import semmle.code.cpp.models.interfaces.DataFlow
 
+cached
 private newtype TIRDataFlowNode =
   TInstructionNode(Instruction i) or
   TOperandNode(Operand op) or
@@ -533,11 +534,11 @@ predicate localFlowStep(Node nodeFrom, Node nodeTo) { simpleLocalFlowStep(nodeFr
  * data flow. It may have less flow than the `localFlowStep` predicate.
  */
 predicate simpleLocalFlowStep(Node nodeFrom, Node nodeTo) {
-  // Instruction -> Instruction flow
-  simpleInstructionLocalFlowStep(nodeFrom.asInstruction(), nodeTo.asInstruction())
-  or
   // Operand -> Instruction flow
-  simpleOperandLocalFlowStep(nodeFrom.asOperand(), nodeTo.asInstruction())
+  simpleInstructionLocalFlowStep(nodeFrom.asOperand(), nodeTo.asInstruction())
+  or
+  // Instruction -> Operand flow
+  simpleOperandLocalFlowStep(nodeFrom.asInstruction(), nodeTo.asOperand())
 }
 
 pragma[noinline]
@@ -549,26 +550,20 @@ private predicate getFieldSizeOfClass(Class c, Type type, int size) {
   )
 }
 
-private predicate simpleOperandLocalFlowStep(Operand opFrom, Instruction iTo) {
-  // Certain dataflow steps (for instance `PostUpdateNode.getPreUpdateNode()`) generates flow to
-  // operands, so we include dataflow from those operands to the "result" of the instruction (i.e., to
-  // the instruction itself).
-  exists(PostUpdateNode post |
-    opFrom = post.getPreUpdateNode().asOperand() and
-    iTo.getAnOperand() = opFrom
+private predicate isSingleFieldClass(Type type, Class cTo) {
+  exists(int size |
+    cTo.getSize() = size and
+    getFieldSizeOfClass(cTo, type, size)
   )
 }
 
-cached
-private predicate simpleInstructionLocalFlowStep(Instruction iFrom, Instruction iTo) {
-  iTo.(CopyInstruction).getSourceValue() = iFrom
+private predicate simpleOperandLocalFlowStep(Instruction iFrom, Operand opTo) {
+  // Propagate flow from an instruction to its exact uses.
+  opTo.getDef() = iFrom
   or
-  iTo.(PhiInstruction).getAnOperand().getDef() = iFrom
-  or
-  // A read side effect is almost never exact since we don't know exactly how
-  // much memory the callee will read.
-  iTo.(ReadSideEffectInstruction).getSideEffectOperand().getAnyDef() = iFrom and
-  not iFrom.isResultConflated()
+  opTo = any(ReadSideEffectInstruction read).getSideEffectOperand() and
+  not iFrom.isResultConflated() and
+  iFrom = opTo.getAnyDef()
   or
   // Loading a single `int` from an `int *` parameter is not an exact load since
   // the parameter may point to an entire array rather than a single `int`. The
@@ -582,20 +577,38 @@ private predicate simpleInstructionLocalFlowStep(Instruction iFrom, Instruction
   // leads to a phi node.
   exists(InitializeIndirectionInstruction init |
     iFrom = init and
-    iTo.(LoadInstruction).getSourceValueOperand().getAnyDef() = init and
+    opTo.(LoadOperand).getAnyDef() = init and
     // Check that the types match. Otherwise we can get flow from an object to
     // its fields, which leads to field conflation when there's flow from other
     // fields to the object elsewhere.
     init.getParameter().getType().getUnspecifiedType().(DerivedType).getBaseType() =
-      iTo.getResultType().getUnspecifiedType()
+      opTo.getType().getUnspecifiedType()
+  )
+  or
+  // Flow from stores to structs with a single field to a load of that field.
+  exists(LoadInstruction load |
+    load.getSourceValueOperand() = opTo and
+    opTo.getAnyDef() = iFrom and
+    isSingleFieldClass(iFrom.getResultType(), opTo.getType())
   )
+}
+
+cached
+private predicate simpleInstructionLocalFlowStep(Operand opFrom, Instruction iTo) {
+  iTo.(CopyInstruction).getSourceValueOperand() = opFrom
+  or
+  iTo.(PhiInstruction).getAnInputOperand() = opFrom
+  or
+  // A read side effect is almost never exact since we don't know exactly how
+  // much memory the callee will read.
+  iTo.(ReadSideEffectInstruction).getSideEffectOperand() = opFrom
   or
   // Treat all conversions as flow, even conversions between different numeric types.
-  iTo.(ConvertInstruction).getUnary() = iFrom
+  iTo.(ConvertInstruction).getUnaryOperand() = opFrom
   or
-  iTo.(CheckedConvertOrNullInstruction).getUnary() = iFrom
+  iTo.(CheckedConvertOrNullInstruction).getUnaryOperand() = opFrom
   or
-  iTo.(InheritanceConversionInstruction).getUnary() = iFrom
+  iTo.(InheritanceConversionInstruction).getUnaryOperand() = opFrom
   or
   // A chi instruction represents a point where a new value (the _partial_
   // operand) may overwrite an old value (the _total_ operand), but the alias
@@ -608,7 +621,7 @@ private predicate simpleInstructionLocalFlowStep(Instruction iFrom, Instruction
   //
   // Flow through the partial operand belongs in the taint-tracking libraries
   // for now.
-  iTo.getAnOperand().(ChiTotalOperand).getDef() = iFrom
+  iTo.getAnOperand().(ChiTotalOperand) = opFrom
   or
   // Add flow from write side-effects to non-conflated chi instructions through their
   // partial operands. From there, a `readStep` will find subsequent reads of that field.
@@ -623,24 +636,16 @@ private predicate simpleInstructionLocalFlowStep(Instruction iFrom, Instruction
   // Here, a `WriteSideEffectInstruction` will provide a new definition for `p->x` after the call to
   // `setX`, which will be melded into `p` through a chi instruction.
   exists(ChiInstruction chi | chi = iTo |
-    chi.getPartialOperand().getDef() = iFrom.(WriteSideEffectInstruction) and
+    opFrom.getAnyDef() instanceof WriteSideEffectInstruction and
+    chi.getPartialOperand() = opFrom and
     not chi.isResultConflated()
   )
   or
-  // Flow from stores to structs with a single field to a load of that field.
-  iTo.(LoadInstruction).getSourceValueOperand().getAnyDef() = iFrom and
-  exists(int size, Type type, Class cTo |
-    type = iFrom.getResultType() and
-    cTo = iTo.getResultType() and
-    cTo.getSize() = size and
-    getFieldSizeOfClass(cTo, type, size)
-  )
-  or
   // Flow through modeled functions
-  modelFlow(iFrom, iTo)
+  modelFlow(opFrom, iTo)
 }
 
-private predicate modelFlow(Instruction iFrom, Instruction iTo) {
+private predicate modelFlow(Operand opFrom, Instruction iTo) {
   exists(
     CallInstruction call, DataFlowFunction func, FunctionInput modelIn, FunctionOutput modelOut
   |
@@ -665,17 +670,17 @@ private predicate modelFlow(Instruction iFrom, Instruction iTo) {
     (
       exists(int index |
         modelIn.isParameter(index) and
-        iFrom = call.getPositionalArgument(index)
+        opFrom = call.getPositionalArgumentOperand(index)
       )
       or
       exists(int index, ReadSideEffectInstruction read |
         modelIn.isParameterDeref(index) and
         read = getSideEffectFor(call, index) and
-        iFrom = read.getSideEffectOperand().getAnyDef()
+        opFrom = read.getSideEffectOperand()
       )
       or
       modelIn.isQualifierAddress() and
-      iFrom = call.getThisArgument()
+      opFrom = call.getThisArgumentOperand()
       // TODO: add read side effects for qualifiers
     )
   )

cpp/ql/src/semmle/code/cpp/models/implementations/StdString.qll

@@ -1,6 +1,6 @@
 /**
  * Provides implementation classes modeling `std::string` and other
- * instantiations of`std::basic_string`. See `semmle.code.cpp.models.Models`
+ * instantiations of `std::basic_string`. See `semmle.code.cpp.models.Models`
  * for usage information.
  */
 
@@ -82,6 +82,32 @@ class StdStringData extends TaintFunction {
   }
 }
 
+/**
+ * The `std::string` function `push_back`.
+ */
+class StdStringPush extends TaintFunction {
+  StdStringPush() { this.hasQualifiedName("std", "basic_string", "push_back") }
+
+  override predicate hasTaintFlow(FunctionInput input, FunctionOutput output) {
+    // flow from parameter to qualifier
+    input.isParameterDeref(0) and
+    output.isQualifierObject()
+  }
+}
+
+/**
+ * The `std::string` functions `front` and `back`.
+ */
+class StdStringFrontBack extends TaintFunction {
+  StdStringFrontBack() { this.hasQualifiedName("std", "basic_string", ["front", "back"]) }
+
+  override predicate hasTaintFlow(FunctionInput input, FunctionOutput output) {
+    // flow from object to returned reference
+    input.isQualifierObject() and
+    output.isReturnValueDeref()
+  }
+}
+
 /**
  * The `std::string` function `operator+`.
  */
@@ -138,6 +164,11 @@ class StdStringAppend extends TaintFunction {
       output.isQualifierObject() or
       output.isReturnValueDeref()
     )
+    or
+    // reverse flow from returned reference to the qualifier (for writes to
+    // the result)
+    input.isReturnValueDeref() and
+    output.isQualifierObject()
   }
 }
 
@@ -173,6 +204,11 @@ class StdStringAssign extends TaintFunction {
       output.isQualifierObject() or
       output.isReturnValueDeref()
     )
+    or
+    // reverse flow from returned reference to the qualifier (for writes to
+    // the result)
+    input.isReturnValueDeref() and
+    output.isQualifierObject()
   }
 }