Merge branch 'main' into no-dtt-in-unbounded-write

Author: MathiasVP

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

@@ -31,6 +31,11 @@ abstract class MustFlowConfiguration extends string {
    */
   abstract predicate isSink(Operand sink);
 
+  /**
+   * Holds if data flow through `instr` is prohibited.
+   */
+  predicate isBarrier(Instruction instr) { none() }
+
   /**
    * Holds if the additional flow step from `node1` to `node2` must be taken
    * into account in the analysis.
@@ -48,18 +53,21 @@ abstract class MustFlowConfiguration extends string {
    */
   final predicate hasFlowPath(MustFlowPathNode source, MustFlowPathSink sink) {
     this.isSource(source.getInstruction()) and
-    source.getASuccessor+() = sink
+    source.getASuccessor*() = sink
   }
 }
 
 /** Holds if `node` flows from a source. */
 pragma[nomagic]
 private predicate flowsFromSource(Instruction node, MustFlowConfiguration config) {
-  config.isSource(node)
-  or
-  exists(Instruction mid |
-    step(mid, node, config) and
-    flowsFromSource(mid, pragma[only_bind_into](config))
+  not config.isBarrier(node) and
+  (
+    config.isSource(node)
+    or
+    exists(Instruction mid |
+      step(mid, node, config) and
+      flowsFromSource(mid, pragma[only_bind_into](config))
+    )
   )
 }
 

cpp/ql/lib/semmle/code/cpp/rangeanalysis/new/internal/semantic/SemanticCFG.qll

@@ -12,9 +12,6 @@ class SemBasicBlock extends Specific::BasicBlock {
   /** Holds if this block (transitively) dominates `otherblock`. */
   final predicate bbDominates(SemBasicBlock otherBlock) { Specific::bbDominates(this, otherBlock) }
 
-  /** Holds if this block has dominance information. */
-  final predicate hasDominanceInformation() { Specific::hasDominanceInformation(this) }
-
   /** Gets an expression that is evaluated in this basic block. */
   final SemExpr getAnExpr() { result.getBasicBlock() = this }
 

cpp/ql/lib/semmle/code/cpp/rangeanalysis/new/internal/semantic/SemanticExprSpecific.qll

@@ -122,8 +122,6 @@ module SemanticExprConfig {
     dominator.dominates(dominated)
   }
 
-  predicate hasDominanceInformation(BasicBlock block) { any() }
-
   private predicate id(Cpp::Locatable x, Cpp::Locatable y) { x = y }
 
   private predicate idOf(Cpp::Locatable x, int y) = equivalenceRelation(id/2)(x, y)
@@ -132,17 +130,7 @@ module SemanticExprConfig {
 
   newtype TSsaVariable =
     TSsaInstruction(IR::Instruction instr) { instr.hasMemoryResult() } or
-    TSsaOperand(IR::Operand op) { op.isDefinitionInexact() } or
-    TSsaPointerArithmeticGuard(ValueNumber instr) {
-      exists(Guard g, IR::Operand use |
-        use = instr.getAUse() and use.getIRType() instanceof IR::IRAddressType
-      |
-        g.comparesLt(use, _, _, _, _) or
-        g.comparesLt(_, use, _, _, _) or
-        g.comparesEq(use, _, _, _, _) or
-        g.comparesEq(_, use, _, _, _)
-      )
-    }
+    TSsaOperand(IR::Operand op) { op.isDefinitionInexact() }
 
   class SsaVariable extends TSsaVariable {
     string toString() { none() }
@@ -151,8 +139,6 @@ module SemanticExprConfig {
 
     IR::Instruction asInstruction() { none() }
 
-    ValueNumber asPointerArithGuard() { none() }
-
     IR::Operand asOperand() { none() }
   }
 
@@ -168,18 +154,6 @@ module SemanticExprConfig {
     final override IR::Instruction asInstruction() { result = instr }
   }
 
-  class SsaPointerArithmeticGuard extends SsaVariable, TSsaPointerArithmeticGuard {
-    ValueNumber vn;
-
-    SsaPointerArithmeticGuard() { this = TSsaPointerArithmeticGuard(vn) }
-
-    final override string toString() { result = vn.toString() }
-
-    final override Location getLocation() { result = vn.getLocation() }
-
-    final override ValueNumber asPointerArithGuard() { result = vn }
-  }
-
   class SsaOperand extends SsaVariable, TSsaOperand {
     IR::Operand op;
 
@@ -212,11 +186,7 @@ module SemanticExprConfig {
     )
   }
 
-  Expr getAUse(SsaVariable v) {
-    result.(IR::LoadInstruction).getSourceValue() = v.asInstruction()
-    or
-    result = v.asPointerArithGuard().getAnInstruction()
-  }
+  Expr getAUse(SsaVariable v) { result.(IR::LoadInstruction).getSourceValue() = v.asInstruction() }
 
   SemType getSsaVariableType(SsaVariable v) {
     result = getSemanticType(v.asInstruction().getResultIRType())
@@ -255,10 +225,7 @@ module SemanticExprConfig {
     final override Location getLocation() { result = block.getLocation() }
 
     final override predicate hasRead(SsaVariable v) {
-      exists(IR::Operand operand |
-        operand.getDef() = v.asInstruction() or
-        operand.getDef() = v.asPointerArithGuard().getAnInstruction()
-      |
+      exists(IR::Operand operand | operand.getDef() = v.asInstruction() |
         not operand instanceof IR::PhiInputOperand and
         operand.getUse().getBlock() = block
       )
@@ -276,10 +243,7 @@ module SemanticExprConfig {
     final override Location getLocation() { result = succ.getLocation() }
 
     final override predicate hasRead(SsaVariable v) {
-      exists(IR::PhiInputOperand operand |
-        operand.getDef() = v.asInstruction() or
-        operand.getDef() = v.asPointerArithGuard().getAnInstruction()
-      |
+      exists(IR::PhiInputOperand operand | operand.getDef() = v.asInstruction() |
         operand.getPredecessorBlock() = pred and
         operand.getUse().getBlock() = succ
       )

cpp/ql/lib/semmle/code/cpp/rangeanalysis/new/internal/semantic/SemanticGuard.qll

@@ -35,32 +35,4 @@ predicate semImplies_v2(SemGuard g1, boolean b1, SemGuard g2, boolean b2) {
   Specific::implies_v2(g1, b1, g2, b2)
 }
 
-/**
- * Holds if `guard` directly controls the position `controlled` with the
- * value `testIsTrue`.
- */
-pragma[nomagic]
-predicate semGuardDirectlyControlsSsaRead(
-  SemGuard guard, SemSsaReadPosition controlled, boolean testIsTrue
-) {
-  guard.directlyControls(controlled.(SemSsaReadPositionBlock).getBlock(), testIsTrue)
-  or
-  exists(SemSsaReadPositionPhiInputEdge controlledEdge | controlledEdge = controlled |
-    guard.directlyControls(controlledEdge.getOrigBlock(), testIsTrue) or
-    guard.hasBranchEdge(controlledEdge.getOrigBlock(), controlledEdge.getPhiBlock(), testIsTrue)
-  )
-}
-
-/**
- * Holds if `guard` controls the position `controlled` with the value `testIsTrue`.
- */
-predicate semGuardControlsSsaRead(SemGuard guard, SemSsaReadPosition controlled, boolean testIsTrue) {
-  semGuardDirectlyControlsSsaRead(guard, controlled, testIsTrue)
-  or
-  exists(SemGuard guard0, boolean testIsTrue0 |
-    semImplies_v2(guard0, testIsTrue0, guard, testIsTrue) and
-    semGuardControlsSsaRead(guard0, controlled, testIsTrue0)
-  )
-}
-
 SemGuard semGetComparisonGuard(SemRelationalExpr e) { result = Specific::comparisonGuard(e) }

cpp/ql/lib/semmle/code/cpp/rangeanalysis/new/internal/semantic/SemanticSSA.qll

@@ -63,36 +63,3 @@ class SemSsaReadPositionBlock extends SemSsaReadPosition {
 
   SemExpr getAnExpr() { result = this.getBlock().getAnExpr() }
 }
-
-/**
- * Holds if `inp` is an input to `phi` along a back edge.
- */
-predicate semBackEdge(SemSsaPhiNode phi, SemSsaVariable inp, SemSsaReadPositionPhiInputEdge edge) {
-  edge.phiInput(phi, inp) and
-  // Conservatively assume that every edge is a back edge if we don't have dominance information.
-  (
-    phi.getBasicBlock().bbDominates(edge.getOrigBlock()) or
-    irreducibleSccEdge(edge.getOrigBlock(), phi.getBasicBlock()) or
-    not edge.getOrigBlock().hasDominanceInformation()
-  )
-}
-
-/**
- * Holds if the edge from b1 to b2 is part of a multiple-entry cycle in an irreducible control flow
- * graph.
- *
- * An ireducible control flow graph is one where the usual dominance-based back edge detection does
- * not work, because there is a cycle with multiple entry points, meaning there are
- * mutually-reachable basic blocks where neither dominates the other. For such a graph, we first
- * remove all detectable back-edges using the normal condition that the predecessor block is
- * dominated by the successor block, then mark all edges in a cycle in the resulting graph as back
- * edges.
- */
-private predicate irreducibleSccEdge(SemBasicBlock b1, SemBasicBlock b2) {
-  trimmedEdge(b1, b2) and trimmedEdge+(b2, b1)
-}
-
-private predicate trimmedEdge(SemBasicBlock pred, SemBasicBlock succ) {
-  pred.getASuccessor() = succ and
-  not succ.bbDominates(pred)
-}

cpp/ql/lib/semmle/code/cpp/rangeanalysis/new/internal/semantic/analysis/RangeAnalysisImpl.qll

@@ -72,14 +72,12 @@ module Sem implements Semantic {
 
   class BasicBlock = SemBasicBlock;
 
+  BasicBlock getABasicBlockSuccessor(BasicBlock bb) { result = bb.getASuccessor() }
+
   class Guard = SemGuard;
 
   predicate implies_v2 = semImplies_v2/4;
 
-  predicate guardDirectlyControlsSsaRead = semGuardDirectlyControlsSsaRead/3;
-
-  predicate guardControlsSsaRead = semGuardControlsSsaRead/3;
-
   class Type = SemType;
 
   class IntegerType = SemIntegerType;
@@ -100,8 +98,6 @@ module Sem implements Semantic {
 
   class SsaReadPositionBlock = SemSsaReadPositionBlock;
 
-  predicate backEdge = semBackEdge/3;
-
   predicate conversionCannotOverflow(Type fromType, Type toType) {
     SemanticType::conversionCannotOverflow(fromType, toType)
   }

cpp/ql/lib/semmle/code/cpp/rangeanalysis/new/internal/semantic/analysis/SignAnalysisCommon.qll

@@ -294,7 +294,7 @@ module SignAnalysis<DeltaSig D, UtilSig<Sem, D> Utils> {
   ) {
     exists(boolean testIsTrue, SemRelationalExpr comp |
       pos.hasReadOfVar(v) and
-      semGuardControlsSsaRead(semGetComparisonGuard(comp), pos, testIsTrue) and
+      guardControlsSsaRead(semGetComparisonGuard(comp), pos, testIsTrue) and
       not unknownSign(lowerbound)
     |
       testIsTrue = true and
@@ -318,7 +318,7 @@ module SignAnalysis<DeltaSig D, UtilSig<Sem, D> Utils> {
   ) {
     exists(boolean testIsTrue, SemRelationalExpr comp |
       pos.hasReadOfVar(v) and
-      semGuardControlsSsaRead(semGetComparisonGuard(comp), pos, testIsTrue) and
+      guardControlsSsaRead(semGetComparisonGuard(comp), pos, testIsTrue) and
       not unknownSign(upperbound)
     |
       testIsTrue = true and
@@ -343,7 +343,7 @@ module SignAnalysis<DeltaSig D, UtilSig<Sem, D> Utils> {
   private predicate eqBound(SemExpr eqbound, SemSsaVariable v, SemSsaReadPosition pos, boolean isEq) {
     exists(SemGuard guard, boolean testIsTrue, boolean polarity, SemExpr e |
       pos.hasReadOfVar(pragma[only_bind_into](v)) and
-      semGuardControlsSsaRead(guard, pragma[only_bind_into](pos), testIsTrue) and
+      guardControlsSsaRead(guard, pragma[only_bind_into](pos), testIsTrue) and
       e = ssaRead(pragma[only_bind_into](v), D::fromInt(0)) and
       guard.isEquality(eqbound, e, polarity) and
       isEq = polarity.booleanXor(testIsTrue).booleanNot() and

cpp/ql/src/Likely Bugs/Memory Management/UninitializedLocal.ql

@@ -13,7 +13,8 @@
  */
 
 import cpp
-import semmle.code.cpp.controlflow.StackVariableReachability
+import semmle.code.cpp.ir.IR
+import semmle.code.cpp.ir.dataflow.MustFlow
 
 /**
  * Auxiliary predicate: Types that don't require initialization
@@ -33,31 +34,6 @@ predicate allocatedType(Type t) {
   allocatedType(t.getUnspecifiedType())
 }
 
-/**
- * A declaration of a local variable that leaves the
- * variable uninitialized.
- */
-DeclStmt declWithNoInit(LocalVariable v) {
-  result.getADeclaration() = v and
-  not exists(v.getInitializer()) and
-  /* The type of the variable is not stack-allocated. */
-  exists(Type t | t = v.getType() | not allocatedType(t))
-}
-
-class UninitialisedLocalReachability extends StackVariableReachability {
-  UninitialisedLocalReachability() { this = "UninitialisedLocal" }
-
-  override predicate isSource(ControlFlowNode node, StackVariable v) { node = declWithNoInit(v) }
-
-  override predicate isSink(ControlFlowNode node, StackVariable v) { useOfVarActual(v, node) }
-
-  override predicate isBarrier(ControlFlowNode node, StackVariable v) {
-    // only report the _first_ possibly uninitialized use
-    useOfVarActual(v, node) or
-    definitionBarrier(v, node)
-  }
-}
-
 pragma[noinline]
 predicate containsInlineAssembly(Function f) { exists(AsmStmt s | s.getEnclosingFunction() = f) }
 
@@ -82,8 +58,33 @@ VariableAccess commonException() {
   containsInlineAssembly(result.getEnclosingFunction())
 }
 
-from UninitialisedLocalReachability r, LocalVariable v, VariableAccess va
+predicate isSinkImpl(Instruction sink, VariableAccess va) {
+  exists(LoadInstruction load |
+    va = load.getUnconvertedResultExpression() and
+    not va = commonException() and
+    sink = load.getSourceValue()
+  )
+}
+
+class MustFlow extends MustFlowConfiguration {
+  MustFlow() { this = "MustFlow" }
+
+  override predicate isSource(Instruction source) {
+    source instanceof UninitializedInstruction and
+    exists(Type t | t = source.getResultType() | not allocatedType(t))
+  }
+
+  override predicate isSink(Operand sink) { isSinkImpl(sink.getDef(), _) }
+
+  override predicate allowInterproceduralFlow() { none() }
+
+  override predicate isBarrier(Instruction instr) { instr instanceof ChiInstruction }
+}
+
+from
+  VariableAccess va, LocalVariable v, MustFlow conf, MustFlowPathNode source, MustFlowPathNode sink
 where
-  r.reaches(_, v, va) and
-  not va = commonException()
+  conf.hasFlowPath(source, sink) and
+  isSinkImpl(sink.getInstruction(), va) and
+  v = va.getTarget()
 select va, "The variable $@ may not be initialized at this access.", v, v.getName()

cpp/ql/src/change-notes/2023-11-07-uninitialized-local.md

@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* The `cpp/uninitialized-local` query has been improved to produce fewer false positives.

cpp/ql/test/library-tests/ir/range-analysis/test.cpp

@@ -134,6 +134,12 @@ void test_div(int x) {
 struct X { int n; };
 void read_argument(const X *);
 
+// This test exists purely to ensure that modulus analysis terminates in the
+// presence of inexact phi operands. The LoadInstruction on `while(x->n) { ... }`
+// reads from a PhiInstruction with two input operands: an exact operand defined
+// by the StoreInstruction generated by `x->n--` and an inexact operand coming
+// from the WriteSideEffect generated by `read_argument(x)`. If we don't consider
+// the inexact operand modulus analysis fails to terminate.
 void nonterminating_without_operands_as_ssa(X *x) {
   read_argument(x);
   while (x->n) {