Skip to content

[NFC] [analyzer] Add ArrayBound tests to document casting bug #127062

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Feb 13, 2025

Conversation

NagyDonat
Copy link
Contributor

Add a few security.ArrayBound testcases that document the false positives caused the fact that the analyzer doesn't model a cast from signed char to unsigned char.

Add a few security.ArrayBound testcases that document the false
positives caused the fact that the analyzer doesn't model a cast from
`signed char` to `unsigned char`.
@NagyDonat NagyDonat requested review from haoNoQ and steakhal February 13, 2025 13:42
@llvmbot llvmbot added the clang Clang issues not falling into any other category label Feb 13, 2025
@llvmbot
Copy link
Member

llvmbot commented Feb 13, 2025

@llvm/pr-subscribers-clang

Author: Donát Nagy (NagyDonat)

Changes

Add a few security.ArrayBound testcases that document the false positives caused the fact that the analyzer doesn't model a cast from signed char to unsigned char.


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

1 Files Affected:

  • (modified) clang/test/Analysis/out-of-bounds.c (+31)
diff --git a/clang/test/Analysis/out-of-bounds.c b/clang/test/Analysis/out-of-bounds.c
index 923797200d0b4..177c03500a0c3 100644
--- a/clang/test/Analysis/out-of-bounds.c
+++ b/clang/test/Analysis/out-of-bounds.c
@@ -194,3 +194,34 @@ char test_comparison_with_extent_symbol(struct incomplete *p) {
   return ((char *)p)[-1]; // no-warning
 }
 
+int table[256], small_table[128];
+int test_cast_to_unsigned(signed char x) {
+  unsigned char y = x;
+  if (x >= 0)
+    return x;
+  // FIXME: Here the analyzer ignores the signed -> unsigned cast, and manages to
+  // load a negative value from an unsigned variable. This causes an underflow
+  // report, which is an ugly false positive.
+  // The underlying issue is tracked by Github ticket #39492.
+  return table[y]; // expected-warning {{Out of bound access to memory preceding}}
+}
+
+int test_cast_to_unsigned_overflow(signed char x) {
+  unsigned char y = x;
+  if (x >= 0)
+    return x;
+  // A variant of 'test_cast_to_unsigned' where the correct behavior would be
+  // an overflow report (because the negative values are cast to `unsigned
+  // char` values that are too large).
+  // FIXME: See comment in 'test_cast_to_unsigned'.
+  return small_table[y]; // expected-warning {{Out of bound access to memory preceding}}
+}
+
+int test_negative_offset_with_unsigned_idx(void) {
+  // An example where the subscript operator uses an unsigned index, but the
+  // underflow report is still justified. (We should try to keep this if we
+  // silence false positives like the one in 'test_cast_to_unsigned'.)
+  int *p = table - 10;
+  unsigned idx = 2u;
+  return p[idx]; // expected-warning {{Out of bound access to memory preceding}}
+}

Copy link
Contributor

@steakhal steakhal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense.

// load a negative value from an unsigned variable. This causes an underflow
// report, which is an ugly false positive.
// The underlying issue is tracked by Github ticket #39492.
return table[y]; // expected-warning {{Out of bound access to memory preceding}}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we also dump the value range of this y sym?
That would probably highlight the "wrong assumption"

Maybe in the rest of the examples too. Wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, that seems to be useful, I added the value dumps to the two testcases where they are relevant. (The third testcase is not buggy, I just created it to highlight potential drawbacks of naive solutions for silencing the bug.)

Copy link
Contributor

@steakhal steakhal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great. Thanks.

@NagyDonat NagyDonat changed the title [analyzer][NFC] Add ArrayBound tests to document casting bug [NFC] [analyzer]Add ArrayBound tests to document casting bug Feb 13, 2025
@NagyDonat NagyDonat changed the title [NFC] [analyzer]Add ArrayBound tests to document casting bug [NFC] [analyzer] Add ArrayBound tests to document casting bug Feb 13, 2025
@NagyDonat NagyDonat merged commit d2240cd into llvm:main Feb 13, 2025
5 of 6 checks passed
joaosaffran pushed a commit to joaosaffran/llvm-project that referenced this pull request Feb 14, 2025
…27062)

Add a few security.ArrayBound testcases that document the false
positives caused the fact that the analyzer doesn't model a cast from
`signed char` to `unsigned char`.
sivan-shani pushed a commit to sivan-shani/llvm-project that referenced this pull request Feb 24, 2025
…27062)

Add a few security.ArrayBound testcases that document the false
positives caused the fact that the analyzer doesn't model a cast from
`signed char` to `unsigned char`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants