Skip to content

Commit 4b2c7ef

Browse files
MathiasVPMathiasVP
authored
Merge pull request github#18615 from MathiasVP/fix-fp-buffer-overflow
C++: Fix FPs in `cpp/overflow-buffer`
2 parents 6e31214 + 48cae7e commit 4b2c7ef

File tree

6 files changed

+80
-16
lines changed

6 files changed

+80
-16
lines changed

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

Lines changed: 18 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -54,13 +54,28 @@ private int isSource(Expr bufferExpr, Element why) {
5454
result = bufferExpr.(AllocationExpr).getSizeBytes() and
5555
why = bufferExpr
5656
or
57-
exists(Type bufferType |
57+
exists(Type bufferType, Variable v |
58+
v = why and
5859
// buffer is the address of a variable
5960
why = bufferExpr.(AddressOfExpr).getAddressable() and
60-
bufferType = why.(Variable).getUnspecifiedType() and
61-
result = bufferType.getSize() and
61+
bufferType = v.getUnspecifiedType() and
6262
not bufferType instanceof ReferenceType and
6363
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+
)
6479
)
6580
or
6681
exists(Union bufferType |

‎cpp/ql/src/change-notes/2025-01-28-overflow-buffer.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
---
2+
category: minorAnalysis
3+
---
4+
* The "Call to memory access function may overflow buffer" query (`cpp/overflow-buffer`) now produces fewer FPs involving non-static member variables.

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
11
edges
2-
subpaths
32
nodes
3+
subpaths
44
#select

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

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,10 @@
4949
| tests.cpp:577:7:577:13 | access to array | This array indexing operation accesses a negative index -1 on the $@. | tests.cpp:565:7:565:12 | buffer | array |
5050
| tests.cpp:637:6:637:15 | access to array | This array indexing operation accesses a negative index -1 on the $@. | tests.cpp:633:7:633:12 | buffer | array |
5151
| tests.cpp:645:7:645:13 | access to array | This array indexing operation accesses a negative index -1 on the $@. | tests.cpp:633:7:633:12 | buffer | array |
52+
| tests.cpp:708:3:708:8 | call to memset | This 'memset' operation accesses 24 bytes but the $@ is only 8 bytes. | tests.cpp:693:16:693:16 | c | destination buffer |
53+
| tests.cpp:712:3:712:8 | call to memset | This 'memset' operation accesses 16 bytes but the $@ is only 8 bytes. | tests.cpp:693:16:693:16 | c | destination buffer |
54+
| tests.cpp:716:3:716:8 | call to memset | This 'memset' operation accesses 24 bytes but the $@ is only 16 bytes. | tests.cpp:692:16:692:16 | b | destination buffer |
55+
| tests.cpp:727:2:727:7 | call to memset | This 'memset' operation accesses 24 bytes but the $@ is only 8 bytes. | tests.cpp:693:16:693:16 | c | destination buffer |
5256
| tests_restrict.c:12:2:12:7 | call to memcpy | This 'memcpy' operation accesses 2 bytes but the $@ is only 1 byte. | tests_restrict.c:7:6:7:13 | smallbuf | source buffer |
5357
| unions.cpp:26:2:26:7 | call to memset | This 'memset' operation accesses 200 bytes but the $@ is only 100 bytes. | unions.cpp:21:10:21:11 | mu | destination buffer |
5458
| unions.cpp:30:2:30:7 | call to memset | This 'memset' operation accesses 200 bytes but the $@ is only 100 bytes. | unions.cpp:15:7:15:11 | small | destination buffer |

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

Lines changed: 12 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -27,8 +27,8 @@ edges
2727
| main.cpp:9:29:9:32 | *argv | tests_restrict.c:15:41:15:44 | *argv | provenance | |
2828
| main.cpp:9:29:9:32 | tests_restrict_main output argument | main.cpp:10:20:10:23 | **argv | provenance | |
2929
| main.cpp:9:29:9:32 | tests_restrict_main output argument | main.cpp:10:20:10:23 | *argv | provenance | |
30-
| main.cpp:10:20:10:23 | **argv | tests.cpp:689:32:689:35 | **argv | provenance | |
31-
| main.cpp:10:20:10:23 | *argv | tests.cpp:689:32:689:35 | *argv | provenance | |
30+
| main.cpp:10:20:10:23 | **argv | tests.cpp:730:32:730:35 | **argv | provenance | |
31+
| main.cpp:10:20:10:23 | *argv | tests.cpp:730:32:730:35 | *argv | provenance | |
3232
| overflowdestination.cpp:23:45:23:48 | **argv | overflowdestination.cpp:23:45:23:48 | **argv | provenance | |
3333
| overflowdestination.cpp:23:45:23:48 | **argv | overflowdestination.cpp:23:45:23:48 | *argv | provenance | |
3434
| test_buffer_overrun.cpp:32:46:32:49 | **argv | test_buffer_overrun.cpp:32:46:32:49 | **argv | provenance | |
@@ -41,12 +41,12 @@ edges
4141
| tests.cpp:628:14:628:14 | *s [*home] | tests.cpp:628:14:628:19 | *home | provenance | |
4242
| tests.cpp:628:14:628:14 | *s [*home] | tests.cpp:628:16:628:19 | *home | provenance | |
4343
| tests.cpp:628:16:628:19 | *home | tests.cpp:628:14:628:19 | *home | provenance | |
44-
| tests.cpp:689:32:689:35 | **argv | tests.cpp:714:9:714:15 | *access to array | provenance | |
45-
| tests.cpp:689:32:689:35 | **argv | tests.cpp:715:9:715:15 | *access to array | provenance | |
46-
| tests.cpp:689:32:689:35 | *argv | tests.cpp:714:9:714:15 | *access to array | provenance | |
47-
| tests.cpp:689:32:689:35 | *argv | tests.cpp:715:9:715:15 | *access to array | provenance | |
48-
| tests.cpp:714:9:714:15 | *access to array | tests.cpp:613:19:613:24 | *source | provenance | |
49-
| tests.cpp:715:9:715:15 | *access to array | tests.cpp:622:19:622:24 | *source | provenance | |
44+
| tests.cpp:730:32:730:35 | **argv | tests.cpp:755:9:755:15 | *access to array | provenance | |
45+
| tests.cpp:730:32:730:35 | **argv | tests.cpp:756:9:756:15 | *access to array | provenance | |
46+
| tests.cpp:730:32:730:35 | *argv | tests.cpp:755:9:755:15 | *access to array | provenance | |
47+
| tests.cpp:730:32:730:35 | *argv | tests.cpp:756:9:756:15 | *access to array | provenance | |
48+
| tests.cpp:755:9:755:15 | *access to array | tests.cpp:613:19:613:24 | *source | provenance | |
49+
| tests.cpp:756:9:756:15 | *access to array | tests.cpp:622:19:622:24 | *source | provenance | |
5050
| tests_restrict.c:15:41:15:44 | **argv | tests_restrict.c:15:41:15:44 | **argv | provenance | |
5151
| tests_restrict.c:15:41:15:44 | *argv | tests_restrict.c:15:41:15:44 | *argv | provenance | |
5252
nodes
@@ -80,10 +80,10 @@ nodes
8080
| tests.cpp:628:14:628:14 | *s [*home] | semmle.label | *s [*home] |
8181
| tests.cpp:628:14:628:19 | *home | semmle.label | *home |
8282
| tests.cpp:628:16:628:19 | *home | semmle.label | *home |
83-
| tests.cpp:689:32:689:35 | **argv | semmle.label | **argv |
84-
| tests.cpp:689:32:689:35 | *argv | semmle.label | *argv |
85-
| tests.cpp:714:9:714:15 | *access to array | semmle.label | *access to array |
86-
| tests.cpp:715:9:715:15 | *access to array | semmle.label | *access to array |
83+
| tests.cpp:730:32:730:35 | **argv | semmle.label | **argv |
84+
| tests.cpp:730:32:730:35 | *argv | semmle.label | *argv |
85+
| tests.cpp:755:9:755:15 | *access to array | semmle.label | *access to array |
86+
| tests.cpp:756:9:756:15 | *access to array | semmle.label | *access to array |
8787
| tests_restrict.c:15:41:15:44 | **argv | semmle.label | **argv |
8888
| tests_restrict.c:15:41:15:44 | **argv | semmle.label | **argv |
8989
| tests_restrict.c:15:41:15:44 | *argv | semmle.label | *argv |

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

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -685,6 +685,47 @@ int test28(MYSTRUCTREF g)
685685
return memcmp(&g, &_myStruct, sizeof(MYSTRUCT)); // GOOD
686686
}
687687

688+
#define offsetof(s, m) __builtin_offsetof(s, m)
689+
690+
struct HasSomeFields {
691+
unsigned long a;
692+
unsigned long b;
693+
unsigned long c;
694+
695+
void test29() {
696+
memset(&a, 0, sizeof(HasSomeFields) - offsetof(HasSomeFields, a)); // GOOD
697+
};
698+
699+
void test30() {
700+
memset(&b, 0, sizeof(HasSomeFields) - offsetof(HasSomeFields, b)); // GOOD
701+
};
702+
703+
void test31() {
704+
memset(&c, 0, sizeof(HasSomeFields) - offsetof(HasSomeFields, c)); // GOOD
705+
};
706+
707+
void test32() {
708+
memset(&c, 0, sizeof(HasSomeFields) - offsetof(HasSomeFields, a)); // BAD
709+
};
710+
711+
void test33() {
712+
memset(&c, 0, sizeof(HasSomeFields) - offsetof(HasSomeFields, b)); // BAD
713+
};
714+
715+
void test34() {
716+
memset(&b, 0, sizeof(HasSomeFields) - offsetof(HasSomeFields, a)); // BAD
717+
};
718+
719+
void test35() {
720+
memset(&b, 0, sizeof(HasSomeFields) - offsetof(HasSomeFields, b) - sizeof(unsigned long)); // GOOD
721+
};
722+
};
723+
724+
void test36() {
725+
HasSomeFields hsf;
726+
memset(&hsf.a, 0, sizeof(HasSomeFields) - offsetof(HasSomeFields, a)); // GOOD
727+
memset(&hsf.c, 0, sizeof(HasSomeFields) - offsetof(HasSomeFields, a)); // BAD
728+
}
688729

689730
int tests_main(int argc, char *argv[])
690731
{

0 commit comments

Comments
 (0)