C++: zero or one byte sized arrays in unions are considered as having the length of the union its a member of

Author: andersfugmann

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

@@ -28,7 +28,8 @@ predicate memberMayBeVarSize(Class c, MemberVariable v) {
     i = max(int j | c.getCanonicalMember(j) instanceof Field | j) and
     v = c.getCanonicalMember(i) and
     // v is an array of size at most 1
-    v.getUnspecifiedType().(ArrayType).getArraySize() <= 1
+    v.getUnspecifiedType().(ArrayType).getArraySize() <= 1 and
+    not c instanceof Union
   ) and
   // If the size is taken, then arithmetic is performed on the result at least once
   (
@@ -59,6 +60,10 @@ int getBufferSize(Expr bufferExpr, Element why) {
     result = bufferVar.getUnspecifiedType().(ArrayType).getSize() and
     why = bufferVar and
     not memberMayBeVarSize(_, bufferVar) and
+    not exists(Union bufferType |
+      bufferType.getAMemberVariable() = why and
+      bufferVar.getUnspecifiedType().(ArrayType).getSize() <= 1
+    ) and
     not result = 0 // zero sized arrays are likely to have special usage, for example
     or
     // behaving a bit like a 'union' overlapping other fields.
@@ -80,6 +85,13 @@ int getBufferSize(Expr bufferExpr, Element why) {
       parentPtr.getTarget().getUnspecifiedType().(PointerType).getBaseType() = parentClass and
       result = getBufferSize(parentPtr, _) + bufferVar.getType().getSize() - parentClass.getSize()
     )
+    or
+    exists(Union bufferType |
+      bufferType.getAMemberVariable() = why and
+      why = bufferVar and
+      bufferVar.getUnspecifiedType().(ArrayType).getSize() <= 1 and
+      result = bufferType.getSize()
+    )
   )
   or
   // buffer is a fixed size dynamic allocation

cpp/ql/test/query-tests/Critical/OverflowStatic/OverflowStatic.expected

@@ -11,12 +11,9 @@
 | test.c:21:9:21:18 | access to array | Potential buffer-overflow: 'ys' has size 5 but 'ys[6]' is accessed here. |
 | test.c:39:3:39:11 | access to array | Potential buffer-overflow: 'buf' has size 1 but 'buf[7]' is accessed here. |
 | test.c:40:3:40:11 | access to array | Potential buffer-overflow: 'buf' has size 1 but 'buf[8]' is accessed here. |
-| test.c:51:3:51:20 | access to array | Potential buffer-overflow: 'ptr' has size 1 but 'ptr[7]' is accessed here. |
-| test.c:52:3:52:18 | access to array | Potential buffer-overflow: 'ptr' has size 1 but 'ptr[8]' is accessed here. |
-| test.c:58:3:58:28 | access to array | Potential buffer-overflow: 'ptr' has size 1 but 'ptr[7]' is accessed here. |
-| test.c:59:3:59:26 | access to array | Potential buffer-overflow: 'ptr' has size 1 but 'ptr[8]' is accessed here. |
-| test.c:65:3:65:20 | access to array | Potential buffer-overflow: 'ptr' has size 1 but 'ptr[7]' is accessed here. |
-| test.c:66:3:66:18 | access to array | Potential buffer-overflow: 'ptr' has size 1 but 'ptr[8]' is accessed here. |
+| test.c:52:3:52:18 | access to array | Potential buffer-overflow: 'ptr' has size 8 but 'ptr[8]' is accessed here. |
+| test.c:59:3:59:26 | access to array | Potential buffer-overflow: 'ptr' has size 8 but 'ptr[8]' is accessed here. |
+| test.c:66:3:66:18 | access to array | Potential buffer-overflow: 'ptr' has size 8 but 'ptr[8]' is accessed here. |
 | test.c:72:3:72:11 | access to array | Potential buffer-overflow: 'buf' has size 1 but 'buf[1]' is accessed here. |
 | test.cpp:19:3:19:12 | access to array | Potential buffer-overflow: counter 'i' <= 3 but 'buffer1' has 3 elements. |
 | test.cpp:20:3:20:12 | access to array | Potential buffer-overflow: counter 'i' <= 3 but 'buffer2' has 3 elements. |

cpp/ql/test/query-tests/Critical/OverflowStatic/test.c

@@ -48,21 +48,21 @@ union u {
 void union_test() {
   union u u;
   u.ptr[0] = 0; // GOOD
-  u.ptr[sizeof(u)-1] = 0; // GOOD [FALSE POSITIVE]
+  u.ptr[sizeof(u)-1] = 0; // GOOD
   u.ptr[sizeof(u)] = 0; // BAD
 }
 
 void test_struct_union() {
   struct { union u u; } v;
   v.u.ptr[0] = 0; // GOOD
-  v.u.ptr[sizeof(union u)-1] = 0; // GOOD [FALSE POSITIVE]
+  v.u.ptr[sizeof(union u)-1] = 0; // GOOD
   v.u.ptr[sizeof(union u)] = 0; // BAD
 }
 
 void union_test2() {
   union { char ptr[1]; unsigned long value; } u;
   u.ptr[0] = 0; // GOOD
-  u.ptr[sizeof(u)-1] = 0; // GOOD [FALSE POSITIVE]
+  u.ptr[sizeof(u)-1] = 0; // GOOD
   u.ptr[sizeof(u)] = 0; // BAD
 }