[clang] [analyzer] Use AllocaRegion in MallocChecker (PR #72402)
https://github.com/DonatNagyE created
https://github.com/llvm/llvm-project/pull/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.
>From 703c06e2d6781c45e55d7021929a06cdb0275a14 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Don=C3=A1t=20Nagy?=
Date: Wed, 15 Nov 2023 16:03:22 +0100
Subject: [PATCH] [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(),
- 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()) {
+ 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
[clang] [analyzer] Use AllocaRegion in MallocChecker (PR #72402)
llvmbot wrote:
@llvm/pr-subscribers-clang-static-analyzer-1
@llvm/pr-subscribers-clang
Author: None (DonatNagyE)
Changes
...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(),
- 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()) {
+ 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 RetVal =
+ ((Family == AF_Alloca) ? SVB.getAllocaRegionVal(CE, LCtx, Count)
+ : SVB.getConjuredHeapSymbolVal(CE, L
[clang] [analyzer] Use AllocaRegion in MallocChecker (PR #72402)
haoNoQ wrote:
Hmm, it really worries me that `MallocChecker` is setting a return value
outside of `evalCall()`. This can easily lead to conflicts if multiple checkers
try to do this: `evalCall()` is protected from conflicts (the engine asserts
that at most one checker evaluates each call) but `checkPostCall()` isn't.
To the best of my knowledge, the only legal way to use `State->BindExpr(...)`
in a checker is to set the return value in `evalCall()`. Ideally we should have
an assertion about this ("Environment is unchanged after checker callback
invocation, unless the callback is a successful `evalCall()`").
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
[clang] [analyzer] Use AllocaRegion in MallocChecker (PR #72402)
@@ -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}}
haoNoQ wrote:
So we'd have no warning in case of
```c++
char CheckUseZeroAllocatedAndInitialized(void) {
char *p = alloca(0);
*p = 4;
return *p;
}
```
? Might be worth testing.
(It's probably not hard to fix it as well? It's not like `AllocaRegion` is
special when it comes to being able to carry dynamic extent?)
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
[clang] [analyzer] Use AllocaRegion in MallocChecker (PR #72402)
@@ -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}}
DonatNagyE wrote:
Ok, I'll test that.
Unfortunately this "allocated with size zero" report is based on the private
"Symbol -> state enum" map that's maintained by MallocChecker (so it's
independent of the dynamic extent). I'd guess that switching to dynamic extent
wouldn't be too difficult and it could simplify the code, but I think that
belongs to a separate commit.
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
[clang] [analyzer] Use AllocaRegion in MallocChecker (PR #72402)
DonatNagyE wrote:
> Hmm, it really worries me that `MallocChecker` is setting a return value
> outside of `evalCall()`. This can easily lead to conflicts if multiple
> checkers try to do this: `evalCall()` is protected from conflicts (the engine
> asserts that at most one checker evaluates each call) but `checkPostCall()`
> isn't.
>
> To the best of my knowledge, the only legal way to use `State->BindExpr(...)`
> in a checker is to set the return value in `evalCall()`. Ideally we should
> have an assertion about this ("Environment is unchanged after checker
> callback invocation, unless the callback is a successful `evalCall()`").
I agree and there's already a FIXME which suggests moving this modeling step
into an EvalCall callback. If you feel that this is an important improvement, I
could create a separate commit that does it.
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
[clang] [analyzer] Use AllocaRegion in MallocChecker (PR #72402)
@@ -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}}
DonatNagyE wrote:
Also note that the report "Use of memory allocated with size zero" is redundant
with ArrayBoundV2, which detects and reports that the offset (0) is not smaller
than the extent (also 0). Based on this I'm not sure that it's useful to
maintain this "size zero" special case.
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
[clang] [analyzer] Use AllocaRegion in MallocChecker (PR #72402)
https://github.com/DonatNagyE edited 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
[clang] [analyzer] Use AllocaRegion in MallocChecker (PR #72402)
https://github.com/DonatNagyE edited 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
[clang] [analyzer] Use AllocaRegion in MallocChecker (PR #72402)
@@ -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(),
- 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()) {
+ state = setDynamicExtent(state, R.getRegion(), *DefSize, SVB);
+ // FIXME: perhaps the following transition should be moved out of the
gamesh411 wrote:
I would go with first asserting that the Size is DefinedOrUnknown anyway, and
if we have a crash with a reproducer, then we can add the if and the test case
for it.
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
[clang] [analyzer] Use AllocaRegion in MallocChecker (PR #72402)
https://github.com/gamesh411 approved this pull request. LGTM, added two remarks inline, but those can be separate patches as well. 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
[clang] [analyzer] Use AllocaRegion in MallocChecker (PR #72402)
https://github.com/gamesh411 edited 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
[clang] [analyzer] Use AllocaRegion in MallocChecker (PR #72402)
@@ -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}}
gamesh411 wrote:
Even if it is not the real question, what we are to do with the 0-size `alloca`
calls, but just to highlight some practical concerns, I found these sources:
https://discourse.llvm.org/t/malloc-free-and-alloca-with-zero-size/9284/3
https://stackoverflow.com/questions/8036654/what-does-alloca0-do-and-return-on-various-platforms
So `alloca(0)` sometimes has a special meaning. If we can give more specific
error messages in these cases, I would prefer to handle those error messages in
the more specific checker.
Even if ArrayBoundV2 has more user-friendly and mature error reporting (and
would cover this case strictly speaking), making this more specific checker
emit better diagnostics as well is something worth considering IMO.
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
[clang] [analyzer] Use AllocaRegion in MallocChecker (PR #72402)
@@ -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}}
DonatNagyE wrote:
It seems that `alloca()` in general and `alloca(0)` in particular can mean many
things, and I don't think that it's worth to create a specific error message
because I cannot say anything concrete in it. This is a nonstandard function,
and while we can model its "basic" behavior, I think that we shouldn't try to
deal with its corner cases.
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
[clang] [analyzer] Use AllocaRegion in MallocChecker (PR #72402)
=?utf-8?q?Donát?= Nagy
Message-ID:
In-Reply-To:
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?=
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(),
- 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()) {
+ 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 RetVal =
+ ((Family == AF_Alloca) ?
[clang] [analyzer] Use AllocaRegion in MallocChecker (PR #72402)
=?utf-8?q?Donát?= Nagy
Message-ID:
In-Reply-To:
@@ -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(),
- 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()) {
+ state = setDynamicExtent(state, R.getRegion(), *DefSize, SVB);
+ // FIXME: perhaps the following transition should be moved out of the
DonatNagyE wrote:
I checked the situation with an assert and a straightforward testcase, and it
turns out that currently the `getAs()` always succeeds, because
`core.CallAndMessage` stops the execution with a bug report when an undefined
argument is passed to a function. (However, if I disabled this functionality of
`core.CallAndMessage`, the test -- obviously -- triggered the assertion.)
Based on this, I decided to keep the current defensive check, because I didn't
want to add a hidden and marginal dependency relationship between these two
checkers. I know that we may assume that `core` checkers are always on, but
that doesn't mean that I should hide landmines like `assert(core.CallAndMessage
is active and works as I assume)` in unrelated checkers.
I updated the comment to explain this situation, and I also moved the
`addTransition()` call out of the `if` because that's the more logical place
for that method call and this `supporting fire` from `core.CallAndMessage`
guarantees that this change won't have unintended side effects (because the
`if` succeeds in normal operation).
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
[clang] [analyzer] Use AllocaRegion in MallocChecker (PR #72402)
=?utf-8?q?Donát?= Nagy Message-ID: In-Reply-To: https://github.com/DonatNagyE edited 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
[clang] [analyzer] Use AllocaRegion in MallocChecker (PR #72402)
=?utf-8?q?Don=C3=A1t?= Nagy Message-ID: In-Reply-To: DonatNagyE wrote: @haoNoQ What do you think about this commit? May I merge it? 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
[clang] [analyzer] Use AllocaRegion in MallocChecker (PR #72402)
Xazax-hun wrote: I think the original `alloca(0)` warning message might be clearer/easier to understand. While it might have platform or compiler dependent meaning, those behaviors are non-portable, so I think it is undesirable in most cases and people probably want to be notified about it. Regarding binding the return value outside of `evalCall`, I think that could be addressed in a separate PR. This one does not make the current situation any worse. But the very least we should add a FIXME now (and potentially open a ticket), to reduce the chances of this getting forgotten. 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
[clang] [analyzer] Use AllocaRegion in MallocChecker (PR #72402)
=?utf-8?q?Donát?= Nagy Message-ID: In-Reply-To: DonatNagyE wrote: > I think the original `alloca(0)` warning message might be clearer/easier to > understand. The difficulty is that we switch from the `SymbolicRegion` that represents the heap allocation to an `AllocaRegion` that doesn't have an associated symbol, while the code that produces the warnings about zero-allocated memory areas (essentially) maintains a set of _symbols_ that correspond to zero-allocated regions (I'd guess that this was implemented before the introduction of dynamic extent). It would be possible to reintroduce the original `alloca(0)` message (while keeping the improvement that `alloca` returns an `AllocaRegion` and not a `SymbolicRegion` on the heap), but that would require significant refactoring (and/or ugly code duplication) and frankly I feel that `alloca` is rare and this would be a waste of time. > While it might have platform or compiler dependent meaning, those behaviors > are non-portable, so I think it is undesirable in most cases and people > probably want to be notified about it. I'd say that "non-portable" and "undesirable in most cases" is true for _practically all_ applications of `alloca`, not just the `alloca(0)` corner case. Experienced programmers who use `alloca` check that their code works with _their own particular_ `alloca` and wouldn't want to see generic messages that may or may not be relevant for them; while novices ( / projects that don't want to deal with `alloca` issues) would be better served by a generic "don't use `alloca`, it's platform-specific" warning. > Regarding binding the return value outside of `evalCall`, I think that could > be addressed in a separate PR. This one does not make the current situation > any worse. But the very least we should add a FIXME now (and potentially open > a ticket), to reduce the chances of this getting forgotten. > > Edit: we already have a TODO, but I think that one does not really do justice > to the urgency of the problem, it does not mention that doing bindings in > checkers outside of `evalCall` is not a good idea. I'll spice up that TODO note; and after merging this commit I could start working on fixing it. 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
[clang] [analyzer] Use AllocaRegion in MallocChecker (PR #72402)
=?utf-8?q?Donát?= Nagy ,
=?utf-8?q?Donát?= Nagy
Message-ID:
In-Reply-To:
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?=
Date: Wed, 15 Nov 2023 16:03:22 +0100
Subject: [PATCH 1/3] [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(),
- 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()) {
+ 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 RetVal =
+
[clang] [analyzer] Use AllocaRegion in MallocChecker (PR #72402)
=?utf-8?q?Donát?= Nagy , =?utf-8?q?Donát?= Nagy Message-ID: In-Reply-To: https://github.com/steakhal approved this pull request. Overall, I'm in favor of this change. On the other hand, I'd urge for not to regress on the diagnostics. To me, `alloca` is like a VLA; which is prone to misuses, thus the edge-cases count there (esp. if tainted). 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
[clang] [analyzer] Use AllocaRegion in MallocChecker (PR #72402)
=?utf-8?q?Donát?= Nagy , =?utf-8?q?Donát?= Nagy Message-ID: In-Reply-To: DonatNagyE wrote: I'm merging this commit in its current shape because even if I'd reimplement a warning for the "use of `alloc(0)`", I'd do it in a separate commit. I thought about potential approaches to implement this "use of zero-allocated" warning for the `AllocaRegion`s and it seems to be troublesome, so I lean towards not doing it. 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
[clang] [analyzer] Use AllocaRegion in MallocChecker (PR #72402)
=?utf-8?q?Donát?= Nagy , =?utf-8?q?Donát?= Nagy Message-ID: In-Reply-To: https://github.com/DonatNagyE closed 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
