Merge pull request #12966 from MathiasVP/dataflow-for-static-vars

C++: Dataflow for static local variables

Author: MathiasVP

cpp/ql/lib/change-notes/2023-04-28-static-local-dataflow.md

@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* The new dataflow (`semmle.code.cpp.dataflow.new.DataFlow`) and taint-tracking libraries (`semmle.code.cpp.dataflow.new.TaintTracking`) now support tracking flow through static local variables.

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

@@ -607,13 +607,21 @@ OutNode getAnOutNode(DataFlowCall call, ReturnKind kind) {
   result.getReturnKind() = kind
 }
 
+/** A variable that behaves like a global variable. */
+class GlobalLikeVariable extends Variable {
+  GlobalLikeVariable() {
+    this instanceof Cpp::GlobalOrNamespaceVariable or
+    this instanceof Cpp::StaticLocalVariable
+  }
+}
+
 /**
  * Holds if data can flow from `node1` to `node2` in a way that loses the
  * calling context. For example, this would happen with flow through a
  * global or static variable.
  */
 predicate jumpStep(Node n1, Node n2) {
-  exists(Cpp::GlobalOrNamespaceVariable v |
+  exists(GlobalLikeVariable v |
     exists(Ssa::GlobalUse globalUse |
       v = globalUse.getVariable() and
       n1.(FinalGlobalValue).getGlobalUse() = globalUse

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

@@ -145,14 +145,14 @@ private newtype TDefOrUseImpl =
       or
       // Since the pruning stage doesn't know about global variables we can't use the above check to
       // rule out dead assignments to globals.
-      base.(VariableAddressInstruction).getAstVariable() instanceof Cpp::GlobalOrNamespaceVariable
+      base.(VariableAddressInstruction).getAstVariable() instanceof GlobalLikeVariable
     )
   } or
   TUseImpl(Operand operand, int indirectionIndex) {
     isUse(_, operand, _, _, indirectionIndex) and
     not isDef(_, _, operand, _, _, _)
   } or
-  TGlobalUse(Cpp::GlobalOrNamespaceVariable v, IRFunction f, int indirectionIndex) {
+  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 |
@@ -162,7 +162,7 @@ private newtype TDefOrUseImpl =
       indirectionIndex = [0 .. defIndex] + 1
     )
   } or
-  TGlobalDefImpl(Cpp::GlobalOrNamespaceVariable v, IRFunction f, int indirectionIndex) {
+  TGlobalDefImpl(GlobalLikeVariable v, IRFunction f, int indirectionIndex) {
     // Represents the initial "definition" of a global variable when entering
     // a function body.
     exists(VariableAddressInstruction vai |
@@ -458,7 +458,7 @@ class FinalParameterUse extends UseImpl, TFinalParameterUse {
 }
 
 class GlobalUse extends UseImpl, TGlobalUse {
-  Cpp::GlobalOrNamespaceVariable global;
+  GlobalLikeVariable global;
   IRFunction f;
 
   GlobalUse() { this = TGlobalUse(global, f, ind) }
@@ -468,7 +468,7 @@ class GlobalUse extends UseImpl, TGlobalUse {
   override int getIndirection() { result = ind + 1 }
 
   /** Gets the global variable associated with this use. */
-  Cpp::GlobalOrNamespaceVariable getVariable() { result = global }
+  GlobalLikeVariable getVariable() { result = global }
 
   /** Gets the `IRFunction` whose body is exited from after this use. */
   IRFunction getIRFunction() { result = f }
@@ -496,14 +496,14 @@ class GlobalUse extends UseImpl, TGlobalUse {
 }
 
 class GlobalDefImpl extends DefOrUseImpl, TGlobalDefImpl {
-  Cpp::GlobalOrNamespaceVariable global;
+  GlobalLikeVariable global;
   IRFunction f;
   int indirectionIndex;
 
   GlobalDefImpl() { this = TGlobalDefImpl(global, f, indirectionIndex) }
 
   /** Gets the global variable associated with this definition. */
-  Cpp::GlobalOrNamespaceVariable getVariable() { result = global }
+  GlobalLikeVariable getVariable() { result = global }
 
   /** Gets the `IRFunction` whose body is evaluated after this definition. */
   IRFunction getIRFunction() { result = f }
@@ -760,13 +760,14 @@ private predicate variableWriteCand(IRBlock bb, int i, SourceVariable v) {
 }
 
 private predicate sourceVariableIsGlobal(
-  SourceVariable sv, Cpp::GlobalOrNamespaceVariable global, IRFunction func, int indirectionIndex
+  SourceVariable sv, GlobalLikeVariable global, IRFunction func, int indirectionIndex
 ) {
   exists(IRVariable irVar, BaseIRVariable base |
     sourceVariableHasBaseAndIndex(sv, base, indirectionIndex) and
     irVar = base.getIRVariable() and
     irVar.getEnclosingIRFunction() = func and
-    global = irVar.getAst()
+    global = irVar.getAst() and
+    not irVar instanceof IRDynamicInitializationFlag
   )
 }
 
@@ -919,7 +920,7 @@ class GlobalDef extends TGlobalDef, SsaDefOrUse {
   IRFunction getIRFunction() { result = global.getIRFunction() }
 
   /** Gets the global variable associated with this definition. */
-  Cpp::GlobalOrNamespaceVariable getVariable() { result = global.getVariable() }
+  GlobalLikeVariable getVariable() { result = global.getVariable() }
 }
 
 class Phi extends TPhi, SsaDefOrUse {

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

@@ -115,6 +115,16 @@ postWithInFlow
 | test.cpp:602:3:602:7 | access to array [post update] | PostUpdateNode should not be the target of local flow. |
 | test.cpp:608:3:608:4 | * ... [post update] | PostUpdateNode should not be the target of local flow. |
 | test.cpp:608:4:608:4 | p [inner post update] | PostUpdateNode should not be the target of local flow. |
+| test.cpp:639:3:639:3 | x [post update] | PostUpdateNode should not be the target of local flow. |
+| test.cpp:646:3:646:3 | x [post update] | PostUpdateNode should not be the target of local flow. |
+| test.cpp:652:3:652:3 | x [post update] | PostUpdateNode should not be the target of local flow. |
+| test.cpp:653:3:653:3 | x [post update] | PostUpdateNode should not be the target of local flow. |
+| test.cpp:659:3:659:3 | x [post update] | PostUpdateNode should not be the target of local flow. |
+| test.cpp:660:3:660:3 | x [post update] | PostUpdateNode should not be the target of local flow. |
+| test.cpp:671:3:671:3 | s [post update] | PostUpdateNode should not be the target of local flow. |
+| test.cpp:681:3:681:3 | s [post update] | PostUpdateNode should not be the target of local flow. |
+| test.cpp:689:3:689:3 | s [post update] | PostUpdateNode should not be the target of local flow. |
+| test.cpp:690:3:690:3 | s [post update] | PostUpdateNode should not be the target of local flow. |
 viableImplInCallContextTooLarge
 uniqueParameterNodeAtPosition
 uniqueParameterNodePosition

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

@@ -627,4 +627,66 @@ void test_def_via_phi_read(bool b)
   }
   intPointerSource(buffer);
   sink(buffer); // $ ast,ir
-}
+}
+
+void test_static_local_1() {
+  static int x = source();
+  sink(x); // $ ast,ir
+}
+
+void test_static_local_2() {
+  static int x = source();
+  x = 0;
+  sink(x); // clean
+}
+
+void test_static_local_3() {
+  static int x = 0;
+  sink(x); // $ ir MISSING: ast
+  x = source();
+}
+
+void test_static_local_4() {
+  static int x = 0;
+  sink(x); // clean
+  x = source();
+  x = 0;
+}
+
+void test_static_local_5() {
+  static int x = 0;
+  sink(x); // $ ir MISSING: ast
+  x = 0;
+  x = source();
+}
+
+void test_static_local_6() {
+  static int s = source();
+  static int* ptr_to_s = &s;
+  sink(*ptr_to_s); // $ ir MISSING: ast
+}
+
+void test_static_local_7() {
+  static int s = source();
+  s = 0;
+  static int* ptr_to_s = &s;
+  sink(*ptr_to_s); // clean
+}
+
+void test_static_local_8() {
+  static int s;
+  static int* ptr_to_s = &s;
+  sink(*ptr_to_s); // $ ir MISSING: ast
+
+  s = source();
+}
+
+void test_static_local_9() {
+  static int s;
+  static int* ptr_to_s = &s;
+  sink(*ptr_to_s); // clean
+
+  s = source();
+  s = 0;
+}
+

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

@@ -4,7 +4,9 @@ uniqueType
 uniqueNodeLocation
 missingLocation
 uniqueNodeToString
+| cpp11.cpp:50:15:50:16 | (no string representation) | Node should have one toString but has 0. |
 missingToString
+| Nodes without toString: 1 |
 parameterCallable
 localFlowIsLocal
 readStepIsLocal

cpp/ql/test/query-tests/Likely Bugs/Format/NonConstantFormat/NonConstantFormat.expected

@@ -3,6 +3,7 @@
 | nested.cpp:21:23:21:26 | fmt0 | The format string argument to snprintf should be constant to prevent security issues and other potential errors. |
 | nested.cpp:79:32:79:38 | call to get_fmt | The format string argument to diagnostic should be constant to prevent security issues and other potential errors. |
 | nested.cpp:87:18:87:20 | fmt | The format string argument to diagnostic should be constant to prevent security issues and other potential errors. |
+| test.cpp:51:10:51:21 | call to make_message | The format string argument to printf should be constant to prevent security issues and other potential errors. |
 | test.cpp:57:12:57:16 | hello | The format string argument to printf should be constant to prevent security issues and other potential errors. |
 | test.cpp:60:12:60:21 | call to const_wash | The format string argument to printf should be constant to prevent security issues and other potential errors. |
 | test.cpp:61:12:61:26 | ... + ... | The format string argument to printf should be constant to prevent security issues and other potential errors. |

cpp/ql/test/query-tests/Likely Bugs/Format/NonConstantFormat/test.cpp

@@ -48,7 +48,7 @@ int main(int argc, char **argv) {
   printf(choose_message(argc - 1), argc - 1); // GOOD
   printf(messages[1]); // GOOD
   printf(message); // GOOD
-  printf(make_message(argc - 1)); // BAD  [NOT DETECTED]
+  printf(make_message(argc - 1)); // BAD
   printf("Hello, World\n"); // GOOD
   printf(_("Hello, World\n")); // GOOD
   {