=?utf-8?q?Donát?= Nagy <donat.n...@ericsson.com> Message-ID: In-Reply-To: <llvm.org/llvm/llvm-project/pull/72...@github.com>
https://github.com/DonatNagyE updated https://github.com/llvm/llvm-project/pull/72402 >From 703c06e2d6781c45e55d7021929a06cdb0275a14 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <donat.n...@ericsson.com> Date: Wed, 15 Nov 2023 16:03:22 +0100 Subject: [PATCH 1/2] [analyzer] Use AllocaRegion in MallocChecker ...to model the results of alloca() and _alloca() calls. Previously it acted as if these functions were returning memory from the heap, which led to alpha.security.ArrayBoundV2 producing incorrect messages. --- .../Core/PathSensitive/SValBuilder.h | 9 ++++++ .../Checkers/BuiltinFunctionChecker.cpp | 29 +++++++++---------- .../StaticAnalyzer/Checkers/MallocChecker.cpp | 10 ++++--- clang/lib/StaticAnalyzer/Core/SValBuilder.cpp | 8 +++++ clang/test/Analysis/malloc.c | 14 +++++++-- clang/test/Analysis/memory-model.cpp | 2 +- .../test/Analysis/out-of-bounds-diagnostics.c | 8 ++--- 7 files changed, 52 insertions(+), 28 deletions(-) diff --git a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h index 692c4384586569e..a64cf7ae4efcb82 100644 --- a/clang/include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h +++ b/clang/include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h @@ -215,6 +215,15 @@ class SValBuilder { const LocationContext *LCtx, QualType type, unsigned Count); + /// Create an SVal representing the result of an alloca()-like call, that is, + /// an AllocaRegion on the stack. + /// + /// After calling this function, it's a good idea to set the extent of the + /// returned AllocaRegion. + loc::MemRegionVal getAllocaRegionVal(const Expr *E, + const LocationContext *LCtx, + unsigned Count); + DefinedOrUnknownSVal getDerivedRegionValueSymbolVal( SymbolRef parentSymbol, const TypedValueRegion *region); diff --git a/clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp index 4a56156de4b27fe..143326c435cf815 100644 --- a/clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp @@ -81,22 +81,21 @@ bool BuiltinFunctionChecker::evalCall(const CallEvent &Call, case Builtin::BI__builtin_alloca_with_align: case Builtin::BI__builtin_alloca: { - // FIXME: Refactor into StoreManager itself? - MemRegionManager& RM = C.getStoreManager().getRegionManager(); - const AllocaRegion* R = - RM.getAllocaRegion(CE, C.blockCount(), C.getLocationContext()); - - // Set the extent of the region in bytes. This enables us to use the - // SVal of the argument directly. If we save the extent in bits, we - // cannot represent values like symbol*8. - auto Size = Call.getArgSVal(0); - if (Size.isUndef()) - return true; // Return true to model purity. - - state = setDynamicExtent(state, R, Size.castAs<DefinedOrUnknownSVal>(), - C.getSValBuilder()); + SValBuilder &SVB = C.getSValBuilder(); + const loc::MemRegionVal R = + SVB.getAllocaRegionVal(CE, C.getLocationContext(), C.blockCount()); - C.addTransition(state->BindExpr(CE, LCtx, loc::MemRegionVal(R))); + // Set the extent of the region in bytes. This enables us to use the SVal + // of the argument directly. If we saved the extent in bits, it'd be more + // difficult to reason about values like symbol*8. + auto Size = Call.getArgSVal(0); + if (auto DefSize = Size.getAs<DefinedOrUnknownSVal>()) { + state = setDynamicExtent(state, R.getRegion(), *DefSize, SVB); + // FIXME: perhaps the following transition should be moved out of the + // 'if' to bind an AllocaRegion (with unknown/unspecified size) even in + // the unlikely case when the size argument is undefined. + C.addTransition(state->BindExpr(CE, LCtx, R)); + } return true; } diff --git a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp index d3a4020280616b0..417305e26c41b09 100644 --- a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp @@ -1731,10 +1731,12 @@ ProgramStateRef MallocChecker::MallocMemAux(CheckerContext &C, // TODO: We could rewrite post visit to eval call; 'malloc' does not have // side effects other than what we model here. unsigned Count = C.blockCount(); - SValBuilder &svalBuilder = C.getSValBuilder(); + SValBuilder &SVB = C.getSValBuilder(); const LocationContext *LCtx = C.getPredecessor()->getLocationContext(); - DefinedSVal RetVal = svalBuilder.getConjuredHeapSymbolVal(CE, LCtx, Count) - .castAs<DefinedSVal>(); + DefinedSVal RetVal = + ((Family == AF_Alloca) ? SVB.getAllocaRegionVal(CE, LCtx, Count) + : SVB.getConjuredHeapSymbolVal(CE, LCtx, Count) + .castAs<DefinedSVal>()); State = State->BindExpr(CE, C.getLocationContext(), RetVal); // Fill the region with the initialization value. @@ -1746,7 +1748,7 @@ ProgramStateRef MallocChecker::MallocMemAux(CheckerContext &C, // Set the region's extent. State = setDynamicExtent(State, RetVal.getAsRegion(), - Size.castAs<DefinedOrUnknownSVal>(), svalBuilder); + Size.castAs<DefinedOrUnknownSVal>(), SVB); return MallocUpdateRefState(C, CE, State, Family); } diff --git a/clang/lib/StaticAnalyzer/Core/SValBuilder.cpp b/clang/lib/StaticAnalyzer/Core/SValBuilder.cpp index e2f0fb6a6731ca0..eb9cde5c8918fc1 100644 --- a/clang/lib/StaticAnalyzer/Core/SValBuilder.cpp +++ b/clang/lib/StaticAnalyzer/Core/SValBuilder.cpp @@ -231,6 +231,14 @@ SValBuilder::getConjuredHeapSymbolVal(const Expr *E, return loc::MemRegionVal(MemMgr.getSymbolicHeapRegion(sym)); } +loc::MemRegionVal SValBuilder::getAllocaRegionVal(const Expr *E, + const LocationContext *LCtx, + unsigned VisitCount) { + const AllocaRegion *R = + getRegionManager().getAllocaRegion(E, VisitCount, LCtx); + return loc::MemRegionVal(R); +} + DefinedSVal SValBuilder::getMetadataSymbolVal(const void *symbolTag, const MemRegion *region, const Expr *expr, QualType type, diff --git a/clang/test/Analysis/malloc.c b/clang/test/Analysis/malloc.c index a3f7a69b8cef347..14b7555c2c58197 100644 --- a/clang/test/Analysis/malloc.c +++ b/clang/test/Analysis/malloc.c @@ -266,13 +266,18 @@ void CheckUseZeroAllocated1(void) { } char CheckUseZeroAllocated2(void) { + // FIXME: The return value of `alloca()` is modeled with `AllocaRegion` + // instead of `SymbolicRegion`, so the current implementation of + // `MallocChecker::checkUseZeroAllocated()` cannot handle it; and we get an + // unrelated, but suitable warning from core.uninitialized.UndefReturn. char *p = alloca(0); - return *p; // expected-warning {{Use of memory allocated with size zero}} + return *p; // expected-warning {{Undefined or garbage value returned to caller}} } char CheckUseZeroWinAllocated2(void) { + // FIXME: Same situation as `CheckUseZeroAllocated2()`. char *p = _alloca(0); - return *p; // expected-warning {{Use of memory allocated with size zero}} + return *p; // expected-warning {{Undefined or garbage value returned to caller}} } void UseZeroAllocated(int *p) { @@ -727,6 +732,11 @@ void paramFree(int *p) { myfoo(p); // expected-warning {{Use of memory after it is freed}} } +void allocaFree(void) { + int *p = alloca(sizeof(int)); + free(p); // expected-warning {{Memory allocated by alloca() should not be deallocated}} +} + int* mallocEscapeRet(void) { int *p = malloc(12); return p; // no warning diff --git a/clang/test/Analysis/memory-model.cpp b/clang/test/Analysis/memory-model.cpp index fbdef4fddbea93b..fd5a286acb60c06 100644 --- a/clang/test/Analysis/memory-model.cpp +++ b/clang/test/Analysis/memory-model.cpp @@ -97,7 +97,7 @@ void symbolic_malloc() { void symbolic_alloca() { int *a = (int *)alloca(12); - clang_analyzer_dump(a); // expected-warning {{Element{HeapSymRegion{conj}} + clang_analyzer_dump(a); // expected-warning {{Element{alloca{}} clang_analyzer_dumpExtent(a); // expected-warning {{12 S64b}} clang_analyzer_dumpElementCount(a); // expected-warning {{3 S64b}} } diff --git a/clang/test/Analysis/out-of-bounds-diagnostics.c b/clang/test/Analysis/out-of-bounds-diagnostics.c index 4f5d0d0bc91c54a..da1573665fa7951 100644 --- a/clang/test/Analysis/out-of-bounds-diagnostics.c +++ b/clang/test/Analysis/out-of-bounds-diagnostics.c @@ -107,12 +107,8 @@ void *alloca(size_t size); int allocaRegion(void) { int *mem = (int*)alloca(2*sizeof(int)); 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}} - // FIXME: this should be - // {{Out of bound access to memory after the end of the memory returned by 'alloca'}} - // {{Access of the memory returned by 'alloca' at index 3, while it holds only 2 'int' elements}} - // but apparently something models 'alloca' as if it was allocating on the heap + // expected-warning@-1 {{Out of bound access to memory after the end of the memory returned by 'alloca'}} + // expected-note@-2 {{Access of the memory returned by 'alloca' at index 3, while it holds only 2 'int' elements}} return *mem; } >From 537cb9e2e50fa8bd321564ea44414c870c68de0e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Don=C3=A1t=20Nagy?= <donat.n...@ericsson.com> Date: Mon, 20 Nov 2023 11:17:28 +0100 Subject: [PATCH 2/2] Update comment for the case __builtin_alloca(<Undefined>) --- .../lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp b/clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp index 143326c435cf815..61521c259ca90a1 100644 --- a/clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp +++ b/clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp @@ -90,12 +90,11 @@ bool BuiltinFunctionChecker::evalCall(const CallEvent &Call, // difficult to reason about values like symbol*8. auto Size = Call.getArgSVal(0); if (auto DefSize = Size.getAs<DefinedOrUnknownSVal>()) { + // This `getAs()` is mostly paranoia, because core.CallAndMessage reports + // undefined function arguments (unless it's disabled somehow). state = setDynamicExtent(state, R.getRegion(), *DefSize, SVB); - // FIXME: perhaps the following transition should be moved out of the - // 'if' to bind an AllocaRegion (with unknown/unspecified size) even in - // the unlikely case when the size argument is undefined. - C.addTransition(state->BindExpr(CE, LCtx, R)); } + C.addTransition(state->BindExpr(CE, LCtx, R)); return true; } _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
