[NFC][analyzer] OOB test consolidation III: 'outofbound' tests #128508
Conversation
Before commit 6e17ed9 the test files `outofbound.c` and `outofbound-notwork.c` tested the behavior of the old alpha checker `alpha.security.ArrayBound` (V1); then that commit converted them into tests for the checker `security.ArrayBound` which was previously called `alpha.security.ArrayBoundV2`. This commit removes these test files and migrates their useful content to `out-of-bounds.c`. The file `outofbound.c` contained lots of testcases that covered features which are also covered in `out-of-bounds.c` or `out-of-bounds-diagnostics.c`; those redundant cases are discarded during this migration process. This is part of a commit series that reorganizes the tests of `security.ArrayBound` to system that's easier to understand and maintain.
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-static-analyzer-1 @llvm/pr-subscribers-clang Author: Donát Nagy (NagyDonat) ChangesBefore commit 6e17ed9 the test files This commit removes these test files and migrates their useful content to This is part of a commit series that reorganizes the tests of Full diff: https://github.com/llvm/llvm-project/pull/128508.diff 3 Files Affected:
diff --git a/clang/test/Analysis/out-of-bounds.c b/clang/test/Analysis/out-of-bounds.c
index 7d6cb4ecf1b24..2174dafc0021b 100644
--- a/clang/test/Analysis/out-of-bounds.c
+++ b/clang/test/Analysis/out-of-bounds.c
@@ -217,3 +217,53 @@ int test_negative_offset_with_unsigned_idx(void) {
unsigned idx = 2u;
return p[idx]; // expected-warning {{Out of bound access to memory preceding}}
}
+
+struct three_words { int c[3]; };
+struct seven_words { int c[7]; };
+void partially_in_bounds(void) {
+ struct seven_words c;
+ struct three_words a, *p = (struct three_words *)&c;
+ p[0] = a; // no-warning
+ p[1] = a; // no-warning
+ p[2] = a; // should warn
+ // FIXME: This is an overflow, but currently security.ArrayBound only checks
+ // that the _beginning_ of the accessed element is within bounds.
+}
+
+void vla(int a) {
+ if (a == 5) {
+ int x[a];
+ x[4] = 4; // no-warning
+ x[5] = 5; // expected-warning{{Out of bound access}}
+ }
+}
+
+void sizeof_vla(int a) {
+ // FIXME: VLA modeling is not good enough to cover this case.
+ if (a == 5) {
+ char x[a];
+ int y[sizeof(x)];
+ y[4] = 4; // no-warning
+ y[5] = 5; // should be {{Out of bounds access}}
+ }
+}
+
+void sizeof_vla_2(int a) {
+ // FIXME: VLA modeling is not good enough to cover this case.
+ if (a == 5) {
+ char x[a];
+ int y[sizeof(x) / sizeof(char)];
+ y[4] = 4; // no-warning
+ y[5] = 5; // should be {{Out of bounds access}}
+ }
+}
+
+void sizeof_vla_3(int a) {
+ // FIXME: VLA modeling is not good enough to cover this case.
+ if (a == 5) {
+ char x[a];
+ int y[sizeof(*&*&*&x)];
+ y[4] = 4; // no-warning
+ y[5] = 5; // should be {{Out of bounds access}}
+ }
+}
diff --git a/clang/test/Analysis/outofbound-notwork.c b/clang/test/Analysis/outofbound-notwork.c
deleted file mode 100644
index 1318c07bbf2a8..0000000000000
--- a/clang/test/Analysis/outofbound-notwork.c
+++ /dev/null
@@ -1,32 +0,0 @@
-// RUN: %clang_analyze_cc1 -Wno-array-bounds -analyzer-checker=core,security.ArrayBound -verify %s
-// XFAIL: *
-
-// Once we better handle modeling of sizes of VLAs, we can pull this back
-// into outofbound.c.
-
-void sizeof_vla(int a) {
- if (a == 5) {
- char x[a];
- int y[sizeof(x)];
- y[4] = 4; // no-warning
- y[5] = 5; // expected-warning{{Out of bounds access}}
- }
-}
-
-void sizeof_vla_2(int a) {
- if (a == 5) {
- char x[a];
- int y[sizeof(x) / sizeof(char)];
- y[4] = 4; // no-warning
- y[5] = 5; // expected-warning{{Out of bounds access}}
- }
-}
-
-void sizeof_vla_3(int a) {
- if (a == 5) {
- char x[a];
- int y[sizeof(*&*&*&x)];
- y[4] = 4; // no-warning
- y[5] = 5; // expected-warning{{Out of bounds access}}
- }
-}
diff --git a/clang/test/Analysis/outofbound.c b/clang/test/Analysis/outofbound.c
deleted file mode 100644
index d3d8ff2b2f0ed..0000000000000
--- a/clang/test/Analysis/outofbound.c
+++ /dev/null
@@ -1,130 +0,0 @@
-// RUN: %clang_analyze_cc1 -Wno-array-bounds -verify %s \
-// RUN: -analyzer-checker=core \
-// RUN: -analyzer-checker=unix \
-// RUN: -analyzer-checker=security.ArrayBound \
-// RUN: -analyzer-config unix.DynamicMemoryModeling:Optimistic=true
-
-typedef __typeof(sizeof(int)) size_t;
-void *malloc(size_t);
-void *calloc(size_t, size_t);
-
-char f1(void) {
- char* s = "abcd";
- char c = s[4]; // no-warning
- return s[5] + c; // expected-warning{{Out of bound access to memory after}}
-}
-
-void f2(void) {
- int *p = malloc(12);
- p[3] = 4; // expected-warning{{Out of bound access to memory after}}
-}
-
-struct three_words {
- int c[3];
-};
-
-struct seven_words {
- int c[7];
-};
-
-void f3(void) {
- struct three_words a, *p;
- p = &a;
- p[0] = a; // no-warning
- p[1] = a; // expected-warning{{Out of bound access to memory after}}
-}
-
-void f4(void) {
- struct seven_words c;
- struct three_words a, *p = (struct three_words *)&c;
- p[0] = a; // no-warning
- p[1] = a; // no-warning
- p[2] = a; // should warn
- // FIXME: This is an overflow, but currently security.ArrayBound only checks
- // that the _beginning_ of the accessed element is within bounds.
-}
-
-void f5(void) {
- char *p = calloc(2,2);
- p[3] = '.'; // no-warning
- p[4] = '!'; // expected-warning{{Out of bound access}}
-}
-
-void f6(void) {
- char a[2];
- int *b = (int*)a;
- b[1] = 3; // expected-warning{{Out of bound access}}
-}
-
-void f7(void) {
- struct three_words a;
- a.c[3] = 1; // expected-warning{{Out of bound access}}
-}
-
-void vla(int a) {
- if (a == 5) {
- int x[a];
- x[4] = 4; // no-warning
- x[5] = 5; // expected-warning{{Out of bound access}}
- }
-}
-
-void alloca_region(int a) {
- if (a == 5) {
- char *x = __builtin_alloca(a);
- x[4] = 4; // no-warning
- x[5] = 5; // expected-warning{{Out of bound access}}
- }
-}
-
-int symbolic_index(int a) {
- int x[2] = {1, 2};
- if (a == 2) {
- return x[a]; // expected-warning{{Out of bound access}}
- }
- return 0;
-}
-
-int symbolic_index2(int a) {
- int x[2] = {1, 2};
- if (a < 0) {
- return x[a]; // expected-warning{{Out of bound access}}
- }
- return 0;
-}
-
-int overflow_binary_search(double in) {
- int eee = 16;
- if (in < 1e-8 || in > 1e23) {
- return 0;
- } else {
- static const double ins[] = {1e-8, 1e-7, 1e-6, 1e-5, 1e-4, 1e-3, 1e-2, 1e-1,
- 1e0, 1e1, 1e2, 1e3, 1e4, 1e5, 1e6, 1e7,
- 1e8, 1e9, 1e10, 1e11, 1e12, 1e13, 1e14, 1e15,
- 1e16, 1e17, 1e18, 1e19, 1e20, 1e21, 1e22};
- if (in < ins[eee]) {
- eee -= 8;
- } else {
- eee += 8;
- }
- if (in < ins[eee]) {
- eee -= 4;
- } else {
- eee += 4;
- }
- if (in < ins[eee]) {
- eee -= 2;
- } else {
- eee += 2;
- }
- if (in < ins[eee]) {
- eee -= 1;
- } else {
- eee += 1;
- }
- if (in < ins[eee]) { // expected-warning {{Out of bound access}}
- eee -= 1;
- }
- }
- return eee;
-}
|
| char f1(void) { | ||
| char* s = "abcd"; | ||
| char c = s[4]; // no-warning | ||
| return s[5] + c; // expected-warning{{Out of bound access to memory after}} | ||
| } |
There was a problem hiding this comment.
AFAIK this test won't have a direct equivalent after this commit; but they are tested incidentally by a few (intFromString and intFromStringDivisible) in out-of-bounds-diagnostics.c. String literals are simple and straightforward regions, so I think this coverage is sufficient for them; but I could migrate this test case if you wish.
| void f2(void) { | ||
| int *p = malloc(12); | ||
| p[3] = 4; // expected-warning{{Out of bound access to memory after}} | ||
| } |
There was a problem hiding this comment.
malloc is tested at mallocRegion in out-of-bounds-diagnostics.c (without unwarranted assumptions about sizeof(int)).
| void f3(void) { | ||
| struct three_words a, *p; | ||
| p = &a; | ||
| p[0] = a; // no-warning | ||
| p[1] = a; // expected-warning{{Out of bound access to memory after}} | ||
| } |
There was a problem hiding this comment.
This is very close to scalarOverflow in out-of-bounds-diagnostics.c. The only difference is that this uses a struct instead of an int, which is not relevant for the logic of the checker (neither is special cased, and there are lots of cases verifying that structs work properly), so I'm discarding this.
| void f4(void) { | ||
| struct seven_words c; | ||
| struct three_words a, *p = (struct three_words *)&c; | ||
| p[0] = a; // no-warning | ||
| p[1] = a; // no-warning | ||
| p[2] = a; // should warn | ||
| // FIXME: This is an overflow, but currently security.ArrayBound only checks | ||
| // that the _beginning_ of the accessed element is within bounds. | ||
| } |
There was a problem hiding this comment.
I'm migrating this testcase to out-of-bounds.c under the more descriptive name partially_in_bounds.
| void f5(void) { | ||
| char *p = calloc(2,2); | ||
| p[3] = '.'; // no-warning | ||
| p[4] = '!'; // expected-warning{{Out of bound access}} | ||
| } |
There was a problem hiding this comment.
From the POV of the ArrayBound checker calloc is identical to malloc. (Any differences between them should be tested in tests for MallocChecker.)
| void vla(int a) { | ||
| if (a == 5) { | ||
| int x[a]; | ||
| x[4] = 4; // no-warning | ||
| x[5] = 5; // expected-warning{{Out of bound access}} | ||
| } | ||
| } |
There was a problem hiding this comment.
Migrating this to out-of-bounds.c.
|
|
||
| void alloca_region(int a) { | ||
| if (a == 5) { | ||
| char *x = __builtin_alloca(a); | ||
| x[4] = 4; // no-warning | ||
| x[5] = 5; // expected-warning{{Out of bound access}} | ||
| } | ||
| } |
There was a problem hiding this comment.
Alloca is tested by allocaRegion in out-of-bounds-diagnostics.c; symbolic extent is tested by other cases.
| int symbolic_index(int a) { | ||
| int x[2] = {1, 2}; | ||
| if (a == 2) { | ||
| return x[a]; // expected-warning{{Out of bound access}} | ||
| } | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
This "weak" symbolic index (which is assumed to be exactly equal to a concrete value) is AFAIK not explicitly tested; but it is not a relevant case because the analyzer engine represents it with the same nonloc::ConcreteInt which would represent an integer literal 2.
| int symbolic_index2(int a) { | ||
| int x[2] = {1, 2}; | ||
| if (a < 0) { | ||
| return x[a]; // expected-warning{{Out of bound access}} | ||
| } | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
Symbolic indices are tested in very many testcases, including e.g. symbolicIndex in out-of-bounds-diagnostics.c.
| int overflow_binary_search(double in) { | ||
| int eee = 16; | ||
| if (in < 1e-8 || in > 1e23) { | ||
| return 0; | ||
| } else { | ||
| static const double ins[] = {1e-8, 1e-7, 1e-6, 1e-5, 1e-4, 1e-3, 1e-2, 1e-1, | ||
| 1e0, 1e1, 1e2, 1e3, 1e4, 1e5, 1e6, 1e7, | ||
| 1e8, 1e9, 1e10, 1e11, 1e12, 1e13, 1e14, 1e15, | ||
| 1e16, 1e17, 1e18, 1e19, 1e20, 1e21, 1e22}; | ||
| if (in < ins[eee]) { | ||
| eee -= 8; | ||
| } else { | ||
| eee += 8; | ||
| } | ||
| if (in < ins[eee]) { | ||
| eee -= 4; | ||
| } else { | ||
| eee += 4; | ||
| } | ||
| if (in < ins[eee]) { | ||
| eee -= 2; | ||
| } else { | ||
| eee += 2; | ||
| } | ||
| if (in < ins[eee]) { | ||
| eee -= 1; | ||
| } else { | ||
| eee += 1; | ||
| } | ||
| if (in < ins[eee]) { // expected-warning {{Out of bound access}} | ||
| eee -= 1; | ||
| } | ||
| } | ||
| return eee; | ||
| } |
There was a problem hiding this comment.
This test case is long and convoluted, but does not provide anything interesting from the point of view of the array bound checker.
The bug report is a very straightforward one: the value of eee can be 16 + 8 + 4 + 2 + 1 = 31 (on the branch that always takes else) while the length of the array is only 31 (8 elements with negative exponents + 1 + 22 with positive exponents). This is difficult to spot and understand for a human reader, but does not require unusual effort from the analyzer -- which is exactly the opposite of a good testcase.
By the way this case was added by commit 2d10fdd in 2012 without any explanation or correlated code change.
There was a problem hiding this comment.
This is a nice cleanup change. I have checked the test cases and the reasoning behind the removals.
It is also welcome that these reasons are documented, at least in the PR.
@NagyDonat What do you think about making these reasoning snippets available in the commit message?
I am not sure that it is enough for them to only exist in the PR comments, but if you say that it would be unnecessary verbose to have the gist of them in the commit message, that is fine as well.
|
I think the "why were these testcases redundant?" information is too verbose for a commit message; moreover it only conveys information about code that was removed from the repository, so it won't be relevant for readers in the future who are presumably only interested in code that stays in the repository. |
|
The failing tests are unrelated to the commit, apparently a script called |
…128508) Before commit 6e17ed9 the test files `outofbound.c` and `outofbound-notwork.c` tested the behavior of the old alpha checker `alpha.security.ArrayBound` (V1); then that commit converted them into tests for the checker `security.ArrayBound` which was previously called `alpha.security.ArrayBoundV2`. This commit removes these test files and migrates their useful content to `out-of-bounds.c`. The file `outofbound.c` contained lots of testcases that covered features which are also covered in `out-of-bounds.c` or `out-of-bounds-diagnostics.c`; those redundant cases are discarded during this migration process. This is part of a commit series that reorganizes the tests of `security.ArrayBound` to a system that's easier to understand and maintain.
Before commit 6e17ed9 the test files
outofbound.candoutofbound-notwork.ctested the behavior of the old alpha checkeralpha.security.ArrayBound(V1); then that commit converted them into tests for the checkersecurity.ArrayBoundwhich was previously calledalpha.security.ArrayBoundV2.This commit removes these test files and migrates their useful content to
out-of-bounds.c. The fileoutofbound.ccontained lots of testcases that covered features which are also covered inout-of-bounds.corout-of-bounds-diagnostics.c; those redundant cases are discarded during this migration process.This is part of a commit series that reorganizes the tests of
security.ArrayBoundto system that's easier to understand and maintain.