[NFC][analyzer] OOB test consolidation I: no-outofbounds.c

[NagyDonat]

Before commit 6e17ed9 the test file no-outofbounds.c tested the behavior of the old alpha checker alpha.security.ArrayBound (V1); then that commit converted it into a test for the checker security.ArrayBound which was previously called alpha.security.ArrayBoundV2.

This commit removes this small separate test file and adds some tests to the "native" test files of security.ArrayBound to ensure that the same functionality is still tested.

This is intended as the first commit in a series that reorganizes the tests of security.ArrayBound to system that's easier to understand and maintain.

[llvmbot]

@llvm/pr-subscribers-clang-static-analyzer-1

@llvm/pr-subscribers-clang

Author: Donát Nagy (NagyDonat)

Changes

Before commit 6e17ed9 the test file no-outofbounds.c tested the behavior of the old alpha checker alpha.security.ArrayBound (V1); then that commit converted it into a test for the checker security.ArrayBound which was previously called alpha.security.ArrayBoundV2.

This commit removes this small separate test file and adds some tests to the "native" test files of security.ArrayBound to ensure that the same functionality is still tested.

This is intended as the first commit in a series that reorganizes the tests of security.ArrayBound to system that's easier to understand and maintain.

---

Full diff: https://github.com/llvm/llvm-project/pull/126539.diff

3 Files Affected:

• (removed) clang/test/Analysis/no-outofbounds.c (-32)
• (modified) clang/test/Analysis/out-of-bounds-diagnostics.c (+28-1)
• (modified) clang/test/Analysis/out-of-bounds.c (+1-1)

diff --git a/clang/test/Analysis/no-outofbounds.c b/clang/test/Analysis/no-outofbounds.c
deleted file mode 100644
index c6219ae74ab42ca..000000000000000
--- a/clang/test/Analysis/no-outofbounds.c
+++ /dev/null
@@ -1,32 +0,0 @@
-// RUN: %clang_analyze_cc1 -analyzer-checker=core,alpha.core,alpha.unix,security.ArrayBound -verify %s
-// expected-no-diagnostics
-
-//===----------------------------------------------------------------------===//
-// This file tests cases where we should not flag out-of-bounds warnings.
-//===----------------------------------------------------------------------===//
-
-void f(void) {
-  long x = 0;
-  char *y = (char*) &x;
-  char c = y[0] + y[1] + y[2]; // no-warning
-  short *z = (short*) &x;
-  short s = z[0] + z[1]; // no-warning
-}
-
-void g(void) {
-  int a[2];
-  char *b = (char*)a;
-  b[3] = 'c'; // no-warning
-}
-
-typedef typeof(sizeof(int)) size_t;
-void *malloc(size_t);
-void free(void *);
-
-void field(void) {
-  struct vec { size_t len; int data[0]; };
-  struct vec *a = malloc(sizeof(struct vec) + 10*sizeof(int));
-  a->len = 10;
-  a->data[1] = 5; // no-warning
-  free(a);
-}
diff --git a/clang/test/Analysis/out-of-bounds-diagnostics.c b/clang/test/Analysis/out-of-bounds-diagnostics.c
index 1db01251148e14b..524fa4e2aaaf769 100644
--- a/clang/test/Analysis/out-of-bounds-diagnostics.c
+++ b/clang/test/Analysis/out-of-bounds-diagnostics.c
@@ -231,7 +231,16 @@ int arrayOfStructsArrow(void) {
   // expected-note@-2 {{Access of 'itemArray' at index 35, while it holds only 20 'struct item' elements}}
 }
 
+char convertedScalar(long long var) {
+  char *p = ((char*)&var);
+  (void) p[3]; // no-warning
+  return p[13];
+  // expected-warning@-1 {{Out of bound access to memory after the end of 'var'}}
+  // expected-note@-2 {{Access of 'var' at index 13, while it holds only 8 'char' elements}}
+}
+
 short convertedArray(void) {
+  (void) ((short*)TenElements)[17]; // no-warning
   return ((short*)TenElements)[47];
   // expected-warning@-1 {{Out of bound access to memory after the end of 'TenElements'}}
   // expected-note@-2 {{Access of 'TenElements' at index 47, while it holds only 20 'short' elements}}
@@ -268,23 +277,41 @@ int intFromStringDivisible(void) {
 
 typedef __typeof(sizeof(int)) size_t;
 void *malloc(size_t size);
+void free(void *mem);
 
 int *mallocRegion(void) {
   int *mem = (int*)malloc(2*sizeof(int));
 
+  mem[1] = 48; // no-warning
+
   mem[3] = -2;
   // expected-warning@-1 {{Out of bound access to memory after the end of the heap area}}
   // expected-note@-2 {{Access of the heap area at index 3, while it holds only 2 'int' elements}}
   return mem;
 }
 
+typedef struct { size_t len; int data[0]; } vec_t;
+
+void mallocFlexibleArray(void) {
+  vec_t *v = malloc(sizeof(vec_t) + 10 * sizeof(int));
+  v->len = 10;
+  v->data[1] = 5; // no-warning
+  v->data[11] = 99;
+  // TODO: Here ideally we would expect
+  //     {{Out of bound access to memory after the end of the heap area}}
+  //     {{Access of the heap area at index 11, while it holds only 10 'int' elements}}
+  // but the analyzer cannot (yet) deduce the size of the flexible array member
+  // from the size of the whole allocated area.
+  free(v);
+}
+
 int *custom_calloc(size_t a, size_t b) {
   size_t res;
 
   return __builtin_mul_overflow(a, b, &res) ? 0 : malloc(res);
 }
 
-int *mallocRegionOverflow(void) {
+int *mallocMulOverflow(void) {
   int *mem = (int*)custom_calloc(10, sizeof(int));
 
   mem[20] = 10;
diff --git a/clang/test/Analysis/out-of-bounds.c b/clang/test/Analysis/out-of-bounds.c
index 734a56602e2aa40..923797200d0b403 100644
--- a/clang/test/Analysis/out-of-bounds.c
+++ b/clang/test/Analysis/out-of-bounds.c
@@ -153,7 +153,7 @@ void test_assume_after_access(unsigned long x) {
 int *get_symbolic(void);
 void test_underflow_symbolic(void) {
   int *buf = get_symbolic();
-  buf[-1] = 0; // no-warning;
+  buf[-1] = 0; // no-warning
 }
 
 // But warn if we understand the internal memory layout of a symbolic region.

[NagyDonat]

Meta: can we ensure that commits that modify files in clang/test/Analysis (i.e. the static analyzer lit test directory) are tagged as "clang: static analyzer"? I will have a few follow-up commits like this and don't want to spam that label.

[Xazax-hun]

can we ensure that commits that modify files in clang/test/Analysis

I believe that directory has tests that are independent of the CSA. So we either have false positives or false negatives or might need to reorganise the test folder a bit.

[NagyDonat]

can we ensure that commits that modify files in clang/test/Analysis

I believe that directory has tests that are independent of the CSA.

😲 But why?? Is there any good reason for keeping this mixed test folder, or is this just technical debt from the ancient codebase?

[Xazax-hun]

I think we just never cleaned this up. If someone volunteered, I'd be happy to approve the restructuring PR.

[pskrgag]

tagged as "clang: static analyzer"

I think we can. Labeler logic is located there and there is no clang/test/Analysis
https://github.com/llvm/llvm-project/blob/main/.github/new-prs-labeler.yml#L496