Merge pull request #16981 from MathiasVP/phi-escape-5-follow-up-2

C++: Alias analysis follow-up to #16907
2026-04-21 15:05:56 +02:00 · 2024-07-25 11:59:44 +01:00
parent 032ae9e1e7 b3bffb6826
commit c5da43e691
11 changed files with 141 additions and 14 deletions
--- a/cpp/ql/lib/semmle/code/cpp/ir/implementation/aliased_ssa/internal/AliasAnalysis.qll
+++ b/cpp/ql/lib/semmle/code/cpp/ir/implementation/aliased_ssa/internal/AliasAnalysis.qll
@@ -338,6 +338,56 @@ private predicate resultEscapesNonReturn(Instruction instr) {
  not instr.isResultModeled()
 }

+/** Holds if `operand` may (transitively) flow to an `AddressOperand`. */
+private predicate consumedAsAddressOperand(Operand operand) {
+  operand instanceof AddressOperand
+  or
+  exists(Operand address |
+    consumedAsAddressOperand(address) and
+    operandIsPropagated(operand, _, address.getDef())
+  )
+}
+
+/**
+ * Holds if `operand` may originate from a base instruction of an allocation,
+ * and that operand may transitively flow to an `AddressOperand`.
+ */
+private predicate propagatedFromAllocationBase(Operand operand, Configuration::Allocation allocation) {
+  consumedAsAddressOperand(operand) and
+  (
+    not exists(Configuration::getOldAllocation(allocation)) and
+    operand.getDef() = allocation.getABaseInstruction()
+    or
+    exists(Operand address |
+      operandIsPropagated(address, _, operand.getDef()) and
+      propagatedFromAllocationBase(address, allocation)
+    )
+  )
+}
+
+private predicate propagatedFromNonAllocationBase(Operand operand) {
+  exists(Instruction def |
+    def = operand.getDef() and
+    not operandIsPropagated(_, _, def) and
+    not def = any(Configuration::Allocation allocation).getABaseInstruction()
+  )
+  or
+  exists(Operand address |
+    operandIsPropagated(address, _, operand.getDef()) and
+    propagatedFromNonAllocationBase(address)
+  )
+}
+
+/**
+ * Holds if we cannot see all producers of an operand for which allocation also flows into.
+ */
+private predicate operandConsumesEscaped(Configuration::Allocation allocation) {
+  exists(AddressOperand address |
+    propagatedFromAllocationBase(address, allocation) and
+    propagatedFromNonAllocationBase(address)
+  )
+}
+
 /**
 * Holds if the address of `allocation` escapes outside the domain of the analysis. This can occur
 * either because the allocation's address is taken within the function and escapes, or because the
@@ -346,12 +396,14 @@ private predicate resultEscapesNonReturn(Instruction instr) {
 predicate allocationEscapes(Configuration::Allocation allocation) {
  allocation.alwaysEscapes()
  or
-  exists(IREscapeAnalysisConfiguration config |
-    config.useSoundEscapeAnalysis() and resultEscapesNonReturn(allocation.getABaseInstruction())
+  exists(IREscapeAnalysisConfiguration config | config.useSoundEscapeAnalysis() |
+    resultEscapesNonReturn(allocation.getABaseInstruction())
+    or
+    operandConsumesEscaped(allocation)
  )
  or
  Configuration::phaseNeedsSoundEscapeAnalysis() and
-  resultEscapesNonReturn(allocation.getABaseInstruction())
+  (resultEscapesNonReturn(allocation.getABaseInstruction()) or operandConsumesEscaped(allocation))
 }

 /**
--- a/cpp/ql/lib/semmle/code/cpp/ir/implementation/aliased_ssa/internal/AliasConfiguration.qll
+++ b/cpp/ql/lib/semmle/code/cpp/ir/implementation/aliased_ssa/internal/AliasConfiguration.qll
@@ -146,3 +146,8 @@ class DynamicAllocation extends Allocation, TDynamicAllocation {
 }

 predicate phaseNeedsSoundEscapeAnalysis() { none() }
+
+UnaliasedSsa::Allocation getOldAllocation(VariableAllocation allocation) {
+  UnaliasedSsa::canReuseSsaForVariable(allocation.getIRVariable()) and
+  result = allocation.getIRVariable()
+}
--- a/cpp/ql/lib/semmle/code/cpp/ir/implementation/aliased_ssa/internal/AliasedSSA.qll
+++ b/cpp/ql/lib/semmle/code/cpp/ir/implementation/aliased_ssa/internal/AliasedSSA.qll
@@ -7,7 +7,7 @@ private import semmle.code.cpp.ir.implementation.unaliased_ssa.internal.SSAConst
 private import semmle.code.cpp.ir.internal.IntegerConstant as Ints
 private import semmle.code.cpp.ir.internal.IntegerInterval as Interval
 private import semmle.code.cpp.ir.implementation.internal.OperandTag
-private import AliasConfiguration
+import AliasConfiguration
 private import codeql.util.Boolean

 private class IntValue = Ints::IntValue;
--- a/cpp/ql/lib/semmle/code/cpp/ir/implementation/unaliased_ssa/internal/AliasAnalysis.qll
+++ b/cpp/ql/lib/semmle/code/cpp/ir/implementation/unaliased_ssa/internal/AliasAnalysis.qll
@@ -338,6 +338,56 @@ private predicate resultEscapesNonReturn(Instruction instr) {
  not instr.isResultModeled()
 }

+/** Holds if `operand` may (transitively) flow to an `AddressOperand`. */
+private predicate consumedAsAddressOperand(Operand operand) {
+  operand instanceof AddressOperand
+  or
+  exists(Operand address |
+    consumedAsAddressOperand(address) and
+    operandIsPropagated(operand, _, address.getDef())
+  )
+}
+
+/**
+ * Holds if `operand` may originate from a base instruction of an allocation,
+ * and that operand may transitively flow to an `AddressOperand`.
+ */
+private predicate propagatedFromAllocationBase(Operand operand, Configuration::Allocation allocation) {
+  consumedAsAddressOperand(operand) and
+  (
+    not exists(Configuration::getOldAllocation(allocation)) and
+    operand.getDef() = allocation.getABaseInstruction()
+    or
+    exists(Operand address |
+      operandIsPropagated(address, _, operand.getDef()) and
+      propagatedFromAllocationBase(address, allocation)
+    )
+  )
+}
+
+private predicate propagatedFromNonAllocationBase(Operand operand) {
+  exists(Instruction def |
+    def = operand.getDef() and
+    not operandIsPropagated(_, _, def) and
+    not def = any(Configuration::Allocation allocation).getABaseInstruction()
+  )
+  or
+  exists(Operand address |
+    operandIsPropagated(address, _, operand.getDef()) and
+    propagatedFromNonAllocationBase(address)
+  )
+}
+
+/**
+ * Holds if we cannot see all producers of an operand for which allocation also flows into.
+ */
+private predicate operandConsumesEscaped(Configuration::Allocation allocation) {
+  exists(AddressOperand address |
+    propagatedFromAllocationBase(address, allocation) and
+    propagatedFromNonAllocationBase(address)
+  )
+}
+
 /**
 * Holds if the address of `allocation` escapes outside the domain of the analysis. This can occur
 * either because the allocation's address is taken within the function and escapes, or because the
@@ -346,12 +396,14 @@ private predicate resultEscapesNonReturn(Instruction instr) {
 predicate allocationEscapes(Configuration::Allocation allocation) {
  allocation.alwaysEscapes()
  or
-  exists(IREscapeAnalysisConfiguration config |
-    config.useSoundEscapeAnalysis() and resultEscapesNonReturn(allocation.getABaseInstruction())
+  exists(IREscapeAnalysisConfiguration config | config.useSoundEscapeAnalysis() |
+    resultEscapesNonReturn(allocation.getABaseInstruction())
+    or
+    operandConsumesEscaped(allocation)
  )
  or
  Configuration::phaseNeedsSoundEscapeAnalysis() and
-  resultEscapesNonReturn(allocation.getABaseInstruction())
+  (resultEscapesNonReturn(allocation.getABaseInstruction()) or operandConsumesEscaped(allocation))
 }

 /**
--- a/cpp/ql/lib/semmle/code/cpp/ir/implementation/unaliased_ssa/internal/AliasConfiguration.qll
+++ b/cpp/ql/lib/semmle/code/cpp/ir/implementation/unaliased_ssa/internal/AliasConfiguration.qll
@@ -1,4 +1,5 @@
 private import AliasConfigurationImports
+private import codeql.util.Unit

 /**
 * A memory allocation that can be tracked by the SimpleSSA alias analysis.
@@ -16,3 +17,5 @@ class Allocation extends IRAutomaticVariable {
 }

 predicate phaseNeedsSoundEscapeAnalysis() { any() }
+
+Unit getOldAllocation(Allocation allocation) { any() }
--- a/cpp/ql/lib/semmle/code/cpp/ir/implementation/unaliased_ssa/internal/SimpleSSA.qll
+++ b/cpp/ql/lib/semmle/code/cpp/ir/implementation/unaliased_ssa/internal/SimpleSSA.qll
@@ -1,7 +1,7 @@
 import AliasAnalysis
 private import SimpleSSAImports
 import SimpleSSAPublicImports
-private import AliasConfiguration
+import AliasConfiguration
 private import codeql.util.Unit

 private predicate isTotalAccess(Allocation var, AddressOperand addrOperand, IRType type) {
--- a/cpp/ql/test/library-tests/ir/ir/aliased_ir.expected
+++ b/cpp/ql/test/library-tests/ir/ir/aliased_ir.expected
@@ -18779,6 +18779,7 @@ ir.cpp:
 # 2667|     r2667_1(glval<int>)   = VariableAddress[intBuffer] : 
 # 2667|     r2667_2(int)          = Constant[8]                : 
 # 2667|     m2667_3(int)          = Store[intBuffer]           : &:r2667_1, r2667_2
+# 2667|     m2667_4(unknown)      = Chi                        : total:m2663_4, partial:m2667_3
 # 2668|     r2668_1(glval<int>)   = VariableAddress[intBuffer] : 
 # 2668|     r2668_2(int *)        = CopyValue                  : r2668_1
 # 2668|     r2668_3(glval<int *>) = VariableAddress[data]      : 
@@ -18786,15 +18787,15 @@ ir.cpp:
 #-----|   Goto -> Block 2

 # 2670|   Block 2
-# 2670|     m2670_1(int)            = Phi                      : from 1:m2667_3
+# 2670|     m2670_1(unknown)        = Phi                      : from 0:~m2663_4, from 1:~m2667_4
 # 2670|     m2670_2(int *)          = Phi                      : from 0:m2665_3, from 1:m2668_4
 # 2670|     r2670_3(glval<unknown>) = FunctionAddress[use_int] : 
 # 2670|     r2670_4(glval<int *>)   = VariableAddress[data]    : 
 # 2670|     r2670_5(int *)          = Load[data]               : &:r2670_4, m2670_2
 # 2670|     r2670_6(int)            = Load[?]                  : &:r2670_5, ~m2670_1
 # 2670|     v2670_7(void)           = Call[use_int]            : func:r2670_3, 0:r2670_6
-# 2670|     m2670_8(unknown)        = ^CallSideEffect          : ~m2663_4
-# 2670|     m2670_9(unknown)        = Chi                      : total:m2663_4, partial:m2670_8
+# 2670|     m2670_8(unknown)        = ^CallSideEffect          : ~m2670_1
+# 2670|     m2670_9(unknown)        = Chi                      : total:m2670_1, partial:m2670_8
 # 2671|     v2671_1(void)           = NoOp                     : 
 # 2663|     v2663_7(void)           = ReturnVoid               : 
 # 2663|     v2663_8(void)           = AliasedUse               : ~m2670_9
--- a/cpp/ql/test/library-tests/ir/ir/aliased_ssa_consistency.expected
+++ b/cpp/ql/test/library-tests/ir/ir/aliased_ssa_consistency.expected
@@ -2,7 +2,6 @@ missingOperand
 unexpectedOperand
 duplicateOperand
 missingPhiOperand
-| ir.cpp:2670:3:2670:9 | Phi: call to use_int | Instruction 'Phi: call to use_int' is missing an operand for predecessor block 'EnterFunction: phi_with_single_input_at_merge' in function '$@'. | ir.cpp:2663:13:2663:42 | void phi_with_single_input_at_merge(bool) | void phi_with_single_input_at_merge(bool) |
 missingOperandType
 duplicateChiOperand
 sideEffectWithoutPrimary
--- a/cpp/ql/test/library-tests/ir/ir/aliased_ssa_consistency_unsound.expected
+++ b/cpp/ql/test/library-tests/ir/ir/aliased_ssa_consistency_unsound.expected
@@ -2,7 +2,6 @@ missingOperand
 unexpectedOperand
 duplicateOperand
 missingPhiOperand
-| ir.cpp:2670:3:2670:9 | Phi: call to use_int | Instruction 'Phi: call to use_int' is missing an operand for predecessor block 'EnterFunction: phi_with_single_input_at_merge' in function '$@'. | ir.cpp:2663:13:2663:42 | void phi_with_single_input_at_merge(bool) | void phi_with_single_input_at_merge(bool) |
 missingOperandType
 duplicateChiOperand
 sideEffectWithoutPrimary
--- a/cpp/ql/test/query-tests/Security/CWE/CWE-457/semmle/tests/LoopConditionsConst.expected
+++ b/cpp/ql/test/query-tests/Security/CWE/CWE-457/semmle/tests/LoopConditionsConst.expected
@@ -23,3 +23,4 @@
 | test.cpp:424:2:425:2 | for(...;...;...) ... | test.cpp:424:18:424:23 | ... < ... | 1 | i | { ... } | i | return ... |
 | test.cpp:433:2:434:2 | for(...;...;...) ... | test.cpp:433:18:433:22 | 0 | 0 |  | { ... } | 0 | return ... |
 | test.cpp:559:3:564:3 | while (...) ... | test.cpp:559:9:559:15 | call to getBool |  | call to getBool | { ... } | call to getBool | ExprStmt |
+| test.cpp:574:3:579:3 | while (...) ... | test.cpp:574:10:574:16 | call to getBool |  | call to getBool | { ... } | call to getBool | ExprStmt |
--- a/cpp/ql/test/query-tests/Security/CWE/CWE-457/semmle/tests/test.cpp
+++ b/cpp/ql/test/query-tests/Security/CWE/CWE-457/semmle/tests/test.cpp
@@ -565,4 +565,19 @@ void test45() {

  *rP = NULL;
  use(r); // GOOD
-}
+}
+
+void test46()
+{
+  LinkedList *r, **rP = &r;
+
+  while (getBool())
+  {
+    LinkedList *s = nullptr;
+    *rP = s;
+    rP = &s->next;
+  }
+
+  *rP = nullptr;
+  use(r);
+}