llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-static-analyzer-1 @llvm/pr-subscribers-clang Author: None (DonatNagyE) <details> <summary>Changes</summary> ...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. --- Full diff: https://github.com/llvm/llvm-project/pull/72402.diff 7 Files Affected: - (modified) clang/include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h (+9) - (modified) clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp (+14-15) - (modified) clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp (+6-4) - (modified) clang/lib/StaticAnalyzer/Core/SValBuilder.cpp (+8) - (modified) clang/test/Analysis/malloc.c (+12-2) - (modified) clang/test/Analysis/memory-model.cpp (+1-1) - (modified) clang/test/Analysis/out-of-bounds-diagnostics.c (+2-6) ``````````diff 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; } `````````` </details> https://github.com/llvm/llvm-project/pull/72402 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
