C++: Also fix 'OverrunWriteProductFlow.ql'

Author: MathiasVP

cpp/ql/src/experimental/Likely Bugs/ArrayAccessProductFlow.ql

@@ -74,5 +74,5 @@ from
   DataFlow::PathNode sink1, DataFlow2::PathNode sink2
 where conf.hasFlowPath(source1, source2, sink1, sink2)
 // TODO: pull delta out and display it
-select sink1, source1, sink1, "off-by one error allocated at $@ bounded by $@", source1, source1,
-  sink2, sink2
+select sink1.getNode(), source1, sink1, "off-by one error allocated at $@ bounded by $@", source1,
+  source1.toString(), sink2, sink2.toString()

cpp/ql/src/experimental/Likely Bugs/OverrunWriteProductFlow.ql

@@ -1,20 +1,21 @@
+/**
+ * @id cpp/overrun-write
+ * @kind path-problem
+ */
+
 import cpp
 import experimental.semmle.code.cpp.dataflow.ProductFlow
 import semmle.code.cpp.ir.IR
 import semmle.code.cpp.valuenumbering.GlobalValueNumbering
 import semmle.code.cpp.models.interfaces.Allocation
 import semmle.code.cpp.models.interfaces.ArrayFunction
+import DataFlow::PathGraph
 
 class StringSizeConfiguration extends ProductFlow::Configuration {
   StringSizeConfiguration() { this = "StringSizeConfiguration" }
 
   override predicate isSourcePair(DataFlow::Node bufSource, DataFlow::Node sizeSource) {
-    exists(
-      GVN sizeGvn // TODO: use-use flow instead of GVN
-    |
-      bufSource.asConvertedExpr().(AllocationExpr).getSizeExpr() = sizeGvn.getAnExpr() and
-      sizeSource.asConvertedExpr() = sizeGvn.getAnExpr()
-    )
+    bufSource.asConvertedExpr().(AllocationExpr).getSizeExpr() = sizeSource.asConvertedExpr()
   }
 
   override predicate isSinkPair(DataFlow::Node bufSink, DataFlow::Node sizeSink) {
@@ -31,4 +32,6 @@ from
   StringSizeConfiguration conf, DataFlow::PathNode source1, DataFlow2::PathNode source2,
   DataFlow::PathNode sink1, DataFlow2::PathNode sink2
 where conf.hasFlowPath(source1, source2, sink1, sink2)
-select source1, source2, sink1, sink2
+// TODO: pull delta out and display it
+select sink1.getNode(), source1, sink1, "overrunning write allocated at $@ bounded by $@", source1,
+  source1.toString(), sink2, sink2.toString()

cpp/ql/test/experimental/query-tests/Security/CWE/CWE-119/OverrunWriteProductFlow.expected

@@ -1,2 +1,33 @@
-| test.cpp:19:19:19:24 | call to malloc | test.cpp:18:17:18:20 | size | test.cpp:26:18:26:23 | Load | test.cpp:26:31:26:39 | Convert |
-| test.cpp:19:19:19:24 | call to malloc | test.cpp:18:17:18:20 | size | test.cpp:30:18:30:23 | Load | test.cpp:30:31:30:39 | Convert |
+edges
+| test.cpp:16:11:16:21 | VariableAddress indirection [string] | test.cpp:24:21:24:31 | Call indirection [string] |
+| test.cpp:16:11:16:21 | VariableAddress indirection [string] | test.cpp:34:21:34:31 | Call indirection [string] |
+| test.cpp:18:5:18:30 | Store | test.cpp:18:10:18:15 | Load indirection [post update] [string] |
+| test.cpp:18:10:18:15 | Load indirection [post update] [string] | test.cpp:16:11:16:21 | VariableAddress indirection [string] |
+| test.cpp:18:19:18:24 | call to malloc | test.cpp:18:5:18:30 | Store |
+| test.cpp:24:21:24:31 | Call indirection [string] | test.cpp:26:13:26:15 | Load indirection [string] |
+| test.cpp:26:13:26:15 | Load indirection [string] | test.cpp:26:18:26:23 | FieldAddress indirection |
+| test.cpp:26:18:26:23 | FieldAddress indirection | test.cpp:26:18:26:23 | Load |
+| test.cpp:29:32:29:34 | str indirection [string] | test.cpp:30:13:30:15 | Load indirection [string] |
+| test.cpp:30:13:30:15 | Load indirection [string] | test.cpp:30:18:30:23 | FieldAddress indirection |
+| test.cpp:30:18:30:23 | FieldAddress indirection | test.cpp:30:18:30:23 | Load |
+| test.cpp:34:21:34:31 | Call indirection [string] | test.cpp:35:21:35:23 | str indirection [string] |
+| test.cpp:35:21:35:23 | str indirection [string] | test.cpp:29:32:29:34 | str indirection [string] |
+nodes
+| test.cpp:16:11:16:21 | VariableAddress indirection [string] | semmle.label | VariableAddress indirection [string] |
+| test.cpp:18:5:18:30 | Store | semmle.label | Store |
+| test.cpp:18:10:18:15 | Load indirection [post update] [string] | semmle.label | Load indirection [post update] [string] |
+| test.cpp:18:19:18:24 | call to malloc | semmle.label | call to malloc |
+| test.cpp:24:21:24:31 | Call indirection [string] | semmle.label | Call indirection [string] |
+| test.cpp:26:13:26:15 | Load indirection [string] | semmle.label | Load indirection [string] |
+| test.cpp:26:18:26:23 | FieldAddress indirection | semmle.label | FieldAddress indirection |
+| test.cpp:26:18:26:23 | Load | semmle.label | Load |
+| test.cpp:29:32:29:34 | str indirection [string] | semmle.label | str indirection [string] |
+| test.cpp:30:13:30:15 | Load indirection [string] | semmle.label | Load indirection [string] |
+| test.cpp:30:18:30:23 | FieldAddress indirection | semmle.label | FieldAddress indirection |
+| test.cpp:30:18:30:23 | Load | semmle.label | Load |
+| test.cpp:34:21:34:31 | Call indirection [string] | semmle.label | Call indirection [string] |
+| test.cpp:35:21:35:23 | str indirection [string] | semmle.label | str indirection [string] |
+subpaths
+#select
+| test.cpp:26:18:26:23 | Load | test.cpp:18:19:18:24 | call to malloc | test.cpp:26:18:26:23 | Load | overrunning write allocated at $@ bounded by $@ | test.cpp:18:19:18:24 | call to malloc | call to malloc | test.cpp:26:31:26:39 | Convert | Convert |
+| test.cpp:30:18:30:23 | Load | test.cpp:18:19:18:24 | call to malloc | test.cpp:30:18:30:23 | Load | overrunning write allocated at $@ bounded by $@ | test.cpp:18:19:18:24 | call to malloc | call to malloc | test.cpp:30:31:30:39 | Convert | Convert |

cpp/ql/test/experimental/query-tests/Security/CWE/CWE-119/test.cpp

@@ -15,8 +15,8 @@ typedef struct
 
 string_t *mk_string_t(int size) {
     string_t *str = (string_t *) malloc(sizeof(string_t));
-    str->size = size;
     str->string = malloc(size);
+    str->size = size;
     return str;
 }