Merge pull request #16915 from MathiasVP/fix-iterator-to-expired-container-fp-4

C++: Fix `cpp/iterator-to-expired-container` FPs

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

@@ -104,7 +104,7 @@ predicate hasRawIndirectInstruction(Instruction instr, int indirectionIndex) {
 
 cached
 private newtype TDefImpl =
-  TDefAddressImpl(BaseIRVariable v) or
+  TDefAddressImpl(BaseSourceVariable v) or
   TDirectDefImpl(Operand address, int indirectionIndex) {
     isDef(_, _, address, _, _, indirectionIndex)
   } or
@@ -325,9 +325,9 @@ private Instruction getInitializationTargetAddress(IRVariable v) {
   )
 }
 
-/** An initial definition of an `IRVariable`'s address. */
-private class DefAddressImpl extends DefImpl, TDefAddressImpl {
-  BaseIRVariable v;
+/** An initial definition of an SSA variable address. */
+abstract private class DefAddressImpl extends DefImpl, TDefAddressImpl {
+  BaseSourceVariable v;
 
   DefAddressImpl() {
     this = TDefAddressImpl(v) and
@@ -342,6 +342,19 @@ private class DefAddressImpl extends DefImpl, TDefAddressImpl {
 
   final override Node0Impl getValue() { none() }
 
+  override Cpp::Location getLocation() { result = v.getLocation() }
+
+  final override SourceVariable getSourceVariable() {
+    result.getBaseVariable() = v and
+    result.getIndirection() = 0
+  }
+
+  final override BaseSourceVariable getBaseSourceVariable() { result = v }
+}
+
+private class DefVariableAddressImpl extends DefAddressImpl {
+  override BaseIRVariable v;
+
   final override predicate hasIndexInBlock(IRBlock block, int index) {
     exists(IRVariable var | var = v.getIRVariable() |
       block.getInstruction(index) = getInitializationTargetAddress(var)
@@ -353,15 +366,14 @@ private class DefAddressImpl extends DefImpl, TDefAddressImpl {
       index = 0
     )
   }
+}
 
-  override Cpp::Location getLocation() { result = v.getIRVariable().getLocation() }
+private class DefCallAddressImpl extends DefAddressImpl {
+  override BaseCallVariable v;
 
-  final override SourceVariable getSourceVariable() {
-    result.getBaseVariable() = v and
-    result.getIndirection() = 0
+  final override predicate hasIndexInBlock(IRBlock block, int index) {
+    block.getInstruction(index) = v.getCallInstruction()
   }
-
-  final override BaseSourceVariable getBaseSourceVariable() { result = v }
 }
 
 private class DirectDef extends DefImpl, TDirectDefImpl {

cpp/ql/test/library-tests/dataflow/calls-as-ssa-variables/test.cpp

@@ -0,0 +1,29 @@
+namespace std
+{
+  struct ptrdiff_t;
+  struct input_iterator_tag
+  {
+  };
+  struct forward_iterator_tag : public input_iterator_tag
+  {
+  };
+}
+
+struct A
+{
+  using value_type = int;
+  using difference_type = std::ptrdiff_t;
+  using pointer = int*;
+  using reference = int&;
+  using iterator_category = std::forward_iterator_tag;
+};
+
+A get();
+
+void test()
+{
+  while (true)
+  {
+    auto &&x = get();
+  }
+}

cpp/ql/test/library-tests/dataflow/calls-as-ssa-variables/test.expected

@@ -0,0 +1,4 @@
+edges
+nodes
+subpaths
+#select

cpp/ql/test/library-tests/dataflow/calls-as-ssa-variables/test.ql

@@ -0,0 +1,23 @@
+/**
+ * @kind path-problem
+ */
+
+import semmle.code.cpp.ir.IR
+import semmle.code.cpp.dataflow.new.DataFlow
+import Flow::PathGraph
+
+module Config implements DataFlow::ConfigSig {
+  predicate isSource(DataFlow::Node source) {
+    source.asInstruction().(VariableAddressInstruction).getIRVariable() instanceof IRTempVariable
+  }
+
+  predicate isSink(DataFlow::Node sink) {
+    sink.asInstruction().(CallInstruction).getStaticCallTarget().hasName("get")
+  }
+}
+
+module Flow = DataFlow::Global<Config>;
+
+from Flow::PathNode source, Flow::PathNode sink
+where Flow::flowPath(source, sink)
+select sink.getNode(), source, sink, ""

cpp/ql/test/query-tests/Security/CWE/CWE-416/semmle/tests/IteratorToExpiredContainer/IteratorToExpiredContainer.expected

@@ -3,4 +3,3 @@
 | test.cpp:702:27:702:27 | call to operator[] | This object is destroyed at the end of the full-expression. |
 | test.cpp:727:23:727:23 | call to operator[] | This object is destroyed at the end of the full-expression. |
 | test.cpp:735:23:735:23 | call to operator[] | This object is destroyed at the end of the full-expression. |
-| test.cpp:826:25:826:43 | pointer to ~HasBeginAndEnd output argument | This object is destroyed at the end of the full-expression. |

cpp/ql/test/query-tests/Security/CWE/CWE-416/semmle/tests/IteratorToExpiredContainer/test.cpp

@@ -823,7 +823,7 @@ void test6()
 {
   while(getBool())
   {
-    for (const int& x : getHasBeginAndEnd()) // GOOD [FALSE POSITIVE]
+    for (const int& x : getHasBeginAndEnd()) // GOOD
     {
     }
   }