Merge pull request github#14736 from MathiasVP/fix-global-indirect-flow

C++: Fix indirect global-variable flow

Author: MathiasVP

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

@@ -632,20 +632,20 @@ predicate jumpStep(Node n1, Node n2) {
       v = globalUse.getVariable() and
       n1.(FinalGlobalValue).getGlobalUse() = globalUse
     |
-      globalUse.getIndirectionIndex() = 1 and
+      globalUse.getIndirection() = 1 and
       v = n2.asVariable()
       or
-      v = n2.asIndirectVariable(globalUse.getIndirectionIndex())
+      v = n2.asIndirectVariable(globalUse.getIndirection())
     )
     or
     exists(Ssa::GlobalDef globalDef |
       v = globalDef.getVariable() and
       n2.(InitialGlobalValue).getGlobalDef() = globalDef
     |
-      globalDef.getIndirectionIndex() = 1 and
+      globalDef.getIndirection() = 1 and
       v = n1.asVariable()
       or
-      v = n1.asIndirectVariable(globalDef.getIndirectionIndex())
+      v = n1.asIndirectVariable(globalDef.getIndirection())
     )
   )
 }

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

@@ -113,22 +113,12 @@ private newtype TDefOrUseImpl =
   TGlobalUse(GlobalLikeVariable v, IRFunction f, int indirectionIndex) {
     // Represents a final "use" of a global variable to ensure that
     // the assignment to a global variable isn't ruled out as dead.
-    exists(VariableAddressInstruction vai, int defIndex |
-      vai.getEnclosingIRFunction() = f and
-      vai.getAstVariable() = v and
-      isDef(_, _, _, vai, _, defIndex) and
-      indirectionIndex = [0 .. defIndex] + 1
-    )
+    isGlobalUse(v, f, _, indirectionIndex)
   } or
   TGlobalDefImpl(GlobalLikeVariable v, IRFunction f, int indirectionIndex) {
     // Represents the initial "definition" of a global variable when entering
     // a function body.
-    exists(VariableAddressInstruction vai |
-      vai.getEnclosingIRFunction() = f and
-      vai.getAstVariable() = v and
-      isUse(_, _, vai, _, indirectionIndex) and
-      not isDef(_, _, vai.getAUse(), _, _, _)
-    )
+    isGlobalDefImpl(v, f, _, indirectionIndex)
   } or
   TIteratorDef(
     Operand iteratorDerefAddress, BaseSourceVariableInstruction container, int indirectionIndex
@@ -150,6 +140,27 @@ private newtype TDefOrUseImpl =
     )
   }
 
+private predicate isGlobalUse(
+  GlobalLikeVariable v, IRFunction f, int indirection, int indirectionIndex
+) {
+  exists(VariableAddressInstruction vai |
+    vai.getEnclosingIRFunction() = f and
+    vai.getAstVariable() = v and
+    isDef(_, _, _, vai, indirection, indirectionIndex)
+  )
+}
+
+private predicate isGlobalDefImpl(
+  GlobalLikeVariable v, IRFunction f, int indirection, int indirectionIndex
+) {
+  exists(VariableAddressInstruction vai |
+    vai.getEnclosingIRFunction() = f and
+    vai.getAstVariable() = v and
+    isUse(_, _, vai, indirection, indirectionIndex) and
+    not isDef(_, _, _, vai, _, indirectionIndex)
+  )
+}
+
 private predicate unspecifiedTypeIsModifiableAt(Type unspecified, int indirectionIndex) {
   indirectionIndex = [1 .. getIndirectionForUnspecifiedType(unspecified).getNumberOfIndirections()] and
   exists(CppType cppType |
@@ -438,7 +449,7 @@ class GlobalUse extends UseImpl, TGlobalUse {
 
   override FinalGlobalValue getNode() { result.getGlobalUse() = this }
 
-  override int getIndirection() { result = ind + 1 }
+  override int getIndirection() { isGlobalUse(global, f, result, ind) }
 
   /** Gets the global variable associated with this use. */
   GlobalLikeVariable getVariable() { result = global }
@@ -460,7 +471,9 @@ class GlobalUse extends UseImpl, TGlobalUse {
     )
   }
 
-  override SourceVariable getSourceVariable() { sourceVariableIsGlobal(result, global, f, ind) }
+  override SourceVariable getSourceVariable() {
+    sourceVariableIsGlobal(result, global, f, this.getIndirection())
+  }
 
   final override Cpp::Location getLocation() { result = f.getLocation() }
 
@@ -501,16 +514,18 @@ class GlobalDefImpl extends DefOrUseImpl, TGlobalDefImpl {
 
   /** Gets the global variable associated with this definition. */
   override SourceVariable getSourceVariable() {
-    sourceVariableIsGlobal(result, global, f, indirectionIndex)
+    sourceVariableIsGlobal(result, global, f, this.getIndirection())
   }
 
+  int getIndirection() { result = indirectionIndex }
+
   /**
    * Gets the type of this use after specifiers have been deeply stripped
    * and typedefs have been resolved.
    */
   Type getUnspecifiedType() { result = global.getUnspecifiedType() }
 
-  override string toString() { result = "GlobalDef" }
+  override string toString() { result = "Def of " + this.getSourceVariable() }
 
   override Location getLocation() { result = f.getLocation() }
 
@@ -980,7 +995,7 @@ class GlobalDef extends TGlobalDef, SsaDefOrUse {
   final override Location getLocation() { result = global.getLocation() }
 
   /** Gets a textual representation of this definition. */
-  override string toString() { result = "GlobalDef" }
+  override string toString() { result = global.toString() }
 
   /**
    * Holds if this definition has index `index` in block `block`, and
@@ -990,6 +1005,9 @@ class GlobalDef extends TGlobalDef, SsaDefOrUse {
     global.hasIndexInBlock(block, index, sv)
   }
 
+  /** Gets the indirection index of this definition. */
+  int getIndirection() { result = global.getIndirection() }
+
   /** Gets the indirection index of this definition. */
   int getIndirectionIndex() { result = global.getIndirectionIndex() }
 

cpp/ql/test/library-tests/dataflow/dataflow-tests/dataflow-consistency.expected

@@ -23,6 +23,7 @@ argHasPostUpdate
 | lambdas.cpp:38:2:38:2 | d | ArgumentNode is missing PostUpdateNode. |
 | lambdas.cpp:45:2:45:2 | e | ArgumentNode is missing PostUpdateNode. |
 | test.cpp:67:29:67:35 | source1 | ArgumentNode is missing PostUpdateNode. |
+| test.cpp:813:19:813:35 | * ... | ArgumentNode is missing PostUpdateNode. |
 postWithInFlow
 | BarrierGuard.cpp:49:6:49:6 | x [post update] | PostUpdateNode should not be the target of local flow. |
 | BarrierGuard.cpp:60:7:60:7 | x [post update] | PostUpdateNode should not be the target of local flow. |
@@ -136,6 +137,9 @@ postWithInFlow
 | test.cpp:728:3:728:4 | * ... [post update] | PostUpdateNode should not be the target of local flow. |
 | test.cpp:728:4:728:4 | p [inner post update] | PostUpdateNode should not be the target of local flow. |
 | test.cpp:734:41:734:41 | x [inner post update] | PostUpdateNode should not be the target of local flow. |
+| test.cpp:808:5:808:21 | * ... [post update] | PostUpdateNode should not be the target of local flow. |
+| test.cpp:808:6:808:21 | global_indirect1 [inner post update] | PostUpdateNode should not be the target of local flow. |
+| test.cpp:832:5:832:17 | global_direct [post update] | PostUpdateNode should not be the target of local flow. |
 viableImplInCallContextTooLarge
 uniqueParameterNodeAtPosition
 uniqueParameterNodePosition

cpp/ql/test/library-tests/dataflow/dataflow-tests/test.cpp

@@ -796,4 +796,44 @@ void test() {
   MyStruct a;
   intPointerSource(a.content, a.content);
   indirect_sink(a.content); // $ ast ir
+}
+
+namespace MoreGlobalTests {
+  int **global_indirect1;
+  int **global_indirect2;
+  int **global_direct;
+
+  void set_indirect1()
+  {
+    *global_indirect1 = indirect_source();
+  }
+
+  void read_indirect1() {
+    sink(global_indirect1); // clean
+    indirect_sink(*global_indirect1); // $ ir MISSING: ast
+  }
+
+  void set_indirect2()
+  {
+    **global_indirect2 = source();
+  }
+
+  void read_indirect2() {
+    sink(global_indirect2); // clean
+    sink(**global_indirect2); // $ ir MISSING: ast
+  }
+
+  // overload source with a boolean parameter so
+  // that we can define a variant that return an int**.
+  int** source(bool);
+
+  void set_direct()
+  {
+    global_direct = source(true);
+  }
+
+  void read_direct() {
+    sink(global_direct); // $ ir MISSING: ast
+    indirect_sink(global_direct); // clean
+  }
 }

cpp/ql/test/library-tests/syntax-zoo/dataflow-ir-consistency.expected

@@ -4,7 +4,12 @@ uniqueType
 uniqueNodeLocation
 missingLocation
 uniqueNodeToString
-| cpp11.cpp:50:15:50:16 | (no string representation) | Node should have one toString but has 0. |
+| builtin.c:5:5:5:11 | (no string representation) | Node should have one toString but has 0. |
+| misc.c:227:7:227:28 | (no string representation) | Node should have one toString but has 0. |
+| static_init_templates.cpp:80:18:80:23 | (no string representation) | Node should have one toString but has 0. |
+| static_init_templates.cpp:80:18:80:23 | (no string representation) | Node should have one toString but has 0. |
+| static_init_templates.cpp:89:18:89:23 | (no string representation) | Node should have one toString but has 0. |
+| static_init_templates.cpp:89:18:89:23 | (no string representation) | Node should have one toString but has 0. |
 parameterCallable
 localFlowIsLocal
 readStepIsLocal