-
Notifications
You must be signed in to change notification settings - Fork 14.3k
[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
Conversation
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`.
@llvm/pr-subscribers-clang Author: Donát Nagy (NagyDonat) ChangesAdd a few security.ArrayBound testcases that document the false positives caused the fact that the analyzer doesn't model a cast from Full diff: https://github.com/llvm/llvm-project/pull/127062.diff 1 Files Affected:
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}}
+}
|
There was a problem hiding this 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}} |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great. Thanks.
…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`.
…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`.
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
tounsigned char
.