Merge branch 'main' into getRubyInSync

Author: erik-krogh

.codeqlmanifest.json

@@ -1,11 +1,16 @@
-{ "provide": [ "ruby/.codeqlmanifest.json",
-                "*/ql/src/qlpack.yml",
-               "*/ql/lib/qlpack.yml",
-               "*/ql/test/qlpack.yml",
-               "cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/tainted/qlpack.yml",
-               "*/ql/examples/qlpack.yml",
-               "*/upgrades/qlpack.yml",
-               "javascript/ql/experimental/adaptivethreatmodeling/lib/qlpack.yml",
-               "javascript/ql/experimental/adaptivethreatmodeling/src/qlpack.yml",
-               "misc/legacy-support/*/qlpack.yml",
-               "misc/suite-helpers/qlpack.yml" ] }
+{
+    "provide": [
+        "*/ql/src/qlpack.yml",
+        "*/ql/lib/qlpack.yml",
+        "*/ql/test/qlpack.yml",
+        "*/ql/examples/qlpack.yml",
+        "*/upgrades/qlpack.yml",
+        "cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/tainted/qlpack.yml",
+        "javascript/ql/experimental/adaptivethreatmodeling/lib/qlpack.yml",
+        "javascript/ql/experimental/adaptivethreatmodeling/src/qlpack.yml",
+        "misc/legacy-support/*/qlpack.yml",
+        "misc/suite-helpers/qlpack.yml",
+        "ruby/ql/consistency-queries/qlpack.yml",
+        "ruby/extractor-pack/codeql-extractor.yml"
+   ]
+}

.github/workflows/ruby-qltest.yml

@@ -32,7 +32,7 @@ jobs:
       - uses: ./ruby/actions/create-extractor-pack
       - name: Run QL tests
         run: |
-          codeql test run --check-databases --check-unused-labels --check-repeated-labels --check-redefined-labels --check-use-before-definition --search-path "${{ github.workspace }}/ruby" --additional-packs "${{ github.workspace }}"  --consistency-queries ql/consistency-queries ql/test
+          codeql test run --check-databases --check-unused-labels --check-repeated-labels --check-redefined-labels --check-use-before-definition --consistency-queries ql/consistency-queries ql/test
         env:
           GITHUB_TOKEN: ${{ github.token }}
       - name: Check QL formatting

cpp/ql/lib/semmle/code/cpp/ir/dataflow/DefaultTaintTracking.qll

@@ -484,8 +484,9 @@ module TaintedWithPath {
     /** Gets the element that `pathNode` wraps, if any. */
     Element getElementFromPathNode(PathNode pathNode) {
       exists(DataFlow::Node node | node = pathNode.(WrapPathNode).inner().getNode() |
-        result = node.asExpr() or
-        result = node.asParameter()
+        result = node.asInstruction().getAST()
+        or
+        result = node.asOperand().getDef().getAST()
       )
       or
       result = pathNode.(EndpointPathNode).inner()

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

@@ -806,7 +806,7 @@ predicate simpleLocalFlowStep(Node nodeFrom, Node nodeTo) {
   simpleOperandLocalFlowStep(nodeFrom.asInstruction(), nodeTo.asOperand())
   or
   // Flow into, through, and out of store nodes
-  StoreNodeFlow::flowInto(nodeFrom, nodeTo)
+  StoreNodeFlow::flowInto(nodeFrom.asInstruction(), nodeTo)
   or
   StoreNodeFlow::flowThrough(nodeFrom, nodeTo)
   or
@@ -831,18 +831,11 @@ private predicate adjacentDefUseFlow(Node nodeFrom, Node nodeTo) {
     //Def-use flow
     Ssa::ssaFlow(nodeFrom, nodeTo)
     or
-    exists(Instruction loadAddress | loadAddress = Ssa::getSourceAddressFromNode(nodeFrom) |
-      // Use-use flow through reads
-      exists(Node address |
-        Ssa::addressFlowTC(address.asInstruction(), loadAddress) and
-        Ssa::ssaFlow(address, nodeTo)
-      )
-      or
-      // Use-use flow through stores.
-      exists(Node store |
-        Ssa::explicitWrite(_, store.asInstruction(), loadAddress) and
-        Ssa::ssaFlow(store, nodeTo)
-      )
+    // Use-use flow through stores.
+    exists(Instruction loadAddress, Node store |
+      loadAddress = Ssa::getSourceAddressFromNode(nodeFrom) and
+      Ssa::explicitWrite(_, store.asInstruction(), loadAddress) and
+      Ssa::ssaFlow(store, nodeTo)
     )
   )
 }
@@ -906,10 +899,13 @@ private module ReadNodeFlow {
   }
 }
 
-private module StoreNodeFlow {
+/**
+ * INTERNAL: Do not use.
+ */
+module StoreNodeFlow {
   /** Holds if the store node `nodeTo` should receive flow from `nodeFrom`. */
-  predicate flowInto(Node nodeFrom, StoreNode nodeTo) {
-    nodeTo.flowInto(Ssa::getDestinationAddress(nodeFrom.asInstruction()))
+  predicate flowInto(Instruction instrFrom, StoreNode nodeTo) {
+    nodeTo.flowInto(Ssa::getDestinationAddress(instrFrom))
   }
 
   /** Holds if the store node `nodeTo` should receive flow from `nodeFom`. */

cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/SsaImplCommon.qll

@@ -634,3 +634,29 @@ class UncertainWriteDefinition extends WriteDefinition {
     )
   }
 }
+
+/** Provides a set of consistency queries. */
+module Consistency {
+  abstract class RelevantDefinition extends Definition {
+    abstract predicate hasLocationInfo(
+      string filepath, int startline, int startcolumn, int endline, int endcolumn
+    );
+  }
+
+  query predicate nonUniqueDef(RelevantDefinition def, SourceVariable v, BasicBlock bb, int i) {
+    ssaDefReachesRead(v, def, bb, i) and
+    not exists(unique(Definition def0 | ssaDefReachesRead(v, def0, bb, i)))
+  }
+
+  query predicate readWithoutDef(SourceVariable v, BasicBlock bb, int i) {
+    variableRead(bb, i, v, _) and
+    not ssaDefReachesRead(v, _, bb, i)
+  }
+
+  query predicate deadDef(RelevantDefinition def, SourceVariable v) {
+    v = def.getSourceVariable() and
+    not ssaDefReachesRead(_, def, _, _) and
+    not phiHasInputFromBlock(_, def, _) and
+    not uncertainWriteDefinitionInput(_, def)
+  }
+}

cpp/ql/lib/semmle/code/cpp/ir/dataflow/internal/SsaInternals.qll

@@ -244,17 +244,6 @@ Instruction getDestinationAddress(Instruction instr) {
     ]
 }
 
-class ReferenceToInstruction extends CopyValueInstruction {
-  ReferenceToInstruction() {
-    this.getResultType() instanceof Cpp::ReferenceType and
-    not this.getUnary().getResultType() instanceof Cpp::ReferenceType
-  }
-
-  Instruction getSourceAddress() { result = getSourceAddressOperand().getDef() }
-
-  Operand getSourceAddressOperand() { result = this.getUnaryOperand() }
-}
-
 /** Gets the source address of `instr` if it is an instruction that behaves like a `LoadInstruction`. */
 Instruction getSourceAddress(Instruction instr) { result = getSourceAddressOperand(instr).getDef() }
 
@@ -266,11 +255,7 @@ Operand getSourceAddressOperand(Instruction instr) {
   result =
     [
       instr.(LoadInstruction).getSourceAddressOperand(),
-      instr.(ReadSideEffectInstruction).getArgumentOperand(),
-      // `ReferenceToInstruction` is really more of an address-of operation,
-      // but by including it in this list we break out of `flowOutOfAddressStep` at an
-      // instruction that, at the source level, looks like a use of a variable.
-      instr.(ReferenceToInstruction).getSourceAddressOperand()
+      instr.(ReadSideEffectInstruction).getArgumentOperand()
     ]
 }
 
@@ -295,10 +280,6 @@ Operand getSourceValueOperand(Instruction instr) {
   result = instr.(LoadInstruction).getSourceValueOperand()
   or
   result = instr.(ReadSideEffectInstruction).getSideEffectOperand()
-  or
-  // See the comment on the `ReferenceToInstruction` disjunct in `getSourceAddressOperand` for why
-  // this case is included.
-  result = instr.(ReferenceToInstruction).getSourceValueOperand()
 }
 
 /**
@@ -513,6 +494,64 @@ private module Cached {
       explicitWrite(false, storeNode.getStoreInstruction(), def)
     )
     or
+    // The destination of a store operation has undergone lvalue-to-rvalue conversion and is now a
+    // right-hand-side of a store operation.
+    // Find the next use of the variable in that store operation, and recursively find the load of that
+    // pointer. For example, consider this case:
+    //
+    // ```cpp
+    // int x = source();
+    // int* p = &x;
+    // sink(*p);
+    // ```
+    //
+    // if we want to find the load of the address of `x`, we see that the pointer is stored into `p`,
+    // and we then need to recursively look for the load of `p`.
+    exists(
+      Def def, StoreInstruction store, IRBlock block1, int rnk1, Use use, IRBlock block2, int rnk2
+    |
+      store = def.getInstruction() and
+      store.getSourceValueOperand() = operand and
+      def.hasRankInBlock(block1, rnk1) and
+      use.hasRankInBlock(block2, rnk2) and
+      adjacentDefRead(_, block1, rnk1, block2, rnk2)
+    |
+      // The shared SSA library has determined that `use` is the next use of the operand
+      // so we find the next load of that use (but only if there is no `PostUpdateNode`) we
+      // need to flow into first.
+      not StoreNodeFlow::flowInto(store, _) and
+      flowOutOfAddressStep(use.getOperand(), nodeTo)
+      or
+      // It may also be the case that `store` gives rise to another store step. So let's make sure that
+      // we also take those into account.
+      StoreNodeFlow::flowInto(store, nodeTo)
+    )
+    or
+    // As we find the next load of an address, we might come across another use of the same variable.
+    // In that case, we recursively find the next use of _that_ operand, and continue searching for
+    // the next load of that operand. For example, consider this case:
+    //
+    // ```cpp
+    // int x = source();
+    // use(&x);
+    // int* p = &x;
+    // sink(*p);
+    // ```
+    //
+    // The next use of `x` after its definition is `use(&x)`, but there is a later load of the address
+    // of `x` that we want to flow to. So we use the shared SSA library to find the next load.
+    not operand = getSourceAddressOperand(_) and
+    exists(Use use1, Use use2, IRBlock block1, int rnk1, IRBlock block2, int rnk2 |
+      use1.getOperand() = operand and
+      use1.hasRankInBlock(block1, rnk1) and
+      // Don't flow to the next use if this use is part of a store operation that totally
+      // overrides a variable.
+      not explicitWrite(true, _, use1.getOperand().getDef()) and
+      adjacentDefRead(_, block1, rnk1, block2, rnk2) and
+      use2.hasRankInBlock(block2, rnk2) and
+      flowOutOfAddressStep(use2.getOperand(), nodeTo)
+    )
+    or
     operand = getSourceAddressOperand(nodeTo.asInstruction())
     or
     exists(ReturnIndirectionInstruction ret |

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

@@ -20,6 +20,14 @@ private import internal.OperandInternal
 private class TStageOperand =
   TRegisterOperand or TNonSSAMemoryOperand or TPhiOperand or TChiOperand;
 
+/**
+ * A known location. Testing `loc instanceof KnownLocation` will account for non existing locations, as
+ * opposed to testing `not loc isntanceof UnknownLocation`
+ */
+private class KnownLocation extends Language::Location {
+  KnownLocation() { not this instanceof Language::UnknownLocation }
+}
+
 /**
  * An operand of an `Instruction`. The operand represents a use of the result of one instruction
  * (the defining instruction) in another instruction (the use instruction)
@@ -45,8 +53,10 @@ class Operand extends TStageOperand {
 
   /**
    * Gets the location of the source code for this operand.
+   * By default this is where the operand is used, but some subclasses may override this
+   * using `getAnyDef()` if it makes more sense.
    */
-  final Language::Location getLocation() { result = this.getUse().getLocation() }
+  Language::Location getLocation() { result = this.getUse().getLocation() }
 
   /**
    * Gets the function that contains this operand.
@@ -269,6 +279,10 @@ class RegisterOperand extends NonPhiOperand, TRegisterOperand {
 
   final override string toString() { result = tag.toString() }
 
+  // most `RegisterOperands` have a more meaningful location at the definition
+  // the only exception are specific cases of `ThisArgumentOperand`
+  override Language::Location getLocation() { result = this.getAnyDef().getLocation() }
+
   final override Instruction getAnyDef() { result = defInstr }
 
   final override Overlap getDefinitionOverlap() {
@@ -401,11 +415,19 @@ class ArgumentOperand extends RegisterOperand {
 }
 
 /**
- * An operand representing the implicit 'this' argument to a member function
+ * An operand representing the implicit `this` argument to a member function
  * call.
  */
 class ThisArgumentOperand extends ArgumentOperand {
   override ThisArgumentOperandTag tag;
+
+  // in most cases the def location makes more sense, but in some corner cases it
+  // has an unknown location: in those cases we fall back to the use location
+  override Language::Location getLocation() {
+    if this.getAnyDef().getLocation() instanceof KnownLocation
+    then result = this.getAnyDef().getLocation()
+    else result = this.getUse().getLocation()
+  }
 }
 
 /**

cpp/ql/lib/semmle/code/cpp/ir/implementation/raw/Operand.qll

@@ -20,6 +20,14 @@ private import internal.OperandInternal
 private class TStageOperand =
   TRegisterOperand or TNonSSAMemoryOperand or TPhiOperand or TChiOperand;
 
+/**
+ * A known location. Testing `loc instanceof KnownLocation` will account for non existing locations, as
+ * opposed to testing `not loc isntanceof UnknownLocation`
+ */
+private class KnownLocation extends Language::Location {
+  KnownLocation() { not this instanceof Language::UnknownLocation }
+}
+
 /**
  * An operand of an `Instruction`. The operand represents a use of the result of one instruction
  * (the defining instruction) in another instruction (the use instruction)
@@ -45,8 +53,10 @@ class Operand extends TStageOperand {
 
   /**
    * Gets the location of the source code for this operand.
+   * By default this is where the operand is used, but some subclasses may override this
+   * using `getAnyDef()` if it makes more sense.
    */
-  final Language::Location getLocation() { result = this.getUse().getLocation() }
+  Language::Location getLocation() { result = this.getUse().getLocation() }
 
   /**
    * Gets the function that contains this operand.
@@ -269,6 +279,10 @@ class RegisterOperand extends NonPhiOperand, TRegisterOperand {
 
   final override string toString() { result = tag.toString() }
 
+  // most `RegisterOperands` have a more meaningful location at the definition
+  // the only exception are specific cases of `ThisArgumentOperand`
+  override Language::Location getLocation() { result = this.getAnyDef().getLocation() }
+
   final override Instruction getAnyDef() { result = defInstr }
 
   final override Overlap getDefinitionOverlap() {
@@ -401,11 +415,19 @@ class ArgumentOperand extends RegisterOperand {
 }
 
 /**
- * An operand representing the implicit 'this' argument to a member function
+ * An operand representing the implicit `this` argument to a member function
  * call.
  */
 class ThisArgumentOperand extends ArgumentOperand {
   override ThisArgumentOperandTag tag;
+
+  // in most cases the def location makes more sense, but in some corner cases it
+  // has an unknown location: in those cases we fall back to the use location
+  override Language::Location getLocation() {
+    if this.getAnyDef().getLocation() instanceof KnownLocation
+    then result = this.getAnyDef().getLocation()
+    else result = this.getUse().getLocation()
+  }
 }
 
 /**