Merge pull request #7627 from geoffw0/nullterm5

C++: Fix branch related FPs in cpp/improper-null-termination.

Author: MathiasVP

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

@@ -23,6 +23,10 @@ DeclStmt declWithNoInit(LocalVariable v) {
   not exists(v.getInitializer())
 }
 
+/**
+ * Control flow reachability from a buffer that is not not null terminated to a
+ * sink that requires null termination.
+ */
 class ImproperNullTerminationReachability extends StackVariableReachabilityWithReassignment {
   ImproperNullTerminationReachability() { this = "ImproperNullTerminationReachability" }
 
@@ -52,12 +56,44 @@ class ImproperNullTerminationReachability extends StackVariableReachabilityWithR
 
   override predicate isBarrier(ControlFlowNode node, StackVariable v) {
     exprDefinition(v, node, _) or
-    mayAddNullTerminator(node, v.getAnAccess()) or
-    node.(AddressOfExpr).getOperand() = v.getAnAccess() or // address taken
     isSinkActual(node, v) // only report first use
   }
 }
 
-from ImproperNullTerminationReachability r, LocalVariable v, VariableAccess va
-where r.reaches(_, v, va)
-select va, "Variable $@ may not be null terminated.", v, v.getName()
+/**
+ * Flow from a place where null termination is added, to a sink of
+ * `ImproperNullTerminationReachability`. This was previously implemented as a
+ * simple barrier in `ImproperNullTerminationReachability`, but there were
+ * false positive results involving multiple paths from source to sink.  We'd
+ * prefer to report only the results we are sure of.
+ */
+class NullTerminationReachability extends StackVariableReachabilityWithReassignment {
+  NullTerminationReachability() { this = "NullTerminationReachability" }
+
+  override predicate isSourceActual(ControlFlowNode node, StackVariable v) {
+    mayAddNullTerminator(node, v.getAnAccess()) or // null termination
+    node.(AddressOfExpr).getOperand() = v.getAnAccess() // address taken (possible null termination)
+  }
+
+  override predicate isSinkActual(ControlFlowNode node, StackVariable v) {
+    // have the same sinks as `ImproperNullTerminationReachability`.
+    exists(ImproperNullTerminationReachability r | r.isSinkActual(node, v))
+  }
+
+  override predicate isBarrier(ControlFlowNode node, StackVariable v) {
+    // don't look further back than the source, or further forward than the sink
+    exists(ImproperNullTerminationReachability r | r.isSourceActual(node, v)) or
+    exists(ImproperNullTerminationReachability r | r.isSinkActual(node, v))
+  }
+}
+
+from
+  ImproperNullTerminationReachability reaches, NullTerminationReachability nullTermReaches,
+  ControlFlowNode source, LocalVariable v, VariableAccess sink
+where
+  reaches.reaches(source, v, sink) and
+  not exists(ControlFlowNode termination |
+    nullTermReaches.reaches(termination, _, sink) and
+    termination != source
+  )
+select sink, "Variable $@ may not be null terminated.", v, v.getName()

cpp/ql/src/change-notes/2022-01-18-improper-null-termination.md

@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* The "Potential improper null termination" (`cpp/improper-null-termination`) query now produces fewer false positive results around control flow branches and loops.

cpp/ql/test/query-tests/Likely Bugs/Memory Management/ImproperNullTermination/ImproperNullTermination.expected

@@ -18,10 +18,6 @@
 | test.cpp:302:10:302:16 | buffer2 | Variable $@ may not be null terminated. | test.cpp:297:8:297:14 | buffer2 | buffer2 |
 | test.cpp:314:10:314:15 | buffer | Variable $@ may not be null terminated. | test.cpp:310:8:310:13 | buffer | buffer |
 | test.cpp:336:18:336:23 | buffer | Variable $@ may not be null terminated. | test.cpp:335:8:335:13 | buffer | buffer |
-| test.cpp:355:11:355:16 | buffer | Variable $@ may not be null terminated. | test.cpp:350:8:350:13 | buffer | buffer |
-| test.cpp:364:11:364:16 | buffer | Variable $@ may not be null terminated. | test.cpp:359:8:359:13 | buffer | buffer |
-| test.cpp:392:11:392:16 | buffer | Variable $@ may not be null terminated. | test.cpp:381:8:381:13 | buffer | buffer |
-| test.cpp:410:11:410:16 | buffer | Variable $@ may not be null terminated. | test.cpp:397:8:397:13 | buffer | buffer |
 | test.cpp:421:19:421:25 | buffer2 | Variable $@ may not be null terminated. | test.cpp:419:8:419:14 | buffer2 | buffer2 |
 | test.cpp:448:17:448:22 | buffer | Variable $@ may not be null terminated. | test.cpp:446:8:446:13 | buffer | buffer |
 | test.cpp:454:18:454:23 | buffer | Variable $@ may not be null terminated. | test.cpp:452:8:452:13 | buffer | buffer |

cpp/ql/test/query-tests/Likely Bugs/Memory Management/ImproperNullTermination/test.cpp

@@ -352,7 +352,7 @@ void test_strlen(bool cond1, bool cond2)
 		if (cond1)
 			buffer[0] = 0;
 		if (cond1)
-			strlen(buffer); // GOOD [FALSE POSITIVE]
+			strlen(buffer); // GOOD
 	}
 
 	{
@@ -361,7 +361,7 @@ void test_strlen(bool cond1, bool cond2)
 		if (cond1)
 			buffer[0] = 0;
 		if (cond2)
-			strlen(buffer); // BAD
+			strlen(buffer); // BAD [NOT DETECTED]
 	}
 
 	{
@@ -389,7 +389,7 @@ void test_strlen(bool cond1, bool cond2)
 
 		if (init != 0)
 		{
-			strlen(buffer); // GOOD [FALSE POSITIVE]
+			strlen(buffer); // GOOD
 		}
 	}
 
@@ -407,7 +407,7 @@ void test_strlen(bool cond1, bool cond2)
 		{
 			// ...
 		} else {
-			strlen(buffer); // GOOD [FALSE POSITIVE]
+			strlen(buffer); // GOOD
 		}
 	}
 }