Merge branch 'main' into deduplicate-dataflow-results-take-3

Author: MathiasVP

cpp/autobuilder/Semmle.Autobuild.Cpp.Tests/Semmle.Autobuild.Cpp.Tests.csproj

@@ -11,12 +11,12 @@
   <ItemGroup>
     <PackageReference Include="System.IO.FileSystem" Version="4.3.0" />
     <PackageReference Include="System.IO.FileSystem.Primitives" Version="4.3.0" />
-    <PackageReference Include="xunit" Version="2.5.0" />
-    <PackageReference Include="xunit.runner.visualstudio" Version="2.5.0">
+    <PackageReference Include="xunit" Version="2.4.2" />
+    <PackageReference Include="xunit.runner.visualstudio" Version="2.4.5">
       <PrivateAssets>all</PrivateAssets>
       <IncludeAssets>runtime; build; native; contentfiles; analyzers</IncludeAssets>
     </PackageReference>
-    <PackageReference Include="Microsoft.NET.Test.Sdk" Version="17.7.1" />
+    <PackageReference Include="Microsoft.NET.Test.Sdk" Version="17.4.0" />
   </ItemGroup>
 
   <ItemGroup>

cpp/autobuilder/Semmle.Autobuild.Cpp/Semmle.Autobuild.Cpp.csproj

@@ -17,7 +17,7 @@
   </ItemGroup>
 
   <ItemGroup>
-    <PackageReference Include="Microsoft.Build" Version="17.7.2" />
+    <PackageReference Include="Microsoft.Build" Version="17.3.2" />
   </ItemGroup>
 
   <ItemGroup>

cpp/ql/lib/change-notes/2023-09-06-as-defining-argument-off-by-one-fix.md

@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* The `DataFlow::asDefiningArgument` predicate now takes its argument from the range starting at `1` instead of `2`. Queries that depend on the single-parameter version of `DataFlow::asDefiningArgument` should have their arguments updated accordingly.

cpp/ql/lib/change-notes/2023-09-07-return-from-end.md

@@ -0,0 +1,5 @@
+---
+category: minorAnalysis
+---
+* Treat functions that reach the end of the function as returning in the IR.
+  They used to be treated as unreachable but it is allowed in C. 

cpp/ql/lib/change-notes/2023-09-08-more-unreachble.md

@@ -0,0 +1,5 @@
+---
+category: minorAnalysis
+---
+* Functions that do not return due to calling functions that don't return (e.g. `exit`) are now detected as
+ non-returning in the IR and dataflow.

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

@@ -275,9 +275,7 @@ class Node extends TIRDataFlowNode {
    * after the `f` has returned.
    */
   Expr asDefiningArgument(int index) {
-    // Subtract one because `DefinitionByReferenceNode` is defined to be in
-    // the range `[0 ... n - 1]` for some `n` instead of `[1 ... n]`.
-    this.(DefinitionByReferenceNode).getIndirectionIndex() = index - 1 and
+    this.(DefinitionByReferenceNode).getIndirectionIndex() = index and
     result = this.(DefinitionByReferenceNode).getArgument()
   }
 

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

@@ -405,9 +405,6 @@ predicate hasUnreachedInstruction(IRFunction func) {
   exists(Call c |
     c.getEnclosingFunction() = func.getFunction() and
     any(Options opt).exits(c.getTarget())
-  ) and
-  not exists(TranslatedUnreachableReturnStmt return |
-    return.getEnclosingFunction().getFunction() = func.getFunction()
   )
 }
 

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

@@ -442,29 +442,26 @@ class TranslatedReturnVoidStmt extends TranslatedReturnStmt {
 
 /**
  * The IR translation of an implicit `return` statement generated by the extractor to handle control
- * flow that reaches the end of a non-`void`-returning function body. Since such control flow
- * produces undefined behavior, we simply generate an `Unreached` instruction to prevent that flow
- * from continuing on to pollute other analysis. The assumption is that the developer is certain
- * that the implicit `return` is unreachable, even if the compiler cannot prove it.
+ * flow that reaches the end of a non-`void`-returning function body. Such control flow
+ * produces undefined behavior in C++ but not in C. However even in C using the return value is
+ * undefined behaviour. We make it return uninitialized memory to get as much flow as possible.
  */
-class TranslatedUnreachableReturnStmt extends TranslatedReturnStmt {
-  TranslatedUnreachableReturnStmt() {
+class TranslatedNoValueReturnStmt extends TranslatedReturnStmt, TranslatedVariableInitialization {
+  TranslatedNoValueReturnStmt() {
     not stmt.hasExpr() and hasReturnValue(stmt.getEnclosingFunction())
   }
 
-  override TranslatedElement getChild(int id) { none() }
-
-  override Instruction getFirstInstruction() { result = this.getInstruction(OnlyInstructionTag()) }
-
-  override predicate hasInstruction(Opcode opcode, InstructionTag tag, CppType resultType) {
-    tag = OnlyInstructionTag() and
-    opcode instanceof Opcode::Unreached and
-    resultType = getVoidType()
+  final override Instruction getInitializationSuccessor() {
+    result = this.getEnclosingFunction().getReturnSuccessorInstruction()
   }
 
-  override Instruction getInstructionSuccessor(InstructionTag tag, EdgeKind kind) { none() }
+  final override Type getTargetType() { result = this.getEnclosingFunction().getReturnType() }
 
-  override Instruction getChildSuccessor(TranslatedElement child) { none() }
+  final override TranslatedInitialization getInitialization() { none() }
+
+  final override IRVariable getIRVariable() {
+    result = this.getEnclosingFunction().getReturnVariable()
+  }
 }
 
 /**

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

@@ -10,6 +10,65 @@ predicate isInfeasibleInstructionSuccessor(Instruction instr, EdgeKind kind) {
   or
   instr.getSuccessor(kind) instanceof UnreachedInstruction and
   kind instanceof GotoEdge
+  or
+  isCallToNonReturningFunction(instr) and exists(instr.getSuccessor(kind))
+}
+
+/**
+ * Holds if all calls to `f` never return (e.g. they call `exit` or loop forever)
+ */
+private predicate isNonReturningFunction(IRFunction f) {
+  // If the function has an instruction with a missing successor then
+  // the analysis is probably going to be incorrect, so assume they exit.
+  not hasInstructionWithMissingSuccessor(f) and
+  (
+    // If all flows to the exit block are pass through an unreachable then f never returns.
+    any(UnreachedInstruction instr).getBlock().postDominates(f.getEntryBlock())
+    or
+    // If there is no flow to the exit block then f never returns.
+    not exists(IRBlock entry, IRBlock exit |
+      exit = f.getExitFunctionInstruction().getBlock() and
+      entry = f.getEntryBlock() and
+      exit = entry.getASuccessor*()
+    )
+    or
+    // If all flows to the exit block are pass through a call that never returns then f never returns.
+    exists(CallInstruction ci |
+      ci.getBlock().postDominates(f.getEntryBlock()) and
+      isCallToNonReturningFunction(ci)
+    )
+  )
+}
+
+/**
+ * Holds if `f` has an instruction with a missing successor.
+ * This matches `instructionWithoutSuccessor` from `IRConsistency`, but
+ * avoids generating the error strings.
+ */
+predicate hasInstructionWithMissingSuccessor(IRFunction f) {
+  exists(Instruction missingSucc |
+    missingSucc.getEnclosingIRFunction() = f and
+    not exists(missingSucc.getASuccessor()) and
+    not missingSucc instanceof ExitFunctionInstruction and
+    // Phi instructions aren't linked into the instruction-level flow graph.
+    not missingSucc instanceof PhiInstruction and
+    not missingSucc instanceof UnreachedInstruction
+  )
+}
+
+/**
+ * Holds if the call `ci` never returns.
+ */
+private predicate isCallToNonReturningFunction(CallInstruction ci) {
+  exists(IRFunction callee, Language::Function staticTarget |
+    staticTarget = ci.getStaticCallTarget() and
+    staticTarget = callee.getFunction() and
+    // We can't easily tell if the call is virtual or not
+    // if the callee is virtual. So assume that the call is virtual
+    // if the target is.
+    not staticTarget.isVirtual() and
+    isNonReturningFunction(callee)
+  )
 }
 
 pragma[noinline]

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

@@ -1,2 +1,3 @@
 import semmle.code.cpp.ir.implementation.raw.IR as IR
 import semmle.code.cpp.ir.implementation.raw.constant.ConstantAnalysis as ConstantAnalysis
+import semmle.code.cpp.ir.internal.IRCppLanguage as Language