Merge pull request github#6915 from geoffw0/nullterm2

C++: Fix the two null termination queries and re-enable them.

Author: MathiasVP

cpp/ql/lib/semmle/code/cpp/commons/NullTermination.qll

@@ -11,6 +11,19 @@ private predicate mayAddNullTerminatorHelper(Expr e, VariableAccess va, Expr e0)
   )
 }
 
+bindingset[n1, n2]
+private predicate controlFlowNodeSuccessorTransitive(ControlFlowNode n1, ControlFlowNode n2) {
+  exists(BasicBlock bb1, int pos1, BasicBlock bb2, int pos2 |
+    pragma[only_bind_into](bb1).getNode(pos1) = n1 and
+    pragma[only_bind_into](bb2).getNode(pos2) = n2 and
+    (
+      bb1 = bb2 and pos1 < pos2
+      or
+      bb1.getASuccessor+() = bb2
+    )
+  )
+}
+
 /**
  * Holds if the expression `e` may add a null terminator to the string in
  * variable `v`.
@@ -30,14 +43,9 @@ predicate mayAddNullTerminator(Expr e, VariableAccess va) {
   )
   or
   // Assignment to another stack variable
-  exists(Expr e0, BasicBlock bb, int pos, BasicBlock bb0, int pos0 |
-    mayAddNullTerminatorHelper(e, va, e0) and
-    bb.getNode(pos) = e and
-    bb0.getNode(pos0) = e0
-  |
-    bb = bb0 and pos < pos0
-    or
-    bb.getASuccessor+() = bb0
+  exists(Expr e0 |
+    mayAddNullTerminatorHelper(pragma[only_bind_into](e), va, pragma[only_bind_into](e0)) and
+    controlFlowNodeSuccessorTransitive(e, e0)
   )
   or
   // Assignment to non-stack variable
@@ -119,14 +127,9 @@ predicate variableMustBeNullTerminated(VariableAccess va) {
       variableMustBeNullTerminated(use) and
       // Simplified: check that `p` may not be null terminated on *any*
       // path to `use` (including the one found via `parameterUsePair`)
-      not exists(Expr e, BasicBlock bb1, int pos1, BasicBlock bb2, int pos2 |
-        mayAddNullTerminator(e, p.getAnAccess()) and
-        bb1.getNode(pos1) = e and
-        bb2.getNode(pos2) = use
-      |
-        bb1 = bb2 and pos1 < pos2
-        or
-        bb1.getASuccessor+() = bb2
+      not exists(Expr e |
+        mayAddNullTerminator(pragma[only_bind_into](e), p.getAnAccess()) and
+        controlFlowNodeSuccessorTransitive(e, use)
       )
     )
   )

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

@@ -27,3 +27,4 @@
 | test.cpp:454:18:454:23 | buffer | Variable $@ may not be null terminated. | test.cpp:452:8:452:13 | buffer | buffer |
 | test.cpp:513:10:513:15 | buffer | Variable $@ may not be null terminated. | test.cpp:511:8:511:13 | buffer | buffer |
 | test.cpp:519:16:519:21 | buffer | Variable $@ may not be null terminated. | test.cpp:517:8:517:13 | buffer | buffer |
+| test.cpp:558:10:558:16 | buffer2 | Variable $@ may not be null terminated. | test.cpp:553:8:553:14 | buffer2 | buffer2 |

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

@@ -536,3 +536,51 @@ void test_printf(char *str)
 	}
 }
 
+void test_reassignment()
+{
+	{
+		char buffer1[1024];
+		char buffer2[1024];
+		char *buffer_ptr = buffer1;
+
+		buffer_ptr = buffer2;
+		strcpy(buffer_ptr, "content"); // null terminates buffer2
+		strdup(buffer2); // GOOD
+	}
+
+	{
+		char buffer1[1024];
+		char buffer2[1024];
+		char *buffer_ptr = buffer1;
+
+		strcpy(buffer_ptr, "content"); // null terminates buffer1
+		buffer_ptr = buffer2;
+		strdup(buffer2); // BAD
+	}
+
+	{
+		char buffer1[1024];
+		char buffer2[1024];
+		char *buffer_ptr = buffer1;
+
+		while (cond())
+		{
+			buffer_ptr = buffer2;
+			strcpy(buffer_ptr, "content"); // null terminates buffer2
+			strdup(buffer2); // GOOD
+		}
+	}
+
+	{
+		char buffer1[1024];
+		char buffer2[1024];
+		char *buffer_ptr = buffer1;
+
+		while (cond())
+		{
+			strcpy(buffer_ptr, "content"); // null terminates buffer1 or buffer2
+			buffer_ptr = buffer2;
+			strdup(buffer2); // BAD [NOT DETECTED]
+		}
+	}
+}