[PATCH] D107078: [analyzer] Catch leaking stack addresses via stack variables

Balázs Benics via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Tue Aug 17 23:26:17 PDT 2021


steakhal marked an inline comment as done.
steakhal added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp:325-326
+      if (ReferrerMemSpace && ReferredMemSpace) {
+        if (ReferredFrame == PoppedFrame &&
+            ReferrerFrame->isParentOf(PoppedFrame)) {
+          V.emplace_back(Referrer, Referred);
----------------
NoQ wrote:
> You probably meant `||`?
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.

{F18569514}


================
Comment at: clang/lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp:397-398
+                               Ctx.getLocationContext());
+      Report->addNote("The temporary object gets destroyed at the end of the "
+                      "full expression",
+                      L);
----------------
NoQ wrote:
> steakhal wrote:
> > NoQ wrote:
> > > 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"(?)
> > > 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): {F18363520}
> > (CodeChecker): {F18364795}
> > 
> > ---
> > 
> > > 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.
> 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.
Okay, I won't emit the extra note.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D107078/new/

https://reviews.llvm.org/D107078



More information about the cfe-commits mailing list