[analyzer] Fix StreamChecker crash in fread modeling #108393
Conversation
In #93408 69bc159 I refined how invalidation is done for `fread`. It can crash, if the "size" or "count" parameters of "fread" is a perfectly contrained negative value. In such cases, when it will try to allocate a SmallVEctor with a negative size, which will cause a crash. To mitigate this issue, let's just guard against negative values. CPP-3247
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-static-analyzer-1 Author: Balazs Benics (steakhal) ChangesIn #93408 69bc159 To mitigate this issue, let's just guard against negative values. CPP-3247 Full diff: https://github.com/llvm/llvm-project/pull/108393.diff 2 Files Affected:
diff --git a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
index 22061373c4b393..8bb7880a3cc283 100644
--- a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp
@@ -1129,7 +1129,7 @@ tryToInvalidateFReadBufferByElements(ProgramStateRef State, CheckerContext &C,
if (!ElemTy.isNull() && CountVal && Size && StartIndexVal) {
int64_t NumBytesRead = Size.value() * CountVal.value();
int64_t ElemSizeInChars = Ctx.getTypeSizeInChars(ElemTy).getQuantity();
- if (ElemSizeInChars == 0)
+ if (ElemSizeInChars == 0 || NumBytesRead < 0)
return nullptr;
bool IncompleteLastElement = (NumBytesRead % ElemSizeInChars) != 0;
diff --git a/clang/test/Analysis/fread.c b/clang/test/Analysis/fread.c
index 3f286421fd7a13..d470f0abebe621 100644
--- a/clang/test/Analysis/fread.c
+++ b/clang/test/Analysis/fread.c
@@ -443,3 +443,33 @@ void test_unaligned_start_read(void) {
fclose(fp);
}
}
+
+void no_crash_if_count_is_negative(long s, unsigned char *buffer) {
+ FILE *fp = fopen("path", "r");
+ if (fp) {
+ if (s * s == -1) {
+ fread(buffer, 1, s * s, fp); // no-crash
+ }
+ fclose(fp);
+ }
+}
+
+void no_crash_if_size_is_negative(long s, unsigned char *buffer) {
+ FILE *fp = fopen("path", "r");
+ if (fp) {
+ if (s * s == -1) {
+ fread(buffer, s * s, 1, fp); // no-crash
+ }
+ fclose(fp);
+ }
+}
+
+void no_crash_if_size_and_count_are_negative(long s, unsigned char *buffer) {
+ FILE *fp = fopen("path", "r");
+ if (fp) {
+ if (s * s == -1) {
+ fread(buffer, s * s, s * s, fp); // no-crash
+ }
+ fclose(fp);
+ }
+}
|
|
I'm debating if we should backport this to clang-19, but I'd say this crash does not happen too often in real life to advocate for it. So I don't intend to backport this, unless someone asks for it. |
clang/test/Analysis/fread.c
Outdated
| void no_crash_if_count_is_negative(long s, unsigned char *buffer) { | ||
| FILE *fp = fopen("path", "r"); | ||
| if (fp) { | ||
| if (s * s == -1) { |
There was a problem hiding this comment.
Nitpick: 0xffff...f f digits and two consecutive integers that are >1 cannot be both perfect squares). I know that the static analyzer doesn't know this and will happily assume that s * s == -1 -- but it would be still nicer to use a different example.
There was a problem hiding this comment.
Yea, I noticed that too. I figured it's a funny example, but refining it is also good.
3 participants
In #93408 69bc159
I refined how invalidation is done for
fread. It can crash, if the "size" or "count" parameters of "fread" is a perfectly constrained negative value. In such cases, when it will try to allocate a SmallVector with a negative size, which will cause a crash.To mitigate this issue, let's just guard against negative values.
CPP-3247