Merge pull request #20193 from MathiasVP/fix-fp-in-overflow-buffer

C++: Fix FP in `cpp/overflow-buffer`

Author: MathiasVP

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

@@ -57,19 +57,27 @@ private Class getRootType(FieldAccess fa) {
   )
 }
 
+/**
+ * Gets the size of `v`. This predicate does not have a result when the
+ * unspecified type of `v` is a `ReferenceType`.
+ */
+private int getVariableSize(Variable v) {
+  exists(Type t |
+    t = v.getUnspecifiedType() and
+    not t instanceof ReferenceType and
+    result = t.getSize()
+  )
+}
+
 /**
  * Gets the size of the buffer access at `va`.
  */
 private int getSize(VariableAccess va) {
   exists(Variable v | va.getTarget() = v |
     // If `v` is not a field then the size of the buffer is just
     // the size of the type of `v`.
-    exists(Type t |
-      t = v.getUnspecifiedType() and
-      not v instanceof Field and
-      not t instanceof ReferenceType and
-      result = t.getSize()
-    )
+    not v instanceof Field and
+    result = getVariableSize(v)
     or
     exists(Class c, int trueSize |
       // Otherwise, we find the "outermost" object and compute the size
@@ -92,7 +100,7 @@ private int getSize(VariableAccess va) {
       // buffer is `12 - 4 = 8`.
       c = getRootType(va) and
       // we calculate the size based on the last field, to avoid including any padding after it
-      trueSize = max(Field f | | f.getOffsetInClass(c) + f.getUnspecifiedType().getSize()) and
+      trueSize = max(Field f | | f.getOffsetInClass(c) + getVariableSize(f)) and
       result = trueSize - v.(Field).getOffsetInClass(c)
     )
   )

cpp/ql/src/change-notes/2025-08-08-overflow-buffer.md

@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* Fixed a false positive in `cpp/overflow-buffer` when the type of the destination buffer is a reference to a class/struct type.

cpp/ql/test/query-tests/Security/CWE/CWE-119/semmle/tests/UnboundedWrite.expected

@@ -27,8 +27,8 @@ edges
 | main.cpp:9:29:9:32 | *argv | tests_restrict.c:15:41:15:44 | *argv | provenance |  |
 | main.cpp:9:29:9:32 | tests_restrict_main output argument | main.cpp:10:20:10:23 | **argv | provenance |  |
 | main.cpp:9:29:9:32 | tests_restrict_main output argument | main.cpp:10:20:10:23 | *argv | provenance |  |
-| main.cpp:10:20:10:23 | **argv | tests.cpp:1060:32:1060:35 | **argv | provenance |  |
-| main.cpp:10:20:10:23 | *argv | tests.cpp:1060:32:1060:35 | *argv | provenance |  |
+| main.cpp:10:20:10:23 | **argv | tests.cpp:1074:32:1074:35 | **argv | provenance |  |
+| main.cpp:10:20:10:23 | *argv | tests.cpp:1074:32:1074:35 | *argv | provenance |  |
 | overflowdestination.cpp:23:45:23:48 | **argv | overflowdestination.cpp:23:45:23:48 | **argv | provenance |  |
 | overflowdestination.cpp:23:45:23:48 | **argv | overflowdestination.cpp:23:45:23:48 | *argv | provenance |  |
 | test_buffer_overrun.cpp:32:46:32:49 | **argv | test_buffer_overrun.cpp:32:46:32:49 | **argv | provenance |  |
@@ -41,12 +41,12 @@ edges
 | tests.cpp:649:14:649:14 | *s [*home] | tests.cpp:649:14:649:19 | *home | provenance |  |
 | tests.cpp:649:14:649:14 | *s [*home] | tests.cpp:649:16:649:19 | *home | provenance |  |
 | tests.cpp:649:16:649:19 | *home | tests.cpp:649:14:649:19 | *home | provenance |  |
-| tests.cpp:1060:32:1060:35 | **argv | tests.cpp:1085:9:1085:15 | *access to array | provenance |  |
-| tests.cpp:1060:32:1060:35 | **argv | tests.cpp:1086:9:1086:15 | *access to array | provenance |  |
-| tests.cpp:1060:32:1060:35 | *argv | tests.cpp:1085:9:1085:15 | *access to array | provenance |  |
-| tests.cpp:1060:32:1060:35 | *argv | tests.cpp:1086:9:1086:15 | *access to array | provenance |  |
-| tests.cpp:1085:9:1085:15 | *access to array | tests.cpp:634:19:634:24 | *source | provenance |  |
-| tests.cpp:1086:9:1086:15 | *access to array | tests.cpp:643:19:643:24 | *source | provenance |  |
+| tests.cpp:1074:32:1074:35 | **argv | tests.cpp:1099:9:1099:15 | *access to array | provenance |  |
+| tests.cpp:1074:32:1074:35 | **argv | tests.cpp:1100:9:1100:15 | *access to array | provenance |  |
+| tests.cpp:1074:32:1074:35 | *argv | tests.cpp:1099:9:1099:15 | *access to array | provenance |  |
+| tests.cpp:1074:32:1074:35 | *argv | tests.cpp:1100:9:1100:15 | *access to array | provenance |  |
+| tests.cpp:1099:9:1099:15 | *access to array | tests.cpp:634:19:634:24 | *source | provenance |  |
+| tests.cpp:1100:9:1100:15 | *access to array | tests.cpp:643:19:643:24 | *source | provenance |  |
 | tests_restrict.c:15:41:15:44 | **argv | tests_restrict.c:15:41:15:44 | **argv | provenance |  |
 | tests_restrict.c:15:41:15:44 | *argv | tests_restrict.c:15:41:15:44 | *argv | provenance |  |
 nodes
@@ -80,10 +80,10 @@ nodes
 | tests.cpp:649:14:649:14 | *s [*home] | semmle.label | *s [*home] |
 | tests.cpp:649:14:649:19 | *home | semmle.label | *home |
 | tests.cpp:649:16:649:19 | *home | semmle.label | *home |
-| tests.cpp:1060:32:1060:35 | **argv | semmle.label | **argv |
-| tests.cpp:1060:32:1060:35 | *argv | semmle.label | *argv |
-| tests.cpp:1085:9:1085:15 | *access to array | semmle.label | *access to array |
-| tests.cpp:1086:9:1086:15 | *access to array | semmle.label | *access to array |
+| tests.cpp:1074:32:1074:35 | **argv | semmle.label | **argv |
+| tests.cpp:1074:32:1074:35 | *argv | semmle.label | *argv |
+| tests.cpp:1099:9:1099:15 | *access to array | semmle.label | *access to array |
+| tests.cpp:1100:9:1100:15 | *access to array | semmle.label | *access to array |
 | tests_restrict.c:15:41:15:44 | **argv | semmle.label | **argv |
 | tests_restrict.c:15:41:15:44 | **argv | semmle.label | **argv |
 | tests_restrict.c:15:41:15:44 | *argv | semmle.label | *argv |

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

@@ -1057,6 +1057,20 @@ void test30() {
 	strncpy(us.buffer2, "", sizeof(us) - 1); // BAD
 }
 
+struct S_Size16 {
+	unsigned short uint16;
+	unsigned char uint8;
+	unsigned char raw[13];
+};
+
+void test31() {
+	S_Size16 e;
+
+	[&e](void* data){
+		memcpy(&e, data, sizeof(e)); // GOOD
+	};
+}
+
 int tests_main(int argc, char *argv[])
 {
 	long long arr17[19];