This is an archive of the discontinued LLVM Phabricator instance.

[analyzer] Catch leaking stack addresses via stack variables
ClosedPublic

Authored by steakhal on Jul 29 2021, 7:36 AM.

Details

Summary

Not only global variables can hold references to dead stack variables.
Consider this example:

void write_stack_address_to(char **q) {
  char local;
  *q = &local;
}

void test_stack() {
  char *p;
  write_stack_address_to(&p);
}

The address of local is assigned to p, which becomes a dangling
pointer after write_stack_address_to() returns.

The StackAddrEscapeChecker was looking for bindings in the store which
referred to variables of the popped stack frame, but it only considered
global variables in this regard. This patch relaxes this, catching
stack variable bindings as well.


This patch also works for temporary objects like:

struct Bar {
  const int &ref;
  explicit Bar(int y) : ref(y) {
    // Okay.
  } // End of the constructor call, `ref` is dangling now. Warning!
};

void test() {
  Bar{33}; // Temporary object, so the corresponding memregion is
           // *not* a VarRegion.
}

The return value optimization aka. copy-elision might kick in but that
is modeled by passing an imaginary CXXThisRegion which refers to the
parent stack frame which is supposed to be the return slot.
Objects residing in the return slot outlive the scope of the inner
call, thus we should expect no warning about them - except if we
explicitly disable copy-elision.

Diff Detail

Event Timeline

steakhal created this revision.Jul 29 2021, 7:36 AM
steakhal requested review of this revision.Jul 29 2021, 7:36 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 29 2021, 7:36 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
steakhal updated this revision to Diff 362791.Jul 29 2021, 8:18 AM

Remove the CXXThisRegion edge case handling.

This is awesome!

clang/test/Analysis/copy-elision.cpp
9–10

Should we use -verify=no-elide here as well? Since we set the DNO_ELIDE_FLAG?

195

It would be useful to have a test with // RUN: -analyzer-output=text \ to make sure that we have a note placed for v at consume(make3(v)). Do we have such a note? This could be done perhaps in another test file.

steakhal marked an inline comment as done.Jul 29 2021, 8:40 AM

Thanks for the quick response.

clang/test/Analysis/copy-elision.cpp
9–10

According to the comment a few lines below:

Copy elision always occurs in C++17, otherwise it's under an on-by-default flag.

So I think even though one passes the elide-constructors=false analyzer config, we follow the language semantics, which requires us to elide those copies, thus no copy should happen.

steakhal planned changes to this revision.Jul 29 2021, 9:36 AM
steakhal marked an inline comment as done.

I'm going to check how the notes look like on real code.

clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
397–399

I'm actually not sure about this. We the genName() returns the appropriate SourceRange for CXXTempObjects, which will be added to the bug report regardless. That might be enough. I'm going to check that.

clang/test/Analysis/copy-elision.cpp
195

I could do that, but we should consider the size of the test file.
The notes will duplicate each expected-warning and add even more besides those.

Should I add them to this file or to another test file covering the same?
If we decide to have that I think it would be valuable to do that in a follow-up patch IMO.
The size of this is already hitting the limit.

400–419

I'm going to move this somewhere else.
Probably outside of the namespace address_vector_tests to the end of this file.

steakhal requested review of this revision.Jul 29 2021, 10:10 AM
steakhal added inline comments.
clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
397–399

getName() operates on the Referred memregion, although I'm highlighting the Referrer memregion. So, the note is not redundant.

martong added inline comments.Jul 30 2021, 6:31 AM
clang/test/Analysis/copy-elision.cpp
9–10

If copy elision always occur then DNO_ELIDE_FLAG has no effect, and as such it is misleading to have it in this RUN line.

steakhal updated this revision to Diff 363101.Jul 30 2021, 8:12 AM

Removed the deceiving -DNO_ELIDE_FLAG define from the c++17 run line.

steakhal marked an inline comment as done.Jul 30 2021, 8:12 AM
steakhal added inline comments.
clang/test/Analysis/copy-elision.cpp
9–10

You are right. Fixed.

NoQ added a comment.Jul 30 2021, 4:58 PM

Y'all writing really good patches lately.

The updates on the C++ tests look spot on to me. I'm not sure if these tests contain any actual UB but the checker was anyway designed to be a bit more aggressive than that and that's exactly the kind of stuff we wanted it to warn us about. And even if it turns out to have been a bad idea from the start, that's a separate story.

I'll have some bikeshedding about warning and note text but overall I really like it.

clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
397–398

This is part of the path, right? We're reporting these bugs at checkEndFunction rather than at the store site so the destruction has already happened by the time we report it. In this case it should be a path note, not an extra blue-bubble note.

As for wording, I suggest full-expression with a dash (that's what other clang diagnostics use) and probably drop the initial "The"(?)

steakhal marked an inline comment as done.Aug 5 2021, 2:30 AM

Y'all writing really good patches lately.

Awesome, thank you!

clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
397–398

This is part of the path, right? We're reporting these bugs at checkEndFunction rather than at the store site so the destruction has already happened by the time we report it. In this case it should be a path note, not an extra blue-bubble note.

I'm not exactly sure how to address this. AFAIK there is no way currently detecting the completion of a full-expression.
I was thinking of Check::RegionChanges, Check::PostStmt<Expr>, Check::Live, but none of these fits my needs. Do you have anything specific in mind that could help me?

Aside from that, the report looks reasonable even with a 'blue' bubble:
(scan-build):


(CodeChecker):


As for wording, I suggest full-expression with a dash (that's what other clang diagnostics use) and probably drop the initial "The"(?)

I see, thanks.

steakhal updated this revision to Diff 364393.Aug 5 2021, 2:33 AM
  • The temporary -> Temporary
  • full expression -> full-expression

I did not fix the extra blue 'bubble' note issue, and I think that is out of the scope of this patch.

NoQ added a comment.Aug 8 2021, 11:21 PM

I did not fix the extra blue 'bubble' note issue, and I think that is out of the scope of this patch.

This is an on-by-default checker. We should not knowingly regress it, even if temporarily.

clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
397–398

Wait, no, in your example the end of the full-expression is not part of the path; it happens after the warning is emitted.

If it was, the right thing to do would have been to catch the destruction of the object of interest rather than the end of the full-expression as such.

But in this case the note seems to be redundant. Lifetime of the temporary object is fairly irrelevant to the report. It only matters that such lifetime is longer than that of the stack address. But the note doesn't tell the user how long that lifetime is; it only says how short it is. So i think this note is redundant.

steakhal updated this revision to Diff 365133.Aug 9 2021, 2:48 AM
steakhal marked 2 inline comments as done.

Removed the extra note about the temporary expression.


I did not fix the extra blue 'bubble' note issue, and I think that is out of the scope of this patch.

This is an on-by-default checker. We should not knowingly regress it, even if temporarily.

I'm not exactly sure what would I regress, but to be on the safe side I won't emit any extra notes this time.

WDYT about the current form?

martong accepted this revision.Aug 17 2021, 10:07 AM

It looks good to me, my concerns are addressed. But please wait for @NoQ 's approval.

This revision is now accepted and ready to land.Aug 17 2021, 10:07 AM
NoQ added a comment.Aug 17 2021, 11:11 AM

I did not fix the extra blue 'bubble' note issue, and I think that is out of the scope of this patch.

This is an on-by-default checker. We should not knowingly regress it, even if temporarily.

I'm not exactly sure what would I regress, but to be on the safe side I won't emit any extra notes this time.

Yup that's what i meant, you can't introduce a note and address problems with it in a follow-up patch; it should be either fully addressed immediately or go under a flag until fixed. I think we're good now that the note is not there.

I think i see one bug in the code but other than that i think we're good to go.

clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
325–326

You probably meant ||?

steakhal marked an inline comment as done.Aug 17 2021, 11:26 PM
steakhal added inline comments.
clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
325–326

No, I think && is justified here. We have to make sure that the popped frame is the one that was referred to by some other frame, below that frame.

397–398

Okay, I won't emit the extra note.

steakhal marked an inline comment as done.Aug 26 2021, 3:39 AM

I plan to commit to this tomorrow. @NoQ @martong

Oh wait, it's not yet accepted by @NoQ. Then, consider this as a polite ping.

NoQ accepted this revision.Aug 26 2021, 1:43 PM

Thanks, all clear now!

clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
325–326

Uh-oh, I misunderstood the whole thing. Looks correct indeed!