[clang] 0424546 - [analyzer] Use AllocaRegion in MallocChecker (#72402)
via cfe-commits
cfe-commits at lists.llvm.org
Tue Nov 28 07:34:49 PST 2023
Author: DonatNagyE
Date: 2023-11-28T16:34:44+01:00
New Revision: 0424546ed4a7570837626922edc66530cd3c3ab7
URL: https://github.com/llvm/llvm-project/commit/0424546ed4a7570837626922edc66530cd3c3ab7
DIFF: https://github.com/llvm/llvm-project/commit/0424546ed4a7570837626922edc66530cd3c3ab7.diff
LOG: [analyzer] Use AllocaRegion in MallocChecker (#72402)
...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.
Added:
Modified:
clang/include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h
clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
clang/test/Analysis/malloc.c
clang/test/Analysis/memory-model.cpp
clang/test/Analysis/out-of-bounds-diagnostics.c
Removed:
################################################################################
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..61521c259ca90a1 100644
--- a/clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
@@ -81,22 +81,20 @@ 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
+ //
diff icult 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);
+ }
+ 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..c5e4add501886a4 100644
--- a/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
+++ b/clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
@@ -1728,13 +1728,15 @@ ProgramStateRef MallocChecker::MallocMemAux(CheckerContext &C,
return nullptr;
// Bind the return value to the symbolic value from the heap region.
- // TODO: We could rewrite post visit to eval call; 'malloc' does not have
- // side effects other than what we model here.
+ // TODO: move use of this functions to an EvalCall callback, becasue
+ // BindExpr() should'nt be used elsewhere.
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..09cd4b0bfce6387 100644
--- a/clang/test/Analysis/malloc.c
+++ b/clang/test/Analysis/malloc.c
@@ -266,13 +266,21 @@ void CheckUseZeroAllocated1(void) {
}
char CheckUseZeroAllocated2(void) {
+ // NOTE: The `AllocaRegion` that models the return value of `alloca()`
+ // doesn't have an associated symbol, so the current implementation of
+ // `MallocChecker::checkUseZeroAllocated()` cannot provide warnings for it.
+ // However, other checkers like core.uninitialized.UndefReturn (that
+ // activates in these TCs) or the array bound checkers provide more generic,
+ // but still sufficient warnings in these cases, so I think it isn't
+ // important to cover this in MallocChecker.
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) {
+ // Note: 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 +735,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;
}
More information about the cfe-commits
mailing list