Skip to content

Commit 403a0eb

Browse files
committed
C++: Fix FPs in 'cpp/overflow-buffer' caused by unions of structs.
1 parent 941ad87 commit 403a0eb

File tree

1 file changed

+70
-28
lines changed

1 file changed

+70
-28
lines changed

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

Lines changed: 70 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,74 @@ predicate memberMayBeVarSize(Class c, MemberVariable v) {
2424
exists(ArrayType t | t = v.getUnspecifiedType() | not t.getArraySize() > 1)
2525
}
2626

27+
/**
28+
* Given a chain of accesses of the form `x.f1.f2...fn` this
29+
* predicate gives the type of `x`. Note that `x` may be an implicit
30+
* `this` expression.
31+
*/
32+
private Class getRootType(FieldAccess fa) {
33+
// If the object is accessed inside a ember function then the root will
34+
// be a(n implicit) `this`. And the root type will be the type of `this`.
35+
exists(VariableAccess root |
36+
root = fa.getQualifier*() and
37+
result =
38+
root.getQualifier()
39+
.(ThisExpr)
40+
.getUnspecifiedType()
41+
.(PointerType)
42+
.getBaseType()
43+
.getUnspecifiedType()
44+
)
45+
or
46+
// Otherwise, if this is not inside a member function there will not be
47+
// a(n implicit) `this`. And the root type is the type of the outermost
48+
// access.
49+
exists(VariableAccess root |
50+
root = fa.getQualifier+() and
51+
not exists(root.getQualifier()) and
52+
result = root.getUnspecifiedType()
53+
)
54+
}
55+
56+
/**
57+
* Gets the size of the buffer access at `va`.
58+
*/
59+
private int getSize(VariableAccess va) {
60+
exists(Variable v | va.getTarget() = v |
61+
// If `v` is not a field then the size of the buffer is just
62+
// the size of the type of `v`.
63+
exists(Type t |
64+
t = v.getUnspecifiedType() and
65+
not v instanceof Field and
66+
not t instanceof ReferenceType and
67+
result = t.getSize()
68+
)
69+
or
70+
exists(Class c |
71+
// Otherwise, we find the "outermost" object and compute the size
72+
// as the difference between the size of the type of the "outermost
73+
// object" and the offset of the field relative to that type.
74+
// For example, consider an access such as:
75+
// ```
76+
// struct S {
77+
// uint32_t x;
78+
// uint32_t y;
79+
// };
80+
// struct S2 {
81+
// S s;
82+
// uint32_t z;
83+
// };
84+
// ```
85+
// Given an object `S2 s2` the size of the buffer `&s2.s.y`
86+
// is the size of the base object type (i.e., `S2`) minutes the offset
87+
// of `y` relative to the type `S2` (i.e., `4`). So the size of the
88+
// buffer is `12 - 4 = 8`.
89+
c = getRootType(va) and
90+
result = c.getSize() - v.(Field).getOffsetInClass(c)
91+
)
92+
)
93+
}
94+
2795
/**
2896
* Holds if `bufferExpr` is an allocation-like expression.
2997
*
@@ -54,37 +122,11 @@ private int isSource(Expr bufferExpr, Element why) {
54122
result = bufferExpr.(AllocationExpr).getSizeBytes() and
55123
why = bufferExpr
56124
or
57-
exists(Type bufferType, Variable v |
125+
exists(Variable v |
58126
v = why and
59127
// buffer is the address of a variable
60128
why = bufferExpr.(AddressOfExpr).getAddressable() and
61-
bufferType = v.getUnspecifiedType() and
62-
not bufferType instanceof ReferenceType and
63-
not any(Union u).getAMemberVariable() = why
64-
|
65-
not v instanceof Field and
66-
result = bufferType.getSize()
67-
or
68-
// If it's an address of a field (i.e., a non-static member variable)
69-
// then it's okay to use that address to access the other member variables.
70-
// For example, this is okay:
71-
// ```
72-
// struct S { uint8_t a, b, c; };
73-
// S s;
74-
// memset(&s.a, 0, sizeof(S) - offsetof(S, a));
75-
exists(Field f |
76-
v = f and
77-
result = f.getDeclaringType().getSize() - f.getByteOffset()
78-
)
79-
)
80-
or
81-
exists(Union bufferType |
82-
// buffer is the address of a union member; in this case, we
83-
// take the size of the union itself rather the union member, since
84-
// it's usually OK to access that amount (e.g. clearing with memset).
85-
why = bufferExpr.(AddressOfExpr).getAddressable() and
86-
bufferType.getAMemberVariable() = why and
87-
result = bufferType.getSize()
129+
result = getSize(bufferExpr.(AddressOfExpr).getOperand())
88130
)
89131
}
90132

0 commit comments

Comments
 (0)