From 342b2df93fcbc86ad7fb374d85b8c40a389b0fa3 Mon Sep 17 00:00:00 2001 From: Anders Fugmann Date: Mon, 13 Sep 2021 11:25:04 +0200 Subject: [PATCH] C++: zero or one byte sized arrays in unions are considered as having the length of the union its a member of --- cpp/ql/lib/semmle/code/cpp/commons/Buffer.qll | 14 +++++++++++++- .../OverflowStatic/OverflowStatic.expected | 9 +++------ .../query-tests/Critical/OverflowStatic/test.c | 6 +++--- 3 files changed, 19 insertions(+), 10 deletions(-) diff --git a/cpp/ql/lib/semmle/code/cpp/commons/Buffer.qll b/cpp/ql/lib/semmle/code/cpp/commons/Buffer.qll index 16bcb6a1e97..76720cc6c76 100644 --- a/cpp/ql/lib/semmle/code/cpp/commons/Buffer.qll +++ b/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 diff --git a/cpp/ql/test/query-tests/Critical/OverflowStatic/OverflowStatic.expected b/cpp/ql/test/query-tests/Critical/OverflowStatic/OverflowStatic.expected index cc55beab6f1..2a95b0eba47 100644 --- a/cpp/ql/test/query-tests/Critical/OverflowStatic/OverflowStatic.expected +++ b/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. | diff --git a/cpp/ql/test/query-tests/Critical/OverflowStatic/test.c b/cpp/ql/test/query-tests/Critical/OverflowStatic/test.c index 7e80480c53c..a33ae82ee7d 100644 --- a/cpp/ql/test/query-tests/Critical/OverflowStatic/test.c +++ b/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 }