new leaks caught on return, can they be caught on throw?

[AE1020]

(Note: I'm using Ubuntu clang version 18.1.3, but I also pulled and built 20.0.0 to see if things were different, or if there were any new settings.)

Leaks of unpaired new seem to be caught well so long as you are using return:

const char* Leak_IS_Detected_Test(int x) {
    bool* guard = new bool;
    if (x > 0)
        return "This path leaks, and is reported";
    delete guard;
    return "This path has no leak";
}

$ clang --analyze caught.cpp
caught.cpp:4:16: warning: Potential leak of memory pointed to by 'guard' [cplusplus.NewDeleteLeaks]
4 | return "This path leaks, and is reported";

But I would also like to catch new leaks (and/or malloc() leaks) in the cases of throws:

const char* Leak_NOT_Detected_Test(int x) {
    bool* guard = new bool;
    if (x > 0)
        throw "This path leaks, but doesn't report";
    delete guard;
    return "This path has no leak";
}

Is there some fiddling of settings such that this could be considered a leak? I got an AI suggestion to try an alternate path diagnostics mode, but if this was the correct incantation it didn't have any effect:

clang --analyze -Xanalyzer -analyzer-config -Xanalyzer path-diagnostics-alternate=true not-caught.cpp

For what I am doing, I'm not concerned whether it's a literal throw or a function that calls the throw with an attribute. But I've gathered that noreturn functions are conservatively assumed to possibly be exit()s so they do not generate reports.

If throws cannot be checked, are there other attributes besides noreturn which would get the same checks as a return statement in terms of control flow?

[llvmbot]

@llvm/issue-subscribers-clang-static-analyzer

Author: None (AE1020)

*(Note: I'm using Ubuntu clang version 18.1.3, but I also pulled and built 20.0.0 to see if things were different, or if there were any new settings.)*

Leaks of unpaired new seem to be caught well so long as you are using return:

const char* Leak_IS_Detected_Test(int x) {
    bool* guard = new bool;
    if (x > 0)
        return "This path leaks, and is reported";
    delete guard;
    return "This path has no leak";
}

> $ clang --analyze caught.cpp
> caught.cpp:4:16: warning: Potential leak of memory pointed to by 'guard' [cplusplus.NewDeleteLeaks]
> 4 | return "This path leaks, and is reported";

But I would also like to catch new leaks (and/or malloc() leaks) in the cases of throws:

const char* Leak_NOT_Detected_Test(int x) {
    bool* guard = new bool;
    if (x > 0)
        throw "This path leaks, but doesn't report";
    delete guard;
    return "This path has no leak";
}

Is there some fiddling of settings such that this could be considered a leak? I got an AI suggestion to try an alternate path diagnostics mode, but if this was the correct incantation it didn't have any effect:

clang --analyze -Xanalyzer -analyzer-config -Xanalyzer path-diagnostics-alternate=true not-caught.cpp

For what I am doing, I'm not concerned whether it's a literal throw or a function that calls the throw with an attribute. But I've gathered that noreturn functions are conservatively assumed to possibly be exit()s so they do not generate reports.

If throws cannot be checked, are there other attributes besides noreturn which would get the same checks as a return statement in terms of control flow?

[steakhal]

Exceptions are not really well handled in the engine.
I had a look at the CFG, and that actually looks good to me: We have a branch for the true case, and that has a Throw, and the basic block is then linked to the exit CFG basic block. So far so good.
My guess is that in the main handler loop inside ExprEngine.cpp we don't have a VisitThrowStmt case, thus even though the statement is there to simulate, we just don't simulate it and then sink the execution path - without finishing that path with calling PostStmt and the cleanup handlers. This would mean that the checkers wouldn't be notified by the check::DeadSymbols callback, thus never have a chance in the MallocChecker to raise the issue.
I don't see any theoretical limitation of not raising here.

I don't have time to dig any deeper, but let me know if you would - then I could give you more hints along the way.

[AE1020]

Looking at ExprEngine.cpp, there is a FIXME comment on the Stmt::CXXThrowExprClass case...

case Stmt::ObjCAtThrowStmtClass:
case Stmt::CXXThrowExprClass:
  // FIXME: This is not complete.  We basically treat @throw as
  // an abort.
  Bldr.generateSink(S, Pred, Pred->getState());
  break;

Good to see that it's just not implemented (vs. some philosophical objection to detecting it!)

So the MallocChecker::HandleLeak() function is actually getting called in the throw case. But then, the "sink" node that was added suppresses any reporting. If you change the /*SuppressOnSink=*/true below to a false then it will report:

// Leaks should not be reported if they are post-dominated by a sink:
// (1) Sinks are higher importance bugs.
// (2) NoReturnFunctionChecker uses sink nodes to represent paths ending
//     with __noreturn functions such as assert() or exit(). We choose not
//     to report leaks on such paths.
BT_Leak[*CheckKind].reset(new BugType(CheckNames[*CheckKind], "Memory leak",
                                      categories::MemoryError,
                                      /*SuppressOnSink=*/true));  // !!! if false, will report

My guess is that in the main handler loop inside ExprEngine.cpp we don't have a VisitThrowStmt case

I tried deleting the Bldr.generateSink() from the Stmt::CXXThrowExprClass and replacing it with a near-clone of what is done for the Stmt::ReturnStmtClass:

case Stmt::ReturnStmtClass:  // this is what's there for return...
  Bldr.takeNodes(Pred);
  VisitReturnStmt(cast<ReturnStmt>(S), Pred, Dst);
  Bldr.addNodes(Dst);
  break;

case Stmt::CXXThrowExprClass:  // !!! blind copying of return's pattern
  Bldr.takeNodes(Pred);
  VisitCXXThrowExpr(cast<CXXThrowExpr>(S), Pred, Dst);
  Bldr.addNodes(Dst);
  break;

Not that I expected the following to work, but... I tried making a ExprEngine::VisitCXXThrowExpr() function that mirrored ExprEngine::VisitReturnStmt():

void ExprEngine::VisitCXXThrowExpr(const CXXThrowExpr *TE, ExplodedNode *Pred,
                                   ExplodedNodeSet &Dst) {
  ExplodedNodeSet dstPreVisit;
  getCheckerManager().runCheckersForPreStmt(dstPreVisit, Pred, TE, *this);

  StmtNodeBuilder B(dstPreVisit, Dst, *currBldrCtx);

  if (TE->getSubExpr()) {  // throw with argument
    for (ExplodedNodeSet::iterator it = dstPreVisit.begin(),
                                  ei = dstPreVisit.end(); it != ei; ++it) {
      B.generateNode(TE, *it, (*it)->getState());
    }
  }  // else no argument (e.g. a rethrow)
}

It doesn't crash, but doesn't help (still no report). I would guess whatever is spitting out the "non-sinking-end-of-function" pattern of nodes one wants is methodized specifically to ReturnStmt, and not something that CXXThrowExpr has "pre-checkers" registered for.

I don't have time to dig any deeper, but let me know if you would - then I could give you more hints along the way.

Hints welcome...!

[AE1020]

So the MallocChecker::HandleLeak() function is actually getting called in the throw case. But then, the "sink" node that was added suppresses any reporting.

Actually... it seems leaving the switch case for CXXThrowExpr completely empty (not adding a sink) doesn't have any effect. It doesn't act like the throw isn't there (in cases where you comment the throw out, it reports).

So without the Bldr.generateSink() there, HandleLeak() is still called... and setting sink suppression to false still reports the error.

It's like it generates the effect of a sink regardless. 🤔

[steakhal]

Thanks for the investigation.
The idea of suppress on sink is that if other checkers report a deadly issue, then the malloc checker wouldn't just add one more bug report on top that one. (I think)
For example, consider that we would reach a delete statement bug we just happened to encounter a null-pointer dereference on the way, thus never reaching the delete expression. We should not report a leak in that case.
So this makes sense. However, it also hurts here if the ExplodedNode is a sink node, but it's not because of a bug report (not from a checker).

Luckily, ExplodedNodes always have a tag if they come from a checker - for example if some checker reported an error.

Now, what this means for us is that we may be able to substitute the "suppress on sink" with always reporting the leak but adding a special BugReportVisitor to the report that would check if the bug report is a Leak report, and the ErrorNode comes from a checker (e.g. has a tag or something). If this is from a checker, mark the bug report invalid, otherwise do nothing - and let the bug report through.

I could be very wrong about this one, because I don't really know much about the "suppress on sink" mechanism, I'm just sharing my rough idea how I'd attack this problem.

[steakhal]

It may be worth reading what exactly we do for suppress on sink in *PathSensitiveBugReporter::findReportInEquivalenceClass.

[AE1020]

Looking at the git blame may be helpful... it seems the original commit with the FIXME comment was for the Objective-C throw, by @tkremenek in 2008 (active on GitHub this year under that handle):

In GRExprEngine treat @throw as an 'abort' that ends the current path. This is a temporary solution.

The corresponding C++ handling was added with a longer commit message in 2012 by @jrose-apple (last active in 2019 on GitHub under that handle):

Our current handling of 'throw' is all CFG-based: it jumps to a 'catch' block
if there is one and the function exit block if not. But this doesn't really
get the right behavior when a function is inlined: execution will continue on
the caller's side, which is always the wrong thing to do.

Even within a single function, 'throw' completely skips any destructors that
are to be run. This is essentially the same problem as @finally -- a CFGBlock
that can have multiple entry points, whose exit points depend on whether it
was entered normally or exceptionally.

Representing 'throw' as a sink matches our current (non-)handling of @throw.
It's not a perfect solution, but it's better than continuing analysis in an
inconsistent or even impossible state.

(It references a link in Apples "Radar" database of rdar://problem/1211371, if someone has access to that.)

The comment suggests that continuing analysis without the sink would be an "inconsistent or even impossible state". But it also suggests it jumps to the function's exit block. As I noted, commenting out the sink did not get "function exit block" behavior... at least not in the sense of reporting a leak in a non-inlined case, it still acted like there was a sink on this simple case:

const char* Simpler_Leak_NOT_Detected_Test(int x) {
    bool* guard = new bool;
    *guard = true;
    if (*guard)
       *guard = false;
   throw "Leak doesn't report with sink included OR commented out";
}

Although... there is a test file exceptions.mm which gives an inlined example of a throw, and mentions it as a false negative for the leak check:

void inlinedCXX() {
  clang_analyzer_checkInlined(true); // expected-warning{{TRUE}}
  throw -1;
}

int testCXX() {
  int a; // uninitialized
  // FIXME: this should be reported as a leak, because C++ exceptions are
  // often not fatal.
  void *mem = malloc(4);
  inlinedCXX();
  free(mem);
  return a; // no-warning
}

In this case of inlining a function that throws, there is a difference from commenting out the sink... it reports a warning on return a when the Bldr.generateSink() is commented out:

exceptions.cpp:14:7: warning: Undefined or garbage value returned to caller [core.uninitialized.UndefReturn]
14 | return a; // no-warning

With Bldr.generateSink() left in, there is no warning.

So inlining seems to be the reason it can't be handled just like return would be, jumping to the function's exit block.

[AE1020]

The idea of suppress on sink is that if other checkers report a deadly issue, then the malloc checker wouldn't just add one more bug report on top that one. (I think)

From the language of what a "sink" is, it does seem to speak that way (by calling them "higher priority errors"). Then there is this secondary facet where it's also used for noreturn functions.

(I'll mention it is the case that noreturn functions can be like throws in the sense that not all terminate the program--either because they use throw in their implementation or in C use a longjmp(). But coming up with a way to annotate a function to discern whether it is noreturn-terminating or noreturn-NON-terminating looks like a more invasive change than adjusting the reporting for throw, which is mostly enough for the purposes of what I am trying to check for.)

Now, what this means for us is that we may be able to substitute the "suppress on sink" with always reporting the leak but adding a special BugReportVisitor to the report that would check if the bug report is a Leak report, and the ErrorNode comes from a checker (e.g. has a tag or something). If this is from a checker, mark the bug report invalid, otherwise do nothing - and let the bug report through.

Selectively changing the reporting behavior for some sinks certainly looks like the most promising path. First of all because MallocChecker::HandleLeak() is getting called in the throw case, so the information is at hand. And having throw jump to the exit block instead of generating a sink is disproven by the case of inlining a function with a throw in it.

Barring adding a likely-too-invasive nuance to noreturn functions, their non-reporting nature must be preserved. So there has to be something that distinguishes a not-an-error "throw sink" (report leaks) from a not-an-error "noreturn sink" (do NOT report leaks).

Is there some targeted way to decorate the sink at the moment of adding it? The Bldr in Bldr.generateSink() is:

StmtNodeBuilder Bldr(Pred, DstTop, *currBldrCtx);

Its definition of generateSink() is:

ExplodedNode *generateSink(const Stmt *S,
                           ExplodedNode *Pred,
                           ProgramStateRef St,
                           const ProgramPointTag *tag = nullptr,
                           ProgramPoint::Kind K = ProgramPoint::PostStmtKind){
  const ProgramPoint &L = ProgramPoint::getProgramPoint(S, K,
                                Pred->getLocationContext(), tag);
  return NodeBuilder::generateSink(L, St, Pred);
}

So it is defaulting a ProgramPointTag to nullptr, and a ProgramPoint::Kind to PostStmtKind. Is this a sort of place where "not an error suppressing sink" could be slipped on as a tag or program point kind?

case Stmt::ObjCAtThrowStmtClass:
case Stmt::CXXThrowExprClass:
    Bldr.generateSink(S, Pred, Pred->getState(), <<ThrowDontSuppressLeaksTag>>);
    // ...or...
    Bldr.generateSink(S, Pred, Pred->getState(), nullptr, <<ProgramPoint::ThrowKind>>);
    break;

Then could this information be inspected on the sink and (somehow) override the /*SuppressOnSink=*/true in the HandleLeak bug reporter?

The idea would be that other sinks (including noreturn) would not carry this tag/point, and still be suppressed.

[steakhal]

The idea of suppress on sink is that if other checkers report a deadly issue, then the malloc checker wouldn't just add one more bug report on top that one. (I think)

From the language of what a "sink" is, it does seem to speak that way (by calling them "higher priority errors"). Then there is this secondary facet where it's also used for noreturn functions.

Correct. In both of these cases the abstract machine halts the execution, thus that execution path stops, aka. sinks.

(I'll mention it is the case that noreturn functions can be like throws in the sense that not all terminate the program--either because they use throw in their implementation or in C use a longjmp(). But coming up with a way to annotate a function to discern whether it is noreturn-terminating or noreturn-NON-terminating looks like a more invasive change than adjusting the reporting for throw, which is mostly enough for the purposes of what I am trying to check for.)

Correct. From the analyzer's perspective it's the same - since we don't model the control-flow of exceptions, and we don't plan to model them. That would be a really large effort, for a limited value. Consequently, I think it's better to patch the Malloc checker to not suppress its findings in this case.

Selectively changing the reporting behavior for some sinks certainly looks like the most promising path. First of all because MallocChecker::HandleLeak() is getting called in the throw case, so the information is at hand. And having throw jump to the exit block instead of generating a sink is disproven by the case of inlining a function with a throw in it.

Yes for both.

Barring adding a likely-too-invasive nuance to noreturn functions, their non-reporting nature must be preserved. So there has to be something that distinguishes a not-an-error "throw sink" (report leaks) from a not-an-error "noreturn sink" (do NOT report leaks).

Exactly. Every bug report is attached to an ExplodedNode in the ExplodedGraph, that represents all the execution paths in the simulation of a top-level entry point. Every bug report is issues by something - pretty much always if not always from Checkers. Such exploded nodes will have a ProgramPoint (accessed by getLocation()) which has an option tag (getTag()) - but this is always set for nodes coming from a checker. For other sinks, e.g. of a noreturn function, I don't think we have a tag associated with the Node.

I think you had a pretty deep investigation of the relevant code pieces, good job.
I think what you suggested for patching generateSink calls for CXXThrowExprClass and friends could make sense.

My idea was to somehow substitute the /*SuppressOnSink=*/true in this checker and use something else.
For this, we should understand why actually /*SuppressOnSink=*/true does in PathSensitiveBugReporter::findReportInEquivalenceClass.
After this, we can think about if it still makes sense to /*SuppressOnSink=*/true or false.

Any why does SuppressOnSink suppress the report in your original example:

const char* Leak_NOT_Detected_Test(int x) {
    bool* guard = new bool;
    if (x > 0)
        throw "This path leaks, but doesn't report";
    delete guard;
    return "This path has no leak";
}

To me, the leak at the } token shouldn't be post-dominated by sink nodes in the exploded graph.

[AE1020]

I think you had a pretty deep investigation of the relevant code pieces, good job.

Maybe another year or so where I can make sense of code that the AIs can't. 😸

Any why does SuppressOnSink suppress the report in your original example:
To me, the leak at the } token shouldn't be post-dominated by sink nodes in the exploded graph.

Not sure what you're saying (?) We know it acts like this:

const char* Leak_NOT_Detected_Test(int x) {
    bool* guard = new bool;
    if (x > 0)
        abort();  // same interpretation as throw in existing code
    delete guard;
    return "This path has no leak";
}

If you're asking what the exact mechanic of suppression is, I hadn't found that yet, but...

My idea was to somehow substitute the /SuppressOnSink=/true in this checker and use something else.

My hope was just to be able to pick one of ProgramPointTag and ProgramPoint::Kind to distinguish the node at generation time. And then, when that moment of suppression is discovered, don't suppress if it's that node kind. If that works, then no interfaces or parameterizations would have to be changed.

But I didn't try it yet, I just posted about it here, so I don't know if it is viable. Or even if it is, if it violates some other overarching strategy for usage of those fields (for instance: would extending ProgramPoint::Kind create some kind of explosion of needing to handle it in other places, or require bumping a version number of some by-product files where these numbers are written, etc.) So I was asking if the kind/tag seemed a fit for the purpose.

Anyway, I'm happy to let you try something if you have more informed approaches. But that was my idea for what I would try with no other guidance.

[steakhal]

But I didn't try it yet, I just posted about it here, so I don't know if it is viable. Or even if it is, if it violates some other overarching strategy for usage of those fields (for instance: would extending ProgramPoint::Kind create some kind of explosion of needing to handle it in other places, or require bumping a version number of some by-product files where these numbers are written, etc.) So I was asking if the kind/tag seemed a fit for the purpose.

Add a new ProgramPoint kind is very likely not necessary. I'd highly consider anything else before reaching out to that.
Adding a new special tag is a lot more lightweight approach. I'm just worried if the sink node already has a tag, then what should you do? This is why I wanted to avoid this approach.

Anyway, I'm happy to let you try something if you have more informed approaches. But that was my idea for what I would try with no other guidance.

I think you are on a good track. Don't hesitate to reach out again.