[clang] [analyzer] Use AllocaRegion in MallocChecker (PR #72402)
via cfe-commits
cfe-commits at lists.llvm.org
Wed Nov 15 07:13:37 PST 2023
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 at -1 {{Out of bound access to memory after the end of the heap area}}
- // expected-note at -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 at -1 {{Out of bound access to memory after the end of the memory returned by 'alloca'}}
+ // expected-note at -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
More information about the cfe-commits
mailing list