Skip to content

[analyzer] Refactor MallocChecker to use BindExpr in evalCall #106081

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.

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 5 commits into from
Sep 16, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ DefinedOrUnknownSVal getDynamicElementCount(ProgramStateRef State,

/// Set the dynamic extent \p Extent of the region \p MR.
ProgramStateRef setDynamicExtent(ProgramStateRef State, const MemRegion *MR,
DefinedOrUnknownSVal Extent, SValBuilder &SVB);
DefinedOrUnknownSVal Extent);

/// Get the dynamic extent for a symbolic value that represents a buffer. If
/// there is an offsetting to the underlying buffer we consider that too.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -215,17 +215,17 @@ class SValBuilder {
/// Conjure a symbol representing heap allocated memory region.
///
/// Note, the expression should represent a location.
DefinedOrUnknownSVal getConjuredHeapSymbolVal(const Expr *E,
const LocationContext *LCtx,
unsigned Count);
DefinedSVal getConjuredHeapSymbolVal(const Expr *E,
const LocationContext *LCtx,
unsigned Count);

/// Conjure a symbol representing heap allocated memory region.
///
/// Note, now, the expression *doesn't* need to represent a location.
/// But the type need to!
DefinedOrUnknownSVal getConjuredHeapSymbolVal(const Expr *E,
const LocationContext *LCtx,
QualType type, unsigned Count);
DefinedSVal getConjuredHeapSymbolVal(const Expr *E,
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.
Expand Down
372 changes: 237 additions & 135 deletions clang/lib/StaticAnalyzer/Checkers/MallocChecker.cpp

Large diffs are not rendered by default.

3 changes: 1 addition & 2 deletions clang/lib/StaticAnalyzer/Checkers/VLASizeChecker.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -266,7 +266,6 @@ void VLASizeChecker::checkPreStmt(const DeclStmt *DS, CheckerContext &C) const {
return;

ASTContext &Ctx = C.getASTContext();
SValBuilder &SVB = C.getSValBuilder();
ProgramStateRef State = C.getState();
QualType TypeToCheck;

Expand Down Expand Up @@ -301,7 +300,7 @@ void VLASizeChecker::checkPreStmt(const DeclStmt *DS, CheckerContext &C) const {
if (VD) {
State =
setDynamicExtent(State, State->getRegion(VD, C.getLocationContext()),
ArraySize.castAs<NonLoc>(), SVB);
ArraySize.castAs<NonLoc>());
}

// Remember our assumptions!
Expand Down
2 changes: 1 addition & 1 deletion clang/lib/StaticAnalyzer/Core/DynamicExtent.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ DefinedOrUnknownSVal getDynamicElementCountWithOffset(ProgramStateRef State,
}

ProgramStateRef setDynamicExtent(ProgramStateRef State, const MemRegion *MR,
DefinedOrUnknownSVal Size, SValBuilder &SVB) {
DefinedOrUnknownSVal Size) {
MR = MR->StripCasts();

if (Size.isUnknown())
Expand Down
3 changes: 1 addition & 2 deletions clang/lib/StaticAnalyzer/Core/ExprEngineCallAndReturn.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -817,8 +817,7 @@ ProgramStateRef ExprEngine::bindReturnValue(const CallEvent &Call,
if (Size.isUndef())
Size = UnknownVal();

State = setDynamicExtent(State, MR, Size.castAs<DefinedOrUnknownSVal>(),
svalBuilder);
State = setDynamicExtent(State, MR, Size.castAs<DefinedOrUnknownSVal>());
} else {
R = svalBuilder.conjureSymbolVal(nullptr, E, LCtx, ResultTy, Count);
}
Expand Down
22 changes: 12 additions & 10 deletions clang/lib/StaticAnalyzer/Core/SValBuilder.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -210,22 +210,24 @@ DefinedOrUnknownSVal SValBuilder::conjureSymbolVal(const Stmt *stmt,
return nonloc::SymbolVal(sym);
}

DefinedOrUnknownSVal
SValBuilder::getConjuredHeapSymbolVal(const Expr *E,
const LocationContext *LCtx,
unsigned VisitCount) {
DefinedSVal SValBuilder::getConjuredHeapSymbolVal(const Expr *E,
const LocationContext *LCtx,
unsigned VisitCount) {
QualType T = E->getType();
return getConjuredHeapSymbolVal(E, LCtx, T, VisitCount);
}

DefinedOrUnknownSVal
SValBuilder::getConjuredHeapSymbolVal(const Expr *E,
const LocationContext *LCtx,
QualType type, unsigned VisitCount) {
DefinedSVal SValBuilder::getConjuredHeapSymbolVal(const Expr *E,
const LocationContext *LCtx,
QualType type,
unsigned VisitCount) {
assert(Loc::isLocType(type));
assert(SymbolManager::canSymbolicate(type));
if (type->isNullPtrType())
return makeZeroVal(type);
if (type->isNullPtrType()) {
// makeZeroVal() returns UnknownVal only in case of FP number, which
// is not the case.
return makeZeroVal(type).castAs<DefinedSVal>();
}

SymbolRef sym = SymMgr.conjureSymbol(E, LCtx, type, VisitCount);
return loc::MemRegionVal(MemMgr.getSymbolicHeapRegion(sym));
Expand Down
17 changes: 0 additions & 17 deletions clang/test/Analysis/NewDelete-checker-test.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -37,10 +37,6 @@ extern "C" void *malloc(size_t);
extern "C" void free (void* ptr);
int *global;

//------------------
// check for leaks
//------------------

//----- Standard non-placement operators
void testGlobalOpNew() {
void *p = operator new(0);
Expand All @@ -67,19 +63,6 @@ void testGlobalNoThrowPlacementExprNewBeforeOverload() {
int *p = new(std::nothrow) int;
} // leak-warning{{Potential leak of memory pointed to by 'p'}}

//----- Standard pointer placement operators
void testGlobalPointerPlacementNew() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you briefly explain why do you delete this testcase? (I'm just curious -- I don't see an obvious reason, but I didn't dig into the handling of placement operators, so I presume that this is justified.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure! This tests checks that CSA looks into new implementation and sees that it just returns some constant pointer. Since PR changes this behavior, test no longer works and CSA reports memory leaks

int i;

void *p1 = operator new(0, &i); // no warn

void *p2 = operator new[](0, &i); // no warn

int *p3 = new(&i) int; // no warn

int *p4 = new(&i) int[0]; // no warn
}

//----- Other cases
void testNewMemoryIsInHeap() {
int *p = new int;
Expand Down
4 changes: 2 additions & 2 deletions clang/test/Analysis/NewDelete-intersections.mm
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,13 @@
// RUN: -analyzer-checker=core \
// RUN: -analyzer-checker=cplusplus.NewDelete

// leak-no-diagnostics

// RUN: %clang_analyze_cc1 -std=c++11 -DLEAKS -fblocks %s \
// RUN: -verify=leak \
// RUN: -analyzer-checker=core \
// RUN: -analyzer-checker=cplusplus.NewDeleteLeaks

// leak-no-diagnostics

// RUN: %clang_analyze_cc1 -std=c++11 -DLEAKS -fblocks %s \
// RUN: -verify=mismatch \
// RUN: -analyzer-checker=core \
Expand Down
35 changes: 0 additions & 35 deletions clang/test/Analysis/malloc-interprocedural.c
Original file line number Diff line number Diff line change
Expand Up @@ -98,38 +98,3 @@ int uafAndCallsFooWithEmptyReturn(void) {
fooWithEmptyReturn(12);
return *x; // expected-warning {{Use of memory after it is freed}}
}


// If we inline any of the malloc-family functions, the checker shouldn't also
// try to do additional modeling.
char *strndup(const char *str, size_t n) {
if (!str)
return 0;

// DO NOT FIX. This is to test that we are actually using the inlined
// behavior!
if (n < 5)
return 0;

size_t length = strlen(str);
if (length < n)
n = length;

char *result = malloc(n + 1);
memcpy(result, str, n);
result[n] = '\0';
return result;
}

void useStrndup(size_t n) {
if (n == 0) {
(void)strndup(0, 20); // no-warning
return;
} else if (n < 5) {
(void)strndup("hi there", n); // no-warning
return;
} else {
(void)strndup("hi there", n);
return; // expected-warning{{leak}}
}
}
Loading