Merge pull request #7551 from MathiasVP/fix-join-orders-in-unsigned-difference-expr-query

C++: Fix join orders in `cpp/unsigned-difference-expression-compared-zero`

.codeqlmanifest.json

@@ -4,12 +4,10 @@
         "*/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",
-        "csharp/ql/campaigns/Solorigate/lib/qlpack.yml",
-        "csharp/ql/campaigns/Solorigate/src/qlpack.yml",
-        "csharp/ql/campaigns/Solorigate/test/qlpack.yml",
         "misc/legacy-support/*/qlpack.yml",
         "misc/suite-helpers/qlpack.yml",
         "ruby/extractor-pack/codeql-extractor.yml",

README.md

@@ -1,11 +1,11 @@
 # CodeQL
 
-This open source repository contains the standard CodeQL libraries and queries that power [GitHub Advanced Security](https://github.com/features/security/code) and the other application security products that [GitHub](https://github.com/features/security/) makes available to its customers worldwide. For the queries, libraries, and extractor that power Go analysis, visit the [CodeQL for Go repository](https://github.com/github/codeql-go).
+This open source repository contains the standard CodeQL libraries and queries that power [LGTM](https://lgtm.com) and the other CodeQL products that [GitHub](https://github.com) makes available to its customers worldwide. For the queries, libraries, and extractor that power Go analysis, visit the [CodeQL for Go repository](https://github.com/github/codeql-go).
 
 ## How do I learn CodeQL and run queries?
 
 There is [extensive documentation](https://codeql.github.com/docs/) on getting started with writing CodeQL.
-You can use the [CodeQL for Visual Studio Code](https://codeql.github.com/docs/codeql-for-visual-studio-code/) extension or the [interactive query console](https://lgtm.com/help/lgtm/using-query-console) on LGTM.com (Semmle Legacy product) to try out your queries on any open source project that's currently being analyzed.
+You can use the [interactive query console](https://lgtm.com/help/lgtm/using-query-console) on LGTM.com or the [CodeQL for Visual Studio Code](https://codeql.github.com/docs/codeql-for-visual-studio-code/) extension to try out your queries on any open source project that's currently being analyzed.
 
 ## Contributing
 
@@ -13,7 +13,7 @@ We welcome contributions to our standard library and standard checks. Do you hav
 
 ## License
 
-The code in this repository is licensed under the [MIT License](LICENSE) by [GitHub](https://github.com). The use of CodeQL on open source code is licensed under specific [Terms & Conditions](https://securitylab.github.com/tools/codeql/license/) UNLESS you have a commercial license in place. If you'd like to use CodeQL with a commercial codebase, please [contact us](https://github.com/enterprise/contact) for further help.
+The code in this repository is licensed under the [MIT License](LICENSE) by [GitHub](https://github.com).
 
 ## Visual Studio Code integration
 

cpp/ql/lib/CHANGELOG.md

@@ -1,3 +1,5 @@
+## 0.0.6
+
 ## 0.0.5
 
 ## 0.0.4

cpp/ql/lib/change-notes/released/0.0.6.md

@@ -0,0 +1 @@
+## 0.0.6

cpp/ql/lib/codeql-pack.release.yml

@@ -1,2 +1,2 @@
 ---
-lastReleaseVersion: 0.0.5
+lastReleaseVersion: 0.0.6

cpp/ql/lib/qlpack.yml

@@ -1,7 +1,8 @@
 name: codeql/cpp-all
-version: 0.0.6-dev
+version: 0.0.6
 groups: cpp
 dbscheme: semmlecode.cpp.dbscheme
 extractor: cpp
 library: true
-upgrades: upgrades
+dependencies:
+  codeql/cpp-upgrades: ^0.0.3

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

@@ -63,7 +63,7 @@ private module VirtualDispatch {
         this.flowsFrom(other, allowOtherFromArg)
       |
         // Call argument
-        exists(DataFlowCall call, Position i |
+        exists(DataFlowCall call, int i |
           other
               .(DataFlow::ParameterNode)
               .isParameterOf(pragma[only_bind_into](call).getStaticCallTarget(), i) and
@@ -268,6 +268,16 @@ Function viableImplInCallContext(CallInstruction call, CallInstruction ctx) {
   )
 }
 
+/** A parameter position represented by an integer. */
+class ParameterPosition extends int {
+  ParameterPosition() { any(ParameterNode p).isParameterOf(_, this) }
+}
+
+/** An argument position represented by an integer. */
+class ArgumentPosition extends int {
+  ArgumentPosition() { any(ArgumentNode a).argumentOf(_, this) }
+}
+
 /** Holds if arguments at position `apos` match parameters at position `ppos`. */
 pragma[inline]
 predicate parameterMatch(ParameterPosition ppos, ArgumentPosition apos) { ppos = apos }

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

@@ -27,7 +27,7 @@ abstract class ArgumentNode extends OperandNode {
    * Holds if this argument occurs at the given position in the given call.
    * The instance argument is considered to have index `-1`.
    */
-  abstract predicate argumentOf(DataFlowCall call, ArgumentPosition pos);
+  abstract predicate argumentOf(DataFlowCall call, int pos);
 
   /** Gets the call in which this node is an argument. */
   DataFlowCall getCall() { this.argumentOf(result, _) }
@@ -42,9 +42,7 @@ private class PrimaryArgumentNode extends ArgumentNode {
 
   PrimaryArgumentNode() { exists(CallInstruction call | op = call.getAnArgumentOperand()) }
 
-  override predicate argumentOf(DataFlowCall call, ArgumentPosition pos) {
-    op = call.getArgumentOperand(pos.(DirectPosition).getIndex())
-  }
+  override predicate argumentOf(DataFlowCall call, int pos) { op = call.getArgumentOperand(pos) }
 
   override string toString() {
     exists(Expr unconverted |
@@ -73,9 +71,9 @@ private class SideEffectArgumentNode extends ArgumentNode {
 
   SideEffectArgumentNode() { op = read.getSideEffectOperand() }
 
-  override predicate argumentOf(DataFlowCall call, ArgumentPosition pos) {
+  override predicate argumentOf(DataFlowCall call, int pos) {
     read.getPrimaryInstruction() = call and
-    pos.(IndirectionPosition).getIndex() = read.getIndex()
+    pos = getArgumentPosOfSideEffect(read.getIndex())
   }
 
   override string toString() {
@@ -92,54 +90,6 @@ private class SideEffectArgumentNode extends ArgumentNode {
   }
 }
 
-/** A parameter position represented by an integer. */
-class ParameterPosition = Position;
-
-/** An argument position represented by an integer. */
-class ArgumentPosition = Position;
-
-class Position extends TPosition {
-  abstract string toString();
-}
-
-class DirectPosition extends TDirectPosition {
-  int index;
-
-  DirectPosition() { this = TDirectPosition(index) }
-
-  string toString() {
-    index = -1 and
-    result = "this"
-    or
-    index != -1 and
-    result = index.toString()
-  }
-
-  int getIndex() { result = index }
-}
-
-class IndirectionPosition extends TIndirectionPosition {
-  int index;
-
-  IndirectionPosition() { this = TIndirectionPosition(index) }
-
-  string toString() {
-    index = -1 and
-    result = "this"
-    or
-    index != -1 and
-    result = index.toString()
-  }
-
-  int getIndex() { result = index }
-}
-
-newtype TPosition =
-  TDirectPosition(int index) { exists(any(CallInstruction c).getArgument(index)) } or
-  TIndirectionPosition(int index) {
-    exists(ReadSideEffectInstruction instr | instr.getIndex() = index)
-  }
-
 private newtype TReturnKind =
   TNormalReturnKind() or
   TIndirectReturnKind(ParameterIndex index)

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

@@ -490,6 +490,19 @@ class ExprNode extends InstructionNode {
   override string toString() { result = this.asConvertedExpr().toString() }
 }
 
+/**
+ * INTERNAL: do not use. Translates a parameter/argument index into a negative
+ * number that denotes the index of its side effect (pointer indirection).
+ */
+bindingset[index]
+int getArgumentPosOfSideEffect(int index) {
+  // -1 -> -2
+  //  0 -> -3
+  //  1 -> -4
+  // ...
+  result = -3 - index
+}
+
 /**
  * The value of a parameter at function entry, viewed as a node in a data
  * flow graph. This includes both explicit parameters such as `x` in `f(x)`
@@ -512,7 +525,7 @@ class ParameterNode extends InstructionNode {
    * implicit `this` parameter is considered to have position `-1`, and
    * pointer-indirection parameters are at further negative positions.
    */
-  predicate isParameterOf(Function f, ParameterPosition pos) { none() } // overridden by subclasses
+  predicate isParameterOf(Function f, int pos) { none() } // overridden by subclasses
 }
 
 /** An explicit positional parameter, not including `this` or `...`. */
@@ -521,8 +534,8 @@ private class ExplicitParameterNode extends ParameterNode {
 
   ExplicitParameterNode() { exists(instr.getParameter()) }
 
-  override predicate isParameterOf(Function f, ParameterPosition pos) {
-    f.getParameter(pos.(DirectPosition).getIndex()) = instr.getParameter()
+  override predicate isParameterOf(Function f, int pos) {
+    f.getParameter(pos) = instr.getParameter()
   }
 
   /** Gets the `Parameter` associated with this node. */
@@ -537,8 +550,8 @@ class ThisParameterNode extends ParameterNode {
 
   ThisParameterNode() { instr.getIRVariable() instanceof IRThisVariable }
 
-  override predicate isParameterOf(Function f, ParameterPosition pos) {
-    pos.(DirectPosition).getIndex() = -1 and instr.getEnclosingFunction() = f
+  override predicate isParameterOf(Function f, int pos) {
+    pos = -1 and instr.getEnclosingFunction() = f
   }
 
   override string toString() { result = "this" }
@@ -548,12 +561,12 @@ class ThisParameterNode extends ParameterNode {
 class ParameterIndirectionNode extends ParameterNode {
   override InitializeIndirectionInstruction instr;
 
-  override predicate isParameterOf(Function f, ParameterPosition pos) {
+  override predicate isParameterOf(Function f, int pos) {
     exists(int index |
       instr.getEnclosingFunction() = f and
       instr.hasIndex(index)
     |
-      pos.(IndirectionPosition).getIndex() = index
+      pos = getArgumentPosOfSideEffect(index)
     )
   }
 

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

@@ -659,15 +659,4 @@ module Consistency {
     not phiHasInputFromBlock(_, def, _) and
     not uncertainWriteDefinitionInput(_, def)
   }
-
-  query predicate notDominatedByDef(RelevantDefinition def, SourceVariable v, BasicBlock bb, int i) {
-    exists(BasicBlock bbDef, int iDef | def.definesAt(v, bbDef, iDef) |
-      ssaDefReachesReadWithinBlock(v, def, bb, i) and
-      (bb != bbDef or i < iDef)
-      or
-      ssaDefReachesRead(v, def, bb, i) and
-      not ssaDefReachesReadWithinBlock(v, def, bb, i) and
-      not def.definesAt(v, getImmediateBasicBlockDominator*(bb), _)
-    )
-  }
 }

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

@@ -51,6 +51,16 @@ private newtype TDefOrUse =
   TExplicitUse(Operand op) { isExplicitUse(op) } or
   TReturnParamIndirection(Operand op) { returnParameterIndirection(op, _) }
 
+pragma[nomagic]
+private int getRank(DefOrUse defOrUse, IRBlock block) {
+  defOrUse =
+    rank[result](int i, DefOrUse cand |
+      block.getInstruction(i) = toInstruction(cand)
+    |
+      cand order by i
+    )
+}
+
 private class DefOrUse extends TDefOrUse {
   /** Gets the instruction associated with this definition, if any. */
   Instruction asDef() { none() }
@@ -64,10 +74,9 @@ private class DefOrUse extends TDefOrUse {
   /** Gets the block of this definition or use. */
   abstract IRBlock getBlock();
 
-  /** Holds if this definition or use has index `index` in block `block`. */
-  final predicate hasIndexInBlock(IRBlock block, int index) {
-    block.getInstruction(index) = toInstruction(this)
-  }
+  /** Holds if this definition or use has rank `rank` in block `block`. */
+  cached
+  final predicate hasRankInBlock(IRBlock block, int rnk) { rnk = getRank(this, block) }
 
   /** Gets the location of this element. */
   abstract Cpp::Location getLocation();
@@ -304,8 +313,8 @@ cached
 private module Cached {
   private predicate defUseFlow(Node nodeFrom, Node nodeTo) {
     exists(IRBlock bb1, int i1, IRBlock bb2, int i2, DefOrUse defOrUse, Use use |
-      defOrUse.hasIndexInBlock(bb1, i1) and
-      use.hasIndexInBlock(bb2, i2) and
+      defOrUse.hasRankInBlock(bb1, i1) and
+      use.hasRankInBlock(bb2, i2) and
       adjacentDefRead(_, bb1, i1, bb2, i2) and
       nodeFrom.asInstruction() = toInstruction(defOrUse) and
       flowOutOfAddressStep(use.getOperand(), nodeTo)
@@ -317,9 +326,9 @@ private module Cached {
     exists(IRBlock bb1, int i1, IRBlock bb2, int i2, Def def, Use use |
       nodeFrom.isTerminal() and
       def.getInstruction() = nodeFrom.getStoreInstruction() and
-      def.hasIndexInBlock(bb1, i1) and
+      def.hasRankInBlock(bb1, i1) and
       adjacentDefRead(_, bb1, i1, bb2, i2) and
-      use.hasIndexInBlock(bb2, i2) and
+      use.hasRankInBlock(bb2, i2) and
       flowOutOfAddressStep(use.getOperand(), nodeTo)
     )
     or
@@ -350,8 +359,8 @@ private module Cached {
 
   private predicate fromReadNode(ReadNode nodeFrom, Node nodeTo) {
     exists(IRBlock bb1, int i1, IRBlock bb2, int i2, Use use1, Use use2 |
-      use1.hasIndexInBlock(bb1, i1) and
-      use2.hasIndexInBlock(bb2, i2) and
+      use1.hasRankInBlock(bb1, i1) and
+      use2.hasRankInBlock(bb2, i2) and
       use1.getOperand().getDef() = nodeFrom.getInstruction() and
       adjacentDefRead(_, bb1, i1, bb2, i2) and
       flowOutOfAddressStep(use2.getOperand(), nodeTo)
@@ -362,7 +371,7 @@ private module Cached {
     exists(PhiNode phi, Use use, IRBlock block, int rnk |
       phi = nodeFrom.getPhiNode() and
       adjacentDefRead(phi, _, _, block, rnk) and
-      use.hasIndexInBlock(block, rnk) and
+      use.hasRankInBlock(block, rnk) and
       flowOutOfAddressStep(use.getOperand(), nodeTo)
     )
   }
@@ -370,7 +379,7 @@ private module Cached {
   private predicate toPhiNode(Node nodeFrom, SsaPhiNode nodeTo) {
     // Flow to phi nodes
     exists(Def def, IRBlock block, int rnk |
-      def.hasIndexInBlock(block, rnk) and
+      def.hasRankInBlock(block, rnk) and
       nodeTo.hasInputAtRankInBlock(block, rnk)
     |
       exists(StoreNodeInstr storeNode |
@@ -503,8 +512,8 @@ private module Cached {
     |
       store = def.getInstruction() and
       store.getSourceValueOperand() = operand and
-      def.hasIndexInBlock(block1, rnk1) and
-      use.hasIndexInBlock(block2, rnk2) 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
@@ -534,12 +543,12 @@ private module Cached {
     not operand = getSourceAddressOperand(_) and
     exists(Use use1, Use use2, IRBlock block1, int rnk1, IRBlock block2, int rnk2 |
       use1.getOperand() = operand and
-      use1.hasIndexInBlock(block1, rnk1) 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.hasIndexInBlock(block2, rnk2) and
+      use2.hasRankInBlock(block2, rnk2) and
       flowOutOfAddressStep(use2.getOperand(), nodeTo)
     )
     or
@@ -611,7 +620,7 @@ import Cached
 predicate variableWrite(IRBlock bb, int i, SourceVariable v, boolean certain) {
   DataFlowImplCommon::forceCachingInSameStage() and
   exists(Def def |
-    def.hasIndexInBlock(bb, i) and
+    def.hasRankInBlock(bb, i) and
     v = def.getSourceVariable() and
     (if def.isCertain() then certain = true else certain = false)
   )
@@ -623,7 +632,7 @@ predicate variableWrite(IRBlock bb, int i, SourceVariable v, boolean certain) {
  */
 predicate variableRead(IRBlock bb, int i, SourceVariable v, boolean certain) {
   exists(Use use |
-    use.hasIndexInBlock(bb, i) and
+    use.hasRankInBlock(bb, i) and
     v = use.getSourceVariable() and
     certain = true
   )

cpp/ql/lib/semmle/code/cpp/security/Overflow.qll

@@ -9,7 +9,6 @@ import semmle.code.cpp.controlflow.Dominance
 private import semmle.code.cpp.valuenumbering.GlobalValueNumbering
 import semmle.code.cpp.rangeanalysis.SimpleRangeAnalysis
 import semmle.code.cpp.rangeanalysis.RangeAnalysisUtils
-import semmle.code.cpp.controlflow.Guards
 
 /**
  * Holds if the value of `use` is guarded using `abs`.
@@ -17,16 +16,53 @@ import semmle.code.cpp.controlflow.Guards
 predicate guardedAbs(Operation e, Expr use) {
   exists(FunctionCall fc | fc.getTarget().getName() = ["abs", "labs", "llabs", "imaxabs"] |
     fc.getArgument(0).getAChild*() = use and
-    exists(GuardCondition c | c.ensuresLt(fc, _, _, e.getBasicBlock(), true))
+    guardedLesser(e, fc)
   )
 }
 
+/**
+ * Gets the position of `stmt` in basic block `block` (this is a thin layer
+ * over `BasicBlock.getNode`, intended to improve performance).
+ */
+pragma[noinline]
+private int getStmtIndexInBlock(BasicBlock block, Stmt stmt) { block.getNode(result) = stmt }
+
+pragma[inline]
+private predicate stmtDominates(Stmt dominator, Stmt dominated) {
+  // In same block
+  exists(BasicBlock block, int dominatorIndex, int dominatedIndex |
+    dominatorIndex = getStmtIndexInBlock(block, dominator) and
+    dominatedIndex = getStmtIndexInBlock(block, dominated) and
+    dominatedIndex >= dominatorIndex
+  )
+  or
+  // In (possibly) different blocks
+  bbStrictlyDominates(dominator.getBasicBlock(), dominated.getBasicBlock())
+}
+
 /**
  * Holds if the value of `use` is guarded to be less than something, and `e`
  * is in code controlled by that guard (where the guard condition held).
  */
+pragma[nomagic]
 predicate guardedLesser(Operation e, Expr use) {
-  exists(GuardCondition c | c.ensuresLt(use, _, _, e.getBasicBlock(), true))
+  exists(IfStmt c, RelationalOperation guard |
+    use = guard.getLesserOperand().getAChild*() and
+    guard = c.getControllingExpr().getAChild*() and
+    stmtDominates(c.getThen(), e.getEnclosingStmt())
+  )
+  or
+  exists(Loop c, RelationalOperation guard |
+    use = guard.getLesserOperand().getAChild*() and
+    guard = c.getControllingExpr().getAChild*() and
+    stmtDominates(c.getStmt(), e.getEnclosingStmt())
+  )
+  or
+  exists(ConditionalExpr c, RelationalOperation guard |
+    use = guard.getLesserOperand().getAChild*() and
+    guard = c.getCondition().getAChild*() and
+    c.getThen().getAChild*() = e
+  )
   or
   guardedAbs(e, use)
 }
@@ -35,8 +71,25 @@ predicate guardedLesser(Operation e, Expr use) {
  * Holds if the value of `use` is guarded to be greater than something, and `e`
  * is in code controlled by that guard (where the guard condition held).
  */
+pragma[nomagic]
 predicate guardedGreater(Operation e, Expr use) {
-  exists(GuardCondition c | c.ensuresLt(use, _, _, e.getBasicBlock(), false))
+  exists(IfStmt c, RelationalOperation guard |
+    use = guard.getGreaterOperand().getAChild*() and
+    guard = c.getControllingExpr().getAChild*() and
+    stmtDominates(c.getThen(), e.getEnclosingStmt())
+  )
+  or
+  exists(Loop c, RelationalOperation guard |
+    use = guard.getGreaterOperand().getAChild*() and
+    guard = c.getControllingExpr().getAChild*() and
+    stmtDominates(c.getStmt(), e.getEnclosingStmt())
+  )
+  or
+  exists(ConditionalExpr c, RelationalOperation guard |
+    use = guard.getGreaterOperand().getAChild*() and
+    guard = c.getCondition().getAChild*() and
+    c.getThen().getAChild*() = e
+  )
   or
   guardedAbs(e, use)
 }

cpp/ql/src/CHANGELOG.md

@@ -1,3 +1,5 @@
+## 0.0.6
+
 ## 0.0.5
 
 ### New Queries

cpp/ql/src/Likely Bugs/AmbiguouslySignedBitField.ql

@@ -26,8 +26,6 @@ where
   // At least for C programs on Windows, BOOL is a common typedef for a type
   // representing BoolType.
   not bf.getType().hasName("BOOL") and
-  // GLib's gboolean is a typedef for a type representing BoolType.
-  not bf.getType().hasName("gboolean") and
   // If this is true, then there cannot be unsigned sign extension or overflow.
   not bf.getDeclaredNumBits() = bf.getType().getSize() * 8 and
   not bf.isAnonymous() and

cpp/ql/src/Security/CWE/CWE-190/ArithmeticUncontrolled.ql

@@ -5,7 +5,7 @@
  * @kind path-problem
  * @problem.severity warning
  * @security-severity 8.6
- * @precision high
+ * @precision medium
  * @id cpp/uncontrolled-arithmetic
  * @tags security
  *       external/cwe/cwe-190
@@ -82,11 +82,8 @@ predicate missingGuard(VariableAccess va, string effect) {
     op.getUnspecifiedType().(IntegralType).isUnsigned() and
     not op instanceof MulExpr
     or
-    // overflow - only report signed integer overflow since unsigned overflow
-    // is well-defined.
-    op.getUnspecifiedType().(IntegralType).isSigned() and
-    missingGuardAgainstOverflow(op, va) and
-    effect = "overflow"
+    // overflow
+    missingGuardAgainstOverflow(op, va) and effect = "overflow"
   )
 }
 

cpp/ql/src/Security/CWE/CWE-191/UnsignedDifferenceExpressionComparedZero.ql

@@ -22,11 +22,13 @@ import semmle.code.cpp.dataflow.DataFlow
  *  Holds if `sub` is guarded by a condition which ensures that
  * `left >= right`.
  */
-pragma[noinline]
+pragma[nomagic]
 predicate isGuarded(SubExpr sub, Expr left, Expr right) {
-  exists(GuardCondition guard, int k |
-    guard.controls(sub.getBasicBlock(), _) and
-    guard.ensuresLt(left, right, k, sub.getBasicBlock(), false) and
+  exprIsSubLeftOrLess(pragma[only_bind_into](sub), _) and // Manual magic
+  exists(GuardCondition guard, int k, BasicBlock bb |
+    pragma[only_bind_into](bb) = sub.getBasicBlock() and
+    guard.controls(pragma[only_bind_into](bb), _) and
+    guard.ensuresLt(left, right, k, bb, false) and
     k >= 0
   )
 }
@@ -36,47 +38,56 @@ predicate isGuarded(SubExpr sub, Expr left, Expr right) {
  * `sub.getLeftOperand()`.
  */
 predicate exprIsSubLeftOrLess(SubExpr sub, DataFlow::Node n) {
-  n.asExpr() = sub.getLeftOperand()
-  or
-  exists(DataFlow::Node other |
-    // dataflow
-    exprIsSubLeftOrLess(sub, other) and
-    (
-      DataFlow::localFlowStep(n, other) or
-      DataFlow::localFlowStep(other, n)
+  interestingSubExpr(sub, _) and // Manual magic
+  (
+    n.asExpr() = sub.getLeftOperand()
+    or
+    exists(DataFlow::Node other |
+      // dataflow
+      exprIsSubLeftOrLess(sub, other) and
+      (
+        DataFlow::localFlowStep(n, other) or
+        DataFlow::localFlowStep(other, n)
+      )
+    )
+    or
+    exists(DataFlow::Node other |
+      // guard constraining `sub`
+      exprIsSubLeftOrLess(sub, other) and
+      isGuarded(sub, other.asExpr(), n.asExpr()) // other >= n
+    )
+    or
+    exists(DataFlow::Node other, float p, float q |
+      // linear access of `other`
+      exprIsSubLeftOrLess(sub, other) and
+      linearAccess(n.asExpr(), other.asExpr(), p, q) and // n = p * other + q
+      p <= 1 and
+      q <= 0
+    )
+    or
+    exists(DataFlow::Node other, float p, float q |
+      // linear access of `n`
+      exprIsSubLeftOrLess(sub, other) and
+      linearAccess(other.asExpr(), n.asExpr(), p, q) and // other = p * n + q
+      p >= 1 and
+      q >= 0
     )
-  )
-  or
-  exists(DataFlow::Node other |
-    // guard constraining `sub`
-    exprIsSubLeftOrLess(sub, other) and
-    isGuarded(sub, other.asExpr(), n.asExpr()) // other >= n
-  )
-  or
-  exists(DataFlow::Node other, float p, float q |
-    // linear access of `other`
-    exprIsSubLeftOrLess(sub, other) and
-    linearAccess(n.asExpr(), other.asExpr(), p, q) and // n = p * other + q
-    p <= 1 and
-    q <= 0
-  )
-  or
-  exists(DataFlow::Node other, float p, float q |
-    // linear access of `n`
-    exprIsSubLeftOrLess(sub, other) and
-    linearAccess(other.asExpr(), n.asExpr(), p, q) and // other = p * n + q
-    p >= 1 and
-    q >= 0
   )
 }
 
-from RelationalOperation ro, SubExpr sub
-where
-  not isFromMacroDefinition(ro) and
+predicate interestingSubExpr(SubExpr sub, RelationalOperation ro) {
   not isFromMacroDefinition(sub) and
   ro.getLesserOperand().getValue().toInt() = 0 and
   ro.getGreaterOperand() = sub and
   sub.getFullyConverted().getUnspecifiedType().(IntegralType).isUnsigned() and
-  exprMightOverflowNegatively(sub.getFullyConverted()) and // generally catches false positives involving constants
-  not exprIsSubLeftOrLess(sub, DataFlow::exprNode(sub.getRightOperand())) // generally catches false positives where there's a relation between the left and right operands
+  // generally catches false positives involving constants
+  exprMightOverflowNegatively(sub.getFullyConverted())
+}
+
+from RelationalOperation ro, SubExpr sub
+where
+  interestingSubExpr(sub, ro) and
+  not isFromMacroDefinition(ro) and
+  // generally catches false positives where there's a relation between the left and right operands
+  not exprIsSubLeftOrLess(sub, DataFlow::exprNode(sub.getRightOperand()))
 select ro, "Unsigned subtraction can never be negative."

cpp/ql/src/Security/CWE/CWE-367/TOCTOUFilesystemRace.ql

@@ -89,83 +89,45 @@ predicate referenceTo(Expr source, Expr use) {
   )
 }
 
-pragma[noinline]
-predicate statCallWithPointer(Expr checkPath, Expr call, Expr e, Variable v) {
-  call = stat(checkPath, e) and
-  e.getAChild*().(VariableAccess).getTarget() = v
-}
-
-predicate checksPath(Expr check, Expr checkPath) {
-  // either:
-  // an access check
-  check = accessCheck(checkPath)
-  or
-  // a stat
-  check = stat(checkPath, _)
-  or
-  // access to a member variable on the stat buf
-  // (morally, this should be a use-use pair, but it seems unlikely
-  // that this variable will get reused in practice)
-  exists(Expr call, Expr e, Variable v |
-    statCallWithPointer(checkPath, call, e, v) and
-    check.(VariableAccess).getTarget() = v and
-    not e.getAChild*() = check // the call that writes to the pointer is not where the pointer is checked.
-  )
-}
-
-pragma[nomagic]
-predicate checkPathControlsUse(Expr check, Expr checkPath, Expr use) {
-  exists(GuardCondition guard | referenceTo(check, guard.getAChild*()) |
-    guard.controls(use.getBasicBlock(), _)
-  ) and
-  checksPath(pragma[only_bind_into](check), checkPath)
-}
-
-pragma[nomagic]
-predicate fileNameOperationControlsUse(Expr check, Expr checkPath, Expr use) {
-  exists(GuardCondition guard | referenceTo(check, guard.getAChild*()) |
-    guard.controls(use.getBasicBlock(), _)
-  ) and
-  pragma[only_bind_into](check) = filenameOperation(checkPath)
-}
-
-predicate checkUse(Expr check, Expr checkPath, FunctionCall use, Expr usePath) {
-  // `check` is part of a guard that controls `use`
-  checkPathControlsUse(check, checkPath, use) and
-  // `check` looks like a check on a filename
-  checksPath(check, checkPath) and
-  // `op` looks like an operation on a filename
-  use = filenameOperation(usePath)
-  or
-  // `check` is part of a guard that controls `use`
-  fileNameOperationControlsUse(check, checkPath, use) and
-  // another filename operation (null pointers can indicate errors)
-  check = filenameOperation(checkPath) and
-  // `op` looks like a sensitive operation on a filename
-  use = sensitiveFilenameOperation(usePath)
-}
-
-pragma[noinline]
-predicate isCheckedPath(
-  Expr check, SsaDefinition def, StackVariable v, FunctionCall use, Expr usePath, Expr checkPath
-) {
-  checkUse(check, checkPath, use, usePath) and
-  def.getAUse(v) = checkPath
-}
-
-pragma[noinline]
-predicate isUsedPath(
-  Expr check, SsaDefinition def, StackVariable v, FunctionCall use, Expr usePath, Expr checkPath
-) {
-  checkUse(check, checkPath, use, usePath) and
-  def.getAUse(v) = usePath
-}
-
-from Expr check, Expr checkPath, FunctionCall use, Expr usePath, SsaDefinition def, StackVariable v
+from Expr check, Expr checkPath, FunctionCall use, Expr usePath
 where
+  // `check` looks like a check on a filename
+  (
+    (
+      // either:
+      // an access check
+      check = accessCheck(checkPath)
+      or
+      // a stat
+      check = stat(checkPath, _)
+      or
+      // access to a member variable on the stat buf
+      // (morally, this should be a use-use pair, but it seems unlikely
+      // that this variable will get reused in practice)
+      exists(Expr call, Expr e, Variable v |
+        call = stat(checkPath, e) and
+        e.getAChild*().(VariableAccess).getTarget() = v and
+        check.(VariableAccess).getTarget() = v and
+        not e.getAChild*() = check // the call that writes to the pointer is not where the pointer is checked.
+      )
+    ) and
+    // `op` looks like an operation on a filename
+    use = filenameOperation(usePath)
+    or
+    // another filename operation (null pointers can indicate errors)
+    check = filenameOperation(checkPath) and
+    // `op` looks like a sensitive operation on a filename
+    use = sensitiveFilenameOperation(usePath)
+  ) and
   // `checkPath` and `usePath` refer to the same SSA variable
-  isCheckedPath(check, def, v, use, usePath, checkPath) and
-  isUsedPath(check, def, v, use, usePath, checkPath)
+  exists(SsaDefinition def, StackVariable v |
+    def.getAUse(v) = checkPath and def.getAUse(v) = usePath
+  ) and
+  // the return value of `check` is used (possibly with one step of
+  // variable indirection) in a guard which controls `use`
+  exists(GuardCondition guard | referenceTo(check, guard.getAChild*()) |
+    guard.controls(use.(ControlFlowNode).getBasicBlock(), _)
+  )
 select use,
   "The $@ being operated upon was previously $@, but the underlying file may have been changed since then.",
   usePath, "filename", check, "checked"

cpp/ql/src/change-notes/2021-12-30-ambiguously-signed-bit-field.md

@@ -1,5 +0,0 @@
----
-category: minorAnalysis
----
-* Added exception for GLib's gboolean to cpp/ambiguously-signed-bit-field.
-  This change reduces the number of false positives in the query.

cpp/ql/src/change-notes/2022-01-05-promote-uncontrolled-arithmetic.md

@@ -1,4 +0,0 @@
----
-category: newQuery
----
-* The "Uncontrolled data in arithmetic expression" (cpp/uncontrolled-arithmetic) query has been enhanced to reduce false positive results and its @precision increased to high.

cpp/ql/src/change-notes/released/0.0.6.md

@@ -0,0 +1 @@
+## 0.0.6

cpp/ql/src/codeql-pack.release.yml

@@ -1,2 +1,2 @@
 ---
-lastReleaseVersion: 0.0.5
+lastReleaseVersion: 0.0.6

cpp/ql/src/qlpack.yml

@@ -1,5 +1,5 @@
 name: codeql/cpp-queries
-version: 0.0.6-dev
+version: 0.0.6
 groups: cpp
 dependencies:
     codeql/cpp-all: "*"

cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/ArithmeticUncontrolled/ArithmeticUncontrolled.expected

@@ -35,6 +35,8 @@ edges
 | test.cpp:190:10:190:13 | call to rand | test.cpp:205:7:205:7 | y |
 | test.cpp:190:10:190:13 | call to rand | test.cpp:208:7:208:7 | y |
 | test.cpp:215:11:215:14 | call to rand | test.cpp:219:8:219:8 | x |
+| test.cpp:223:20:223:23 | call to rand | test.cpp:227:8:227:8 | x |
+| test.cpp:223:20:223:25 | (unsigned int)... | test.cpp:227:8:227:8 | x |
 nodes
 | test.c:18:13:18:16 | call to rand | semmle.label | call to rand |
 | test.c:21:17:21:17 | r | semmle.label | r |
@@ -90,6 +92,9 @@ nodes
 | test.cpp:208:7:208:7 | y | semmle.label | y |
 | test.cpp:215:11:215:14 | call to rand | semmle.label | call to rand |
 | test.cpp:219:8:219:8 | x | semmle.label | x |
+| test.cpp:223:20:223:23 | call to rand | semmle.label | call to rand |
+| test.cpp:223:20:223:25 | (unsigned int)... | semmle.label | (unsigned int)... |
+| test.cpp:227:8:227:8 | x | semmle.label | x |
 subpaths
 #select
 | test.c:21:17:21:17 | r | test.c:18:13:18:16 | call to rand | test.c:21:17:21:17 | r | $@ flows to here and is used in arithmetic, potentially causing an overflow. | test.c:18:13:18:16 | call to rand | Uncontrolled value |
@@ -120,3 +125,5 @@ subpaths
 | test.cpp:205:7:205:7 | y | test.cpp:190:10:190:13 | call to rand | test.cpp:205:7:205:7 | y | $@ flows to here and is used in arithmetic, potentially causing an overflow. | test.cpp:190:10:190:13 | call to rand | Uncontrolled value |
 | test.cpp:208:7:208:7 | y | test.cpp:190:10:190:13 | call to rand | test.cpp:208:7:208:7 | y | $@ flows to here and is used in arithmetic, potentially causing an overflow. | test.cpp:190:10:190:13 | call to rand | Uncontrolled value |
 | test.cpp:219:8:219:8 | x | test.cpp:215:11:215:14 | call to rand | test.cpp:219:8:219:8 | x | $@ flows to here and is used in arithmetic, potentially causing an overflow. | test.cpp:215:11:215:14 | call to rand | Uncontrolled value |
+| test.cpp:227:8:227:8 | x | test.cpp:223:20:223:23 | call to rand | test.cpp:227:8:227:8 | x | $@ flows to here and is used in arithmetic, potentially causing an overflow. | test.cpp:223:20:223:23 | call to rand | Uncontrolled value |
+| test.cpp:227:8:227:8 | x | test.cpp:223:20:223:25 | (unsigned int)... | test.cpp:227:8:227:8 | x | $@ flows to here and is used in arithmetic, potentially causing an overflow. | test.cpp:223:20:223:23 | call to rand | Uncontrolled value |

cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/ArithmeticUncontrolled/test.c

@@ -157,11 +157,3 @@ void moreTests() {
     r = r - 100; // BAD
   }
 }
-
-void guarded_test(unsigned p) {
-  unsigned data = (unsigned int)rand();
-  if (p >= data) {
-    return;
-  }
-  unsigned z = data - p; // GOOD
-}

cpp/ql/test/query-tests/Security/CWE/CWE-190/semmle/ArithmeticUncontrolled/test.cpp

@@ -224,6 +224,6 @@ void test_mod_limit()
 		unsigned int y = 100;
 		unsigned int z;
 
-		z = (x + y) % 1000; // DUBIOUS (this could overflow but the result is controlled)
+		z = (x + y) % 1000; // DUBIOUS (this could overflow but the result is controlled) [REPORTED]
 	}
 }

cpp/upgrades/CHANGELOG.md

@@ -0,0 +1,5 @@
+## 0.0.6
+
+## 0.0.5
+
+## 0.0.4

cpp/upgrades/change-notes/released/0.0.4.md

@@ -0,0 +1 @@
+## 0.0.4

cpp/upgrades/change-notes/released/0.0.5.md

@@ -0,0 +1 @@
+## 0.0.5

cpp/upgrades/change-notes/released/0.0.6.md

@@ -0,0 +1 @@
+## 0.0.6