Any plans to rework NoStoreFuncVisitor as well?

Hi!

Just a quick one – I want to generalize NoStoreFuncVisitor to be able to construct messages such as “Returning without deallocating or changing the ownership of allocated memory”. The grand idea is to create a NoStateChangeFuncVisitor base class that can be specialized for what a (lack of a) state change is.

Are there any similar ongoing efforts or shall I proceed?

Cheers,
Kristóf

Hi Kristóf,

In my opinion, this is the checker’s logic and it shouldn’t live inside of `NoStoreFuncVisitor`. However, I think that `NoStateChangeFuncVisitor` can require an extra step for the users.

During my big discussion with Artem in the patchset, where I introduced the `Tracker` interface, I suggested to extract different bits of what I called “events” from different parts of `trackExpressionValue`, so that checkers can react on those events. At first, we make it part of the tracker mechanism, so we detect them during this stage, and later we can put special tags on the nodes.

Long story short, I think that `NoStore` is event and with that it is similar to `Store`. We can introduce `NoStoreHandler` (probably we can come up with a better name than that), that produces the note, just like `StoreHandler` does. And the user can plug in their own handler to produce customized note.

What do you think about this?

Valeriy

I think that sounds great, and I have thought about adding this later down the line to your tracking interface. The issue with the specific, and rather poor memory leak report I’m investigating is that its not a (lack of a) value change to a region that I want to track down, but rather a change of some arbitrary property on a symbol. Take for instance the following code snippet:

void sink(int *P) {} // no notes

void f() {
sink(new int(5)); // note: Memory is allocated
// Well hold on, sink() was supposed to deal with
// that, this must be a false positive…
} // warning: Potential memory leak [cplusplus.NewDeleteLeaks]

The fact that the allocated memory leaks is the property of the symbol being dead (SymbolReaper::isDead) despite still being marked as allocated after the call to sink(). The fact that sink() didn’t help to prevent this error is a property of:

  • No region with a lifetime longer then the call to f holds the value of this symbol
  • The RefState hasn’t changed (from Kind::Allocated or Kind::AllocatedOfSizeZero)

…and to me, it seems like both of these should be checked at bugreporting, not analysis time.

As these properties could only be checked during tracking, I think a handler the way you describe it (as I understand) would be insufficient. While this really is a MallocChecker-specific problem, I think generalizing NoStoreFuncVisitor to have an overridable “did anything change about this entity during this function call?” would be beneficial for many other similar checkers. That might as well end up as an extension to the existing StoreHandler interface.

Yeah, in No${Something}FuncVisitor the definition of ${Something} is entirely checker-specific. This means that generality is required not only with respect to the text of the note, but also with respect to defining the nature of ${Something}.

The flimsiest part of No${Something}FuncVisitor is identifying that the function was supposed to do ${Something} it didn’t do. We typically boil this down to “there exists another execution path on which it did actually do ${Something}” but we can’t check that either because that execution path isn’t necessarily explored. So we dumb it down even further to see if there are syntactic constructs in that function that would have accomplished ${Something} (such as assignment to the given pointee in case of ${Something}=Store) - which is not only very imprecise without flow sensitivity but also fails when inter-procedural-ness is involved (i.e., the function was supposed to accomplish ${Something} by calling another function that actually does ${Something}).

I think one TODO here is that we could still take advantage of path exploration by having it signal us when it found a delete on a different path. It would help us with inter-procedural-ness and possibly with some flow-sensitivity as well.

Kristof’s case is interesting in a different manner. If taken literally, it doesn’t satisfy our criteria of “there exists another execution path on which it did actually do ${Something}”. We probably shouldn’t emit a note every time the function simply accepts the value of interest by pointer, because this doesn’t necessarily prove the intent to delete the pointer; there are a million other reasons to accept a pointer. However, Kristof’s case does still deserve a note because sink() is the only function that had a chance to delete the pointer.

A similar situation can be encountered with uninitialized variables:

void bar() {
}

void foo() {
int x;
bar(&x); // Display path in bar() because no other function could initialize ‘x’
return x; // Warning: Use of uninitialized variable
}

So I guess the question is:

  • We clearly need the text of the note to be configurable by the checker
  • We clearly need the definition of ${Something} to be configurable by the checker
  • But do we need the inner workings of this whole auxiliary analysis to be configurable by the checker?

Or can we get away with simply supplying a definition of ${Something} as, say, an ASTMatcher with which the auxiliary analysis would scan the function? Because, damn, such analysis can get very advanced and complicated, so in any case we’d better provide a sane default and not duplicate this analysis in every checker.