Skip to content

[analyzer][NFC] Add tests for and refactor StackAddrEscapeChecker 1/3 #105652

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 2 commits into from
Aug 26, 2024
Merged

[analyzer][NFC] Add tests for and refactor StackAddrEscapeChecker 1/3 #105652

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
db68dcf
[analyzer] [NFC] Add tests for and refactor StackAddrEscapeChecker
necto Aug 20, 2024
83d2af4
Merge branch 'main' into az/CPP-4734-stack-leak-refactor
necto Aug 26, 2024
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
71 changes: 38 additions & 33 deletions clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -288,12 +288,37 @@ void StackAddrEscapeChecker::checkPreStmt(const ReturnStmt *RS,
EmitStackError(C, R, RetE);
}

std::optional<std::string> printReferrer(const MemRegion *Referrer) {
assert(Referrer);
const StringRef ReferrerMemorySpace = [](const MemSpaceRegion *Space) {
if (isa<StaticGlobalSpaceRegion>(Space))
return "static";
if (isa<GlobalsSpaceRegion>(Space))
return "global";
assert(isa<StackSpaceRegion>(Space));
return "stack";
}(Referrer->getMemorySpace());

// We should really only have VarRegions here.
// Anything else is really surprising, and we should get notified if such
// ever happens.
const auto *ReferrerVar = dyn_cast<VarRegion>(Referrer);
if (!ReferrerVar) {
assert(false && "We should have a VarRegion here");
return std::nullopt; // Defensively skip this one.
}
const std::string ReferrerVarName =
ReferrerVar->getDecl()->getDeclName().getAsString();

return (ReferrerMemorySpace + " variable '" + ReferrerVarName + "'").str();
}

void StackAddrEscapeChecker::checkEndFunction(const ReturnStmt *RS,
CheckerContext &Ctx) const {
if (!ChecksEnabled[CK_StackAddrEscapeChecker])
return;

ProgramStateRef State = Ctx.getState();
ExplodedNode *Node = Ctx.getPredecessor();

// Iterate over all bindings to global variables and see if it contains
// a memory region in the stack space.
Expand All @@ -315,15 +340,10 @@ void StackAddrEscapeChecker::checkEndFunction(const ReturnStmt *RS,
if (!ReferrerMemSpace || !ReferredMemSpace)
return false;

const auto *ReferrerFrame = ReferrerMemSpace->getStackFrame();
const auto *ReferredFrame = ReferredMemSpace->getStackFrame();

if (ReferrerMemSpace && ReferredMemSpace) {
if (ReferredFrame == PoppedFrame &&
ReferrerFrame->isParentOf(PoppedFrame)) {
V.emplace_back(Referrer, Referred);
return true;
}
if (ReferredMemSpace->getStackFrame() == PoppedFrame &&
ReferrerMemSpace->getStackFrame()->isParentOf(PoppedFrame)) {
V.emplace_back(Referrer, Referred);
return true;
}
return false;
}
Expand Down Expand Up @@ -352,14 +372,15 @@ void StackAddrEscapeChecker::checkEndFunction(const ReturnStmt *RS,
};

CallBack Cb(Ctx);
ProgramStateRef State = Node->getState();
State->getStateManager().getStoreManager().iterBindings(State->getStore(),
Cb);

if (Cb.V.empty())
return;

// Generate an error node.
ExplodedNode *N = Ctx.generateNonFatalErrorNode(State);
ExplodedNode *N = Ctx.generateNonFatalErrorNode(State, Node);
if (!N)
return;

Expand All @@ -374,13 +395,13 @@ void StackAddrEscapeChecker::checkEndFunction(const ReturnStmt *RS,

// Generate a report for this bug.
const StringRef CommonSuffix =
"upon returning to the caller. This will be a dangling reference";
" upon returning to the caller. This will be a dangling reference";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is having two spaces after a dot a common convention we have? This is not a change in this PR, I'm just wondering if this was intentional or a typo.

Copy link
Contributor

Choose a reason for hiding this comment

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

It is an older typographical convention that was historically common: https://en.wikipedia.org/wiki/Sentence_spacing

However, in the last 10-20 years it became obsolete almost everywhere, and I don't recall any other message where the static analyzer uses it. I'd guess that it was intentional (perhaps the personally preferred style of some contributor?), but we should probably return to single spacing.

Copy link
Contributor Author

@necto necto Aug 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I found this style in a few other messages:

Call to 'dispatch_once' uses the local variable 'once' for the predicate value. Using such transient memory for the predicate is potentially dangerous. Perhaps you intended to declare the variable as 'static'?

link

Object leaked: object allocated and stored into 'object' is returned from a function whose name ('CFGetRuleViolation') does not contain 'Copy' or 'Create'. This violates the naming convention rules given in the Memory Management Guide for Core Foundation

link

The return value from the call to 'setuid' is not checked. If an error occurs in 'setuid', the following code may execute with unexpected privileges

link

Function 'rand' is obsolete because it implements a poor random number generator. Use 'arc4random' instead

link

Overall, if lit-test messages stats is of any indication, it is relatively balanced:

grep -R '\(warning\|note\){.*\.  [A-Za-z]' clang/test/ | wc

-> 23 (double space after dot)

grep -R '\(warning\|note\){.*\. [A-Za-z]' clang/test/ | wc

-> 37 (single space after dot)

SmallString<128> Buf;
llvm::raw_svector_ostream Out(Buf);
const SourceRange Range = genName(Out, Referred, Ctx.getASTContext());

if (isa<CXXTempObjectRegion, CXXLifetimeExtendedObjectRegion>(Referrer)) {
Out << " is still referred to by a temporary object on the stack "
Out << " is still referred to by a temporary object on the stack"
<< CommonSuffix;
auto Report =
std::make_unique<PathSensitiveBugReport>(*BT_stackleak, Out.str(), N);
Expand All @@ -390,28 +411,12 @@ void StackAddrEscapeChecker::checkEndFunction(const ReturnStmt *RS,
return;
}

const StringRef ReferrerMemorySpace = [](const MemSpaceRegion *Space) {
if (isa<StaticGlobalSpaceRegion>(Space))
return "static";
if (isa<GlobalsSpaceRegion>(Space))
return "global";
assert(isa<StackSpaceRegion>(Space));
return "stack";
}(Referrer->getMemorySpace());

// We should really only have VarRegions here.
// Anything else is really surprising, and we should get notified if such
// ever happens.
const auto *ReferrerVar = dyn_cast<VarRegion>(Referrer);
if (!ReferrerVar) {
assert(false && "We should have a VarRegion here");
continue; // Defensively skip this one.
auto ReferrerVariable = printReferrer(Referrer);
if (!ReferrerVariable) {
continue;
}
const std::string ReferrerVarName =
ReferrerVar->getDecl()->getDeclName().getAsString();

Out << " is still referred to by the " << ReferrerMemorySpace
<< " variable '" << ReferrerVarName << "' " << CommonSuffix;
Out << " is still referred to by the " << *ReferrerVariable << CommonSuffix;
auto Report =
std::make_unique<PathSensitiveBugReport>(*BT_stackleak, Out.str(), N);
if (Range.isValid())
Expand Down
31 changes: 31 additions & 0 deletions clang/test/Analysis/stack-addr-ps.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,3 +95,34 @@ void callTestRegister(void) {
char buf[20];
testRegister(buf); // no-warning
}

void top_level_leaking(int **out) {
int local = 42;
*out = &local; // no-warning FIXME
}

void callee_leaking_via_param(int **out) {
int local = 1;
*out = &local;
// expected-warning@-1{{Address of stack memory associated with local variable 'local' is still referred to by the stack variable 'ptr'}}
}

void caller_for_leaking_callee() {
int *ptr = 0;
callee_leaking_via_param(&ptr);
}

void callee_nested_leaking(int **out) {
int local = 1;
*out = &local;
// expected-warning@-1{{Address of stack memory associated with local variable 'local' is still referred to by the stack variable 'ptr'}}
}

void caller_mid_for_nested_leaking(int **mid) {
callee_nested_leaking(mid);
}

void caller_for_nested_leaking() {
int *ptr = 0;
caller_mid_for_nested_leaking(&ptr);
}
Loading
Loading