Skip to content

[analyzer] Use AllocaRegion in MallocChecker #72402

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Nov 28, 2023
Merged

[analyzer] Use AllocaRegion in MallocChecker #72402

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
703c06e
[analyzer] Use AllocaRegion in MallocChecker
NagyDonat Nov 15, 2023
537cb9e
Update comment for the case __builtin_alloca(<Undefined>)
NagyDonat Nov 20, 2023
2a8d1f6
Tweak comments
NagyDonat Nov 24, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions clang/include/clang/StaticAnalyzer/Core/PathSensitive/SValBuilder.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down
28 changes: 13 additions & 15 deletions clang/lib/StaticAnalyzer/Checkers/BuiltinFunctionChecker.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
// 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);
}
C.addTransition(state->BindExpr(CE, LCtx, R));
return true;
}

Expand Down
14 changes: 8 additions & 6 deletions clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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);
}
Expand Down
8 changes: 8 additions & 0 deletions clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
17 changes: 15 additions & 2 deletions clang/test/Analysis/malloc.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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}}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we'd have no warning in case of

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?)

Copy link
Contributor Author

@NagyDonat NagyDonat Nov 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I'll test that. (Edit: I tried and the error disappears if I add *p = 4. However, as I write below, ArrayBoundV2 is already checking this kind of overflow, and that checker produces an useful error message even in the case when *p = 4 is added.)

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.

Copy link
Contributor Author

@NagyDonat NagyDonat Nov 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. What do you think about this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

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) {
Expand Down Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion clang/test/Analysis/memory-model.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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}}
}
Expand Down
8 changes: 2 additions & 6 deletions clang/test/Analysis/out-of-bounds-diagnostics.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand Down