Merge branch 'main' into post-release-prep/codeql-cli-2.20.0

Author: angelapwen

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

@@ -2275,6 +2275,12 @@ private predicate guardControlsPhiInput(
  */
 signature predicate guardChecksSig(IRGuardCondition g, Expr e, boolean branch);
 
+bindingset[g, n]
+pragma[inline_late]
+private predicate controls(IRGuardCondition g, Node n, boolean edge) {
+  g.controls(n.getBasicBlock(), edge)
+}
+
 /**
  * Provides a set of barrier nodes for a guard that validates an expression.
  *
@@ -2318,15 +2324,17 @@ module BarrierGuard<guardChecksSig/3 guardChecks> {
     exists(IRGuardCondition g, Expr e, ValueNumber value, boolean edge |
       e = value.getAnInstruction().getConvertedResultExpression() and
       result.asConvertedExpr() = e and
-      guardChecks(g, value.getAnInstruction().getConvertedResultExpression(), edge) and
-      g.controls(result.getBasicBlock(), edge)
+      guardChecks(g,
+        pragma[only_bind_into](value.getAnInstruction().getConvertedResultExpression()), edge) and
+      controls(g, result, edge)
     )
     or
     exists(
       IRGuardCondition g, boolean branch, Ssa::DefinitionExt def, IRBlock input, Ssa::PhiNode phi
     |
       guardChecks(g, def.getARead().asOperand().getDef().getConvertedResultExpression(), branch) and
-      guardControlsPhiInput(g, branch, def, input, phi) and
+      guardControlsPhiInput(g, branch, def, pragma[only_bind_into](input),
+        pragma[only_bind_into](phi)) and
       result = TSsaPhiInputNode(phi, input)
     )
   }
@@ -2404,8 +2412,9 @@ module BarrierGuard<guardChecksSig/3 guardChecks> {
     exists(IRGuardCondition g, Expr e, ValueNumber value, boolean edge |
       e = value.getAnInstruction().getConvertedResultExpression() and
       result.asIndirectConvertedExpr(indirectionIndex) = e and
-      guardChecks(g, value.getAnInstruction().getConvertedResultExpression(), edge) and
-      g.controls(result.getBasicBlock(), edge)
+      guardChecks(g,
+        pragma[only_bind_into](value.getAnInstruction().getConvertedResultExpression()), edge) and
+      controls(g, result, edge)
     )
     or
     exists(
@@ -2414,7 +2423,8 @@ module BarrierGuard<guardChecksSig/3 guardChecks> {
       guardChecks(g,
         def.getARead().asIndirectOperand(indirectionIndex).getDef().getConvertedResultExpression(),
         branch) and
-      guardControlsPhiInput(g, branch, def, input, phi) and
+      guardControlsPhiInput(g, branch, def, pragma[only_bind_into](input),
+        pragma[only_bind_into](phi)) and
       result = TSsaPhiInputNode(phi, input)
     )
   }
@@ -2443,17 +2453,18 @@ module InstructionBarrierGuard<instructionGuardChecksSig/3 instructionGuardCheck
   /** Gets a node that is safely guarded by the given guard check. */
   Node getABarrierNode() {
     exists(IRGuardCondition g, ValueNumber value, boolean edge, Operand use |
-      instructionGuardChecks(g, value.getAnInstruction(), edge) and
+      instructionGuardChecks(g, pragma[only_bind_into](value.getAnInstruction()), edge) and
       use = value.getAnInstruction().getAUse() and
       result.asOperand() = use and
-      g.controls(result.getBasicBlock(), edge)
+      controls(g, result, edge)
     )
     or
     exists(
       IRGuardCondition g, boolean branch, Ssa::DefinitionExt def, IRBlock input, Ssa::PhiNode phi
     |
       instructionGuardChecks(g, def.getARead().asOperand().getDef(), branch) and
-      guardControlsPhiInput(g, branch, def, input, phi) and
+      guardControlsPhiInput(g, branch, def, pragma[only_bind_into](input),
+        pragma[only_bind_into](phi)) and
       result = TSsaPhiInputNode(phi, input)
     )
   }

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

@@ -1276,6 +1276,7 @@ class DefinitionExt extends SsaImpl::DefinitionExt {
   }
 
   /** Gets a node that represents a read of this SSA definition. */
+  pragma[nomagic]
   Node getARead() {
     exists(SourceVariable sv, IRBlock bb, int i | SsaCached::ssaDefReachesReadExt(sv, this, bb, i) |
       useToNode(bb, i, sv, result)

cpp/ql/lib/semmle/code/cpp/ir/implementation/EdgeKind.qll

@@ -8,7 +8,8 @@ private newtype TEdgeKind =
   TGotoEdge() or // Single successor (including fall-through)
   TTrueEdge() or // 'true' edge of conditional branch
   TFalseEdge() or // 'false' edge of conditional branch
-  TExceptionEdge() or // Thrown exception
+  TCppExceptionEdge() or // Thrown C++ exception
+  TSehExceptionEdge() or // Thrown SEH exception
   TDefaultEdge() or // 'default' label of switch
   TCaseEdge(string minValue, string maxValue) {
     // Case label of switch
@@ -20,56 +21,77 @@ private newtype TEdgeKind =
  * `Instruction` or `IRBlock` has at most one successor of any single
  * `EdgeKind`.
  */
-abstract class EdgeKind extends TEdgeKind {
+abstract private class EdgeKindImpl extends TEdgeKind {
   /** Gets a textual representation of this edge kind. */
   abstract string toString();
 }
 
+final class EdgeKind = EdgeKindImpl;
+
 /**
  * A "goto" edge, representing the unconditional successor of an `Instruction`
  * or `IRBlock`.
  */
-class GotoEdge extends EdgeKind, TGotoEdge {
+class GotoEdge extends EdgeKindImpl, TGotoEdge {
   final override string toString() { result = "Goto" }
 }
 
 /**
  * A "true" edge, representing the successor of a conditional branch when the
  * condition is non-zero.
  */
-class TrueEdge extends EdgeKind, TTrueEdge {
+class TrueEdge extends EdgeKindImpl, TTrueEdge {
   final override string toString() { result = "True" }
 }
 
 /**
  * A "false" edge, representing the successor of a conditional branch when the
  * condition is zero.
  */
-class FalseEdge extends EdgeKind, TFalseEdge {
+class FalseEdge extends EdgeKindImpl, TFalseEdge {
   final override string toString() { result = "False" }
 }
 
+abstract private class ExceptionEdgeImpl extends EdgeKindImpl { }
+
 /**
  * An "exception" edge, representing the successor of an instruction when that
  * instruction's evaluation throws an exception.
+ *
+ * Exception edges are expclitly sublcassed to `CppExceptionEdge` and `SehExceptionEdge`
+ * only. Further sublcasses, if required, should be added privately here for IR efficiency.
+ */
+final class ExceptionEdge = ExceptionEdgeImpl;
+
+/**
+ * An "exception" edge, representing the successor of an instruction when that
+ * instruction's evaluation throws a C++ exception.
+ */
+class CppExceptionEdge extends ExceptionEdgeImpl, TCppExceptionEdge {
+  final override string toString() { result = "C++ Exception" }
+}
+
+/**
+ * An "exception" edge, representing the successor of an instruction when that
+ * instruction's evaluation throws an SEH exception.
  */
-class ExceptionEdge extends EdgeKind, TExceptionEdge {
-  final override string toString() { result = "Exception" }
+class SehExceptionEdge extends ExceptionEdgeImpl, TSehExceptionEdge {
+  final override string toString() { result = "SEH Exception" }
 }
 
 /**
  * A "default" edge, representing the successor of a `Switch` instruction when
  * none of the case values matches the condition value.
  */
-class DefaultEdge extends EdgeKind, TDefaultEdge {
+class DefaultEdge extends EdgeKindImpl, TDefaultEdge {
   final override string toString() { result = "Default" }
 }
 
 /**
  * A "case" edge, representing the successor of a `Switch` instruction when the
  * the condition value matches a corresponding `case` label.
  */
-class CaseEdge extends EdgeKind, TCaseEdge {
+class CaseEdge extends EdgeKindImpl, TCaseEdge {
   string minValue;
   string maxValue;
 
@@ -121,9 +143,14 @@ module EdgeKind {
   FalseEdge falseEdge() { result = TFalseEdge() }
 
   /**
-   * Gets the single instance of the `ExceptionEdge` class.
+   * Gets the single instance of the `CppExceptionEdge` class.
+   */
+  CppExceptionEdge cppExceptionEdge() { result = TCppExceptionEdge() }
+
+  /**
+   * Gets the single instance of the `SehExceptionEdge` class.
    */
-  ExceptionEdge exceptionEdge() { result = TExceptionEdge() }
+  SehExceptionEdge sehExceptionEdge() { result = TSehExceptionEdge() }
 
   /**
    * Gets the single instance of the `DefaultEdge` class.

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

@@ -88,7 +88,7 @@ abstract class TranslatedCall extends TranslatedExpr {
       result = this.getParent().getChildSuccessor(this, kind)
       or
       this.mayThrowException() and
-      kind instanceof ExceptionEdge and
+      kind instanceof CppExceptionEdge and
       result = this.getParent().getExceptionSuccessorInstruction(any(GotoEdge edge))
     )
   }

cpp/ql/lib/semmle/code/cpp/ir/implementation/raw/internal/TranslatedExpr.qll

@@ -3039,7 +3039,7 @@ class TranslatedDestructorsAfterThrow extends TranslatedElement, TTranslatedDest
       or
       // And otherwise, exit this element with an exceptional edge
       not exists(this.getChild(id + 1)) and
-      kind instanceof ExceptionEdge and
+      kind instanceof CppExceptionEdge and
       result = this.getParent().getExceptionSuccessorInstruction(any(GotoEdge edge))
     )
   }
@@ -3078,7 +3078,7 @@ abstract class TranslatedThrowExpr extends TranslatedNonConstantExpr {
       result = this.getDestructors().getFirstInstruction(kind)
       or
       not exists(this.getDestructors()) and
-      kind instanceof ExceptionEdge and
+      kind instanceof CppExceptionEdge and
       result = this.getParent().getExceptionSuccessorInstruction(any(GotoEdge edge))
     )
   }

cpp/ql/lib/semmle/code/cpp/ir/implementation/raw/internal/TranslatedStmt.qll

@@ -932,7 +932,7 @@ class TranslatedCatchByTypeHandler extends TranslatedHandler {
       kind instanceof GotoEdge and
       result = this.getParameter().getFirstInstruction(kind)
       or
-      kind instanceof ExceptionEdge and
+      kind instanceof CppExceptionEdge and
       if exists(this.getDestructors())
       then result = this.getDestructors().getFirstInstruction(any(GotoEdge edge))
       else result = this.getParent().(TranslatedTryStmt).getNextHandler(this, any(GotoEdge edge))

cpp/ql/src/Best Practices/GuardedFree.ql

@@ -27,8 +27,12 @@ predicate blockContainsPreprocessorBranches(BasicBlock bb) {
   )
 }
 
-from GuardCondition gc, FreeCall fc, Variable v, BasicBlock bb
-where
+/**
+ * Holds if `gc` ensures that `v` is non-zero when reaching `bb`, and `bb`
+ * contains a single statement which is `fc`.
+ */
+pragma[nomagic]
+private predicate interesting(GuardCondition gc, FreeCall fc, Variable v, BasicBlock bb) {
   gc.ensuresEq(v.getAnAccess(), 0, bb, false) and
   fc.getArgument(0) = v.getAnAccess() and
   bb = fc.getBasicBlock() and
@@ -39,9 +43,21 @@ where
     // Block statement with a single nested statement: if (x) { free(x); }
     strictcount(bb.(BlockStmt).getAStmt()) = 1
   ) and
-  strictcount(BasicBlock bb2 | gc.ensuresEq(_, 0, bb2, _) | bb2) = 1 and
   not fc.isInMacroExpansion() and
   not blockContainsPreprocessorBranches(bb) and
   not (gc instanceof BinaryOperation and not gc instanceof ComparisonOperation) and
   not exists(CommaExpr c | c.getAChild*() = fc)
+}
+
+/** Holds if `gc` only guards a single block. */
+bindingset[gc]
+pragma[inline_late]
+private predicate guardConditionGuardsUniqueBlock(GuardCondition gc) {
+  strictcount(BasicBlock bb | gc.ensuresEq(_, 0, bb, _)) = 1
+}
+
+from GuardCondition gc, FreeCall fc, Variable v, BasicBlock bb
+where
+  interesting(gc, fc, v, bb) and
+  guardConditionGuardsUniqueBlock(gc)
 select gc, "unnecessary NULL check before call to $@", fc, "free"

cpp/ql/src/Likely Bugs/Format/WrongNumberOfFormatArguments.ql

@@ -44,7 +44,8 @@ where
   ) and
   // A typical problem is that string literals are concatenated, but if one of the string
   // literals is an undefined macro, then this just leads to a syntax error.
-  not exists(SyntaxError e | e.affects(fl))
+  not exists(SyntaxError e | e.affects(fl)) and
+  not ffc.getArgument(_) instanceof ErrorExpr
 select ffc,
   "Format for " + ffcName + " expects " + expected.toString() + " arguments but given " +
     given.toString()

cpp/ql/src/Likely Bugs/Format/WrongTypeFormatArguments.ql

@@ -170,7 +170,7 @@ where
   ) and
   not arg.isAffectedByMacro() and
   not arg.isFromUninstantiatedTemplate(_) and
-  not actual.getUnspecifiedType() instanceof ErroneousType and
+  not actual.stripType() instanceof ErroneousType and
   not arg.(Call).mayBeFromImplicitlyDeclaredFunction()
 select arg,
   "This format specifier for type '" + expected.getName() + "' does not match the argument type '" +

cpp/ql/src/Security/CWE/CWE-120/BadlyBoundedWrite.ql

@@ -25,7 +25,8 @@ from BufferWrite bw, int destSize
 where
   bw.hasExplicitLimit() and // has an explicit size limit
   destSize = max(getBufferSize(bw.getDest(), _)) and
-  bw.getExplicitLimit() > destSize // but it's larger than the destination
+  bw.getExplicitLimit() > destSize and // but it's larger than the destination
+  not bw.getDest().getType().stripType() instanceof ErroneousType // destSize may be incorrect
 select bw,
   "This '" + bw.getBWDesc() + "' operation is limited to " + bw.getExplicitLimit() +
     " bytes but the destination is only " + destSize + " bytes."