Merge pull request #14704 from MathiasVP/fix-uninitialized-local

C++: IR'ify `cpp/uninitialized-local` and fix FPs

cpp/ql/lib/semmle/code/cpp/ir/dataflow/MustFlow.qll

@@ -31,6 +31,11 @@ abstract class MustFlowConfiguration extends string {
    */
   abstract predicate isSink(Operand sink);
 
+  /**
+   * Holds if data flow through `instr` is prohibited.
+   */
+  predicate isBarrier(Instruction instr) { none() }
+
   /**
    * Holds if the additional flow step from `node1` to `node2` must be taken
    * into account in the analysis.
@@ -48,18 +53,21 @@ abstract class MustFlowConfiguration extends string {
    */
   final predicate hasFlowPath(MustFlowPathNode source, MustFlowPathSink sink) {
     this.isSource(source.getInstruction()) and
-    source.getASuccessor+() = sink
+    source.getASuccessor*() = sink
   }
 }
 
 /** Holds if `node` flows from a source. */
 pragma[nomagic]
 private predicate flowsFromSource(Instruction node, MustFlowConfiguration config) {
-  config.isSource(node)
-  or
-  exists(Instruction mid |
-    step(mid, node, config) and
-    flowsFromSource(mid, pragma[only_bind_into](config))
+  not config.isBarrier(node) and
+  (
+    config.isSource(node)
+    or
+    exists(Instruction mid |
+      step(mid, node, config) and
+      flowsFromSource(mid, pragma[only_bind_into](config))
+    )
   )
 }
 

cpp/ql/src/Likely Bugs/Memory Management/UninitializedLocal.ql

@@ -13,7 +13,8 @@
  */
 
 import cpp
-import semmle.code.cpp.controlflow.StackVariableReachability
+import semmle.code.cpp.ir.IR
+import semmle.code.cpp.ir.dataflow.MustFlow
 
 /**
  * Auxiliary predicate: Types that don't require initialization
@@ -33,31 +34,6 @@ predicate allocatedType(Type t) {
   allocatedType(t.getUnspecifiedType())
 }
 
-/**
- * A declaration of a local variable that leaves the
- * variable uninitialized.
- */
-DeclStmt declWithNoInit(LocalVariable v) {
-  result.getADeclaration() = v and
-  not exists(v.getInitializer()) and
-  /* The type of the variable is not stack-allocated. */
-  exists(Type t | t = v.getType() | not allocatedType(t))
-}
-
-class UninitialisedLocalReachability extends StackVariableReachability {
-  UninitialisedLocalReachability() { this = "UninitialisedLocal" }
-
-  override predicate isSource(ControlFlowNode node, StackVariable v) { node = declWithNoInit(v) }
-
-  override predicate isSink(ControlFlowNode node, StackVariable v) { useOfVarActual(v, node) }
-
-  override predicate isBarrier(ControlFlowNode node, StackVariable v) {
-    // only report the _first_ possibly uninitialized use
-    useOfVarActual(v, node) or
-    definitionBarrier(v, node)
-  }
-}
-
 pragma[noinline]
 predicate containsInlineAssembly(Function f) { exists(AsmStmt s | s.getEnclosingFunction() = f) }
 
@@ -82,8 +58,33 @@ VariableAccess commonException() {
   containsInlineAssembly(result.getEnclosingFunction())
 }
 
-from UninitialisedLocalReachability r, LocalVariable v, VariableAccess va
+predicate isSinkImpl(Instruction sink, VariableAccess va) {
+  exists(LoadInstruction load |
+    va = load.getUnconvertedResultExpression() and
+    not va = commonException() and
+    sink = load.getSourceValue()
+  )
+}
+
+class MustFlow extends MustFlowConfiguration {
+  MustFlow() { this = "MustFlow" }
+
+  override predicate isSource(Instruction source) {
+    source instanceof UninitializedInstruction and
+    exists(Type t | t = source.getResultType() | not allocatedType(t))
+  }
+
+  override predicate isSink(Operand sink) { isSinkImpl(sink.getDef(), _) }
+
+  override predicate allowInterproceduralFlow() { none() }
+
+  override predicate isBarrier(Instruction instr) { instr instanceof ChiInstruction }
+}
+
+from
+  VariableAccess va, LocalVariable v, MustFlow conf, MustFlowPathNode source, MustFlowPathNode sink
 where
-  r.reaches(_, v, va) and
-  not va = commonException()
+  conf.hasFlowPath(source, sink) and
+  isSinkImpl(sink.getInstruction(), va) and
+  v = va.getTarget()
 select va, "The variable $@ may not be initialized at this access.", v, v.getName()

cpp/ql/src/change-notes/2023-11-07-uninitialized-local.md

@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* The `cpp/uninitialized-local` query has been improved to produce fewer false positives.

cpp/ql/test/query-tests/Security/CWE/CWE-457/semmle/tests/UninitializedLocal.expected

@@ -1,14 +1,7 @@
 | test.cpp:12:6:12:8 | foo | The variable $@ may not be initialized at this access. | test.cpp:11:6:11:8 | foo | foo |
-| test.cpp:30:6:30:8 | foo | The variable $@ may not be initialized at this access. | test.cpp:26:6:26:8 | foo | foo |
-| test.cpp:46:6:46:8 | foo | The variable $@ may not be initialized at this access. | test.cpp:42:6:42:8 | foo | foo |
-| test.cpp:55:7:55:9 | foo | The variable $@ may not be initialized at this access. | test.cpp:50:6:50:8 | foo | foo |
-| test.cpp:67:7:67:9 | foo | The variable $@ may not be initialized at this access. | test.cpp:61:6:61:8 | foo | foo |
-| test.cpp:92:6:92:8 | foo | The variable $@ may not be initialized at this access. | test.cpp:82:6:82:8 | foo | foo |
 | test.cpp:113:6:113:8 | foo | The variable $@ may not be initialized at this access. | test.cpp:111:6:111:8 | foo | foo |
-| test.cpp:132:9:132:9 | j | The variable $@ may not be initialized at this access. | test.cpp:126:6:126:6 | j | j |
 | test.cpp:219:3:219:3 | x | The variable $@ may not be initialized at this access. | test.cpp:218:7:218:7 | x | x |
 | test.cpp:243:13:243:13 | i | The variable $@ may not be initialized at this access. | test.cpp:241:6:241:6 | i | i |
-| test.cpp:329:9:329:11 | val | The variable $@ may not be initialized at this access. | test.cpp:321:6:321:8 | val | val |
 | test.cpp:336:10:336:10 | a | The variable $@ may not be initialized at this access. | test.cpp:333:7:333:7 | a | a |
 | test.cpp:369:10:369:10 | a | The variable $@ may not be initialized at this access. | test.cpp:358:7:358:7 | a | a |
 | test.cpp:378:9:378:11 | val | The variable $@ may not be initialized at this access. | test.cpp:359:6:359:8 | val | val |

cpp/ql/test/query-tests/Security/CWE/CWE-457/semmle/tests/test.cpp

@@ -27,7 +27,7 @@ void test4(bool b) {
 	if (b) {
 		foo = 1;
 	}
-	use(foo); // BAD
+	use(foo); // BAD [NOT DETECTED]
 }
 
 void test5() {
@@ -43,7 +43,7 @@ void test5(int count) {
 	for (int i = 0; i < count; i++) {
 		foo = i;
 	}
-	use(foo); // BAD
+	use(foo); // BAD [NOT DETECTED]
 }
 
 void test6(bool b) {
@@ -52,7 +52,7 @@ void test6(bool b) {
 		foo = 42;
 	}
 	if (b) {
-		use(foo); // GOOD (REPORTED, FP)
+		use(foo); // GOOD
 	}
 }
 
@@ -64,7 +64,7 @@ void test7(bool b) {
 		set = true;
 	}
 	if (set) {
-		use(foo); // GOOD (REPORTED, FP)
+		use(foo); // GOOD
 	}
 }
 
@@ -89,7 +89,7 @@ void test9(int count) {
 	if (!set) {
 		foo = 42;
 	}
-	use(foo); // GOOD (REPORTED, FP)
+	use(foo); // GOOD
 }
 
 void test10() {
@@ -129,7 +129,7 @@ int absWrong(int i) {
 	} else if (i < 0) {
 		j = -i;
 	}
-	return j; // wrong: j may not be initialized before use
+	return j; // wrong: j may not be initialized before use [NOT DETECTED]
 }
 
 // Example from qhelp
@@ -326,7 +326,7 @@ int test28() {
 		a = false;
 		c = false;
 	}
-	return val; // GOOD [FALSE POSITIVE]
+	return val; // GOOD
 }
 
 int test29() {
@@ -472,4 +472,64 @@ void test44() {
 	int y = 1;
 
 	void(x + y); // BAD
+}
+
+enum class State { StateA, StateB, StateC };
+
+int exhaustive_switch(State s) {
+	int y;
+	switch(s) {
+		case State::StateA:
+			y = 1;
+			break;
+		case State::StateB:
+			y = 2;
+			break;
+		case State::StateC:
+			y = 3;
+			break;
+	}
+	return y; // GOOD (y is always initialized)
+}
+
+int exhaustive_switch_2(State s) {
+	int y;
+	switch(s) {
+		case State::StateA:
+			y = 1;
+			break;
+		default:
+			y = 2;
+			break;
+	}
+	return y; // GOOD (y is always initialized)
+}
+
+int non_exhaustive_switch(State s) {
+	int y;
+	switch(s) {
+		case State::StateA:
+			y = 1;
+			break;
+		case State::StateB:
+			y = 2;
+			break;
+	}
+	return y; // BAD [NOT DETECTED] (y is not initialized when s = StateC)
+}
+
+int non_exhaustive_switch_2(State s) {
+	int y;
+	switch(s) {
+		case State::StateA:
+			y = 1;
+			break;
+		case State::StateB:
+			y = 2;
+			break;
+	}
+	if(s != State::StateC) {
+		return y; // GOOD (y is not initialized when s = StateC, but if s = StateC we won't reach this point)
+	}
+	return 0;
 }