https://github.com/steakhal updated https://github.com/llvm/llvm-project/pull/97199
>From 6eeab014b09cfa0909c3ccb1b2d3e6fadadb983f Mon Sep 17 00:00:00 2001 From: Balazs Benics <benicsbal...@gmail.com> Date: Mon, 1 Jul 2024 17:29:25 +0200 Subject: [PATCH 1/5] [analyzer] Fix crash in Stream checker when using void pointers We can get zero type size (thus div by zero crash) if the region is for a 'void*' pointer. In this patch, let's just override the void type with a char type to avoid the crash. Fixes https://github.com/llvm/llvm-project/pull/93408#issuecomment-2189766510 --- .../StaticAnalyzer/Checkers/StreamChecker.cpp | 39 ++++++++++++------- clang/test/Analysis/stream.c | 13 +++++++ 2 files changed, 38 insertions(+), 14 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp index 9aee7f952ad2d..0d1d50d19f4c4 100644 --- a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp @@ -1034,16 +1034,19 @@ void StreamChecker::preWrite(const FnDescription *Desc, const CallEvent &Call, C.addTransition(State); } -static std::optional<QualType> getPointeeType(const MemRegion *R) { +static QualType getPointeeType(const MemRegion *R) { if (!R) - return std::nullopt; - if (const auto *ER = dyn_cast<ElementRegion>(R)) - return ER->getElementType(); - if (const auto *TR = dyn_cast<TypedValueRegion>(R)) - return TR->getValueType(); - if (const auto *SR = dyn_cast<SymbolicRegion>(R)) - return SR->getPointeeStaticType(); - return std::nullopt; + return {}; + QualType Ty = [R] { + if (const auto *ER = dyn_cast<ElementRegion>(R)) + return ER->getElementType(); + if (const auto *TR = dyn_cast<TypedValueRegion>(R)) + return TR->getValueType(); + if (const auto *SR = dyn_cast<SymbolicRegion>(R)) + return SR->getPointeeStaticType(); + return QualType{}; + }(); + return !Ty.isNull() ? Ty->getCanonicalTypeUnqualified() : QualType{}; } static std::optional<NonLoc> getStartIndex(SValBuilder &SVB, @@ -1073,7 +1076,16 @@ tryToInvalidateFReadBufferByElements(ProgramStateRef State, CheckerContext &C, const auto *Buffer = dyn_cast_or_null<SubRegion>(Call.getArgSVal(0).getAsRegion()); - std::optional<QualType> ElemTy = getPointeeType(Buffer); + const ASTContext &Ctx = C.getASTContext(); + + QualType ElemTy = getPointeeType(Buffer); + + // Consider pointer to void as a pointer to char buffer such that it has a + // non-zero type size. + if (!ElemTy.isNull() && ElemTy == Ctx.VoidTy) { + ElemTy = Ctx.CharTy; + } + std::optional<SVal> StartElementIndex = getStartIndex(C.getSValBuilder(), Buffer); @@ -1086,10 +1098,9 @@ tryToInvalidateFReadBufferByElements(ProgramStateRef State, CheckerContext &C, std::optional<int64_t> StartIndexVal = getKnownValue(State, StartElementIndex.value_or(UnknownVal())); - if (ElemTy && CountVal && Size && StartIndexVal) { + if (!ElemTy.isNull() && CountVal && Size && StartIndexVal) { int64_t NumBytesRead = Size.value() * CountVal.value(); - int64_t ElemSizeInChars = - C.getASTContext().getTypeSizeInChars(*ElemTy).getQuantity(); + int64_t ElemSizeInChars = Ctx.getTypeSizeInChars(ElemTy).getQuantity(); bool IncompleteLastElement = (NumBytesRead % ElemSizeInChars) != 0; int64_t NumCompleteOrIncompleteElementsRead = NumBytesRead / ElemSizeInChars + IncompleteLastElement; @@ -1097,7 +1108,7 @@ tryToInvalidateFReadBufferByElements(ProgramStateRef State, CheckerContext &C, constexpr int MaxInvalidatedElementsLimit = 64; if (NumCompleteOrIncompleteElementsRead <= MaxInvalidatedElementsLimit) { return escapeByStartIndexAndCount(State, Call, C.blockCount(), Buffer, - *ElemTy, *StartIndexVal, + ElemTy, *StartIndexVal, NumCompleteOrIncompleteElementsRead); } } diff --git a/clang/test/Analysis/stream.c b/clang/test/Analysis/stream.c index db03d90cd8af4..0869b1b325172 100644 --- a/clang/test/Analysis/stream.c +++ b/clang/test/Analysis/stream.c @@ -453,3 +453,16 @@ void getline_buffer_size_negative() { free(buffer); fclose(file); } + +void gh_93408_regression(void *buffer) { + FILE *f = fopen("/tmp/foo.txt", "r"); + fread(buffer, 1, 1, f); // expected-warning {{Stream pointer might be NULL}} no-crash: void is treated as char. + fclose(f); +} + +typedef void VOID; +void gh_93408_regression_typedef(VOID *buffer) { + FILE *f = fopen("/tmp/foo.txt", "r"); + fread(buffer, 1, 1, f); // expected-warning {{Stream pointer might be NULL}} no-crash: VOID is treated as char. + fclose(f); +} >From 4b26e6fb59e4794f12faa7afd6353e4db6d7fba5 Mon Sep 17 00:00:00 2001 From: Balazs Benics <benicsbal...@gmail.com> Date: Mon, 1 Jul 2024 17:29:25 +0200 Subject: [PATCH 2/5] Prefer directly checking against 0 --- .../StaticAnalyzer/Checkers/StreamChecker.cpp | 11 ++---- clang/test/Analysis/stream.c | 36 +++++++++++++++++-- 2 files changed, 37 insertions(+), 10 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp index 0d1d50d19f4c4..530d971fc5305 100644 --- a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp @@ -1077,15 +1077,7 @@ tryToInvalidateFReadBufferByElements(ProgramStateRef State, CheckerContext &C, dyn_cast_or_null<SubRegion>(Call.getArgSVal(0).getAsRegion()); const ASTContext &Ctx = C.getASTContext(); - QualType ElemTy = getPointeeType(Buffer); - - // Consider pointer to void as a pointer to char buffer such that it has a - // non-zero type size. - if (!ElemTy.isNull() && ElemTy == Ctx.VoidTy) { - ElemTy = Ctx.CharTy; - } - std::optional<SVal> StartElementIndex = getStartIndex(C.getSValBuilder(), Buffer); @@ -1101,6 +1093,9 @@ 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) + return nullptr; + bool IncompleteLastElement = (NumBytesRead % ElemSizeInChars) != 0; int64_t NumCompleteOrIncompleteElementsRead = NumBytesRead / ElemSizeInChars + IncompleteLastElement; diff --git a/clang/test/Analysis/stream.c b/clang/test/Analysis/stream.c index 0869b1b325172..c924cbd36f759 100644 --- a/clang/test/Analysis/stream.c +++ b/clang/test/Analysis/stream.c @@ -456,13 +456,45 @@ void getline_buffer_size_negative() { void gh_93408_regression(void *buffer) { FILE *f = fopen("/tmp/foo.txt", "r"); - fread(buffer, 1, 1, f); // expected-warning {{Stream pointer might be NULL}} no-crash: void is treated as char. + fread(buffer, 1, 1, f); // expected-warning {{Stream pointer might be NULL}} no-crash fclose(f); } typedef void VOID; void gh_93408_regression_typedef(VOID *buffer) { FILE *f = fopen("/tmp/foo.txt", "r"); - fread(buffer, 1, 1, f); // expected-warning {{Stream pointer might be NULL}} no-crash: VOID is treated as char. + fread(buffer, 1, 1, f); // expected-warning {{Stream pointer might be NULL}} no-crash + fclose(f); +} + +struct FAM { + int data; + int tail[]; +}; + +struct FAM0 { + int data; + int tail[0]; +}; + +void gh_93408_regression_FAM(struct FAM *p) { + FILE *f = fopen("/tmp/foo.txt", "r"); + fread(p->tail, 1, 1, f); // expected-warning {{Stream pointer might be NULL}} + fclose(f); +} + +void gh_93408_regression_FAM0(struct FAM0 *p) { + FILE *f = fopen("/tmp/foo.txt", "r"); + fread(p->tail, 1, 1, f); // expected-warning {{Stream pointer might be NULL}} + fclose(f); +} + +struct ZeroSized { + int data[0]; +}; + +void gh_93408_regression_ZeroSized(struct ZeroSized *buffer) { + FILE *f = fopen("/tmp/foo.txt", "r"); + fread(buffer, 1, 1, f); // expected-warning {{Stream pointer might be NULL}} no-crash fclose(f); } >From 0f1ad42a0d5b16d7f44178e29490af3fcdfd7bad Mon Sep 17 00:00:00 2001 From: Balazs Benics <benicsbal...@gmail.com> Date: Mon, 1 Jul 2024 17:29:25 +0200 Subject: [PATCH 3/5] Replace immediately invoked lambda by duplicating code --- .../StaticAnalyzer/Checkers/StreamChecker.cpp | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp index 530d971fc5305..b1769081764d5 100644 --- a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp @@ -1037,16 +1037,13 @@ void StreamChecker::preWrite(const FnDescription *Desc, const CallEvent &Call, static QualType getPointeeType(const MemRegion *R) { if (!R) return {}; - QualType Ty = [R] { - if (const auto *ER = dyn_cast<ElementRegion>(R)) - return ER->getElementType(); - if (const auto *TR = dyn_cast<TypedValueRegion>(R)) - return TR->getValueType(); - if (const auto *SR = dyn_cast<SymbolicRegion>(R)) - return SR->getPointeeStaticType(); - return QualType{}; - }(); - return !Ty.isNull() ? Ty->getCanonicalTypeUnqualified() : QualType{}; + if (const auto *ER = dyn_cast<ElementRegion>(R)) + return ER->getElementType()->getCanonicalTypeUnqualified(); + if (const auto *TR = dyn_cast<TypedValueRegion>(R)) + return TR->getValueType()->getCanonicalTypeUnqualified(); + if (const auto *SR = dyn_cast<SymbolicRegion>(R)) + return SR->getPointeeStaticType()->getCanonicalTypeUnqualified(); + return QualType{}; } static std::optional<NonLoc> getStartIndex(SValBuilder &SVB, >From 10ec54a666cf5d92f63594ab443986968f7d7905 Mon Sep 17 00:00:00 2001 From: Balazs Benics <benicsbal...@gmail.com> Date: Mon, 1 Jul 2024 17:29:25 +0200 Subject: [PATCH 4/5] Harmonize return paths --- clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp index b1769081764d5..96e74478c4797 100644 --- a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp @@ -1043,7 +1043,7 @@ static QualType getPointeeType(const MemRegion *R) { return TR->getValueType()->getCanonicalTypeUnqualified(); if (const auto *SR = dyn_cast<SymbolicRegion>(R)) return SR->getPointeeStaticType()->getCanonicalTypeUnqualified(); - return QualType{}; + return {}; } static std::optional<NonLoc> getStartIndex(SValBuilder &SVB, >From fdf2dfdc1483477da665776421daaf896c5d18d3 Mon Sep 17 00:00:00 2001 From: Balazs Benics <benicsbal...@gmail.com> Date: Mon, 1 Jul 2024 17:29:25 +0200 Subject: [PATCH 5/5] Drop the getCanonicalTypeUnqualified calls --- clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp index 96e74478c4797..e8d538388e56c 100644 --- a/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/StreamChecker.cpp @@ -1038,11 +1038,11 @@ static QualType getPointeeType(const MemRegion *R) { if (!R) return {}; if (const auto *ER = dyn_cast<ElementRegion>(R)) - return ER->getElementType()->getCanonicalTypeUnqualified(); + return ER->getElementType(); if (const auto *TR = dyn_cast<TypedValueRegion>(R)) - return TR->getValueType()->getCanonicalTypeUnqualified(); + return TR->getValueType(); if (const auto *SR = dyn_cast<SymbolicRegion>(R)) - return SR->getPointeeStaticType()->getCanonicalTypeUnqualified(); + return SR->getPointeeStaticType(); return {}; } _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
