Page MenuHomePhabricator

[analyzer] Add path notes to FuchsiaHandleCheck
ClosedPublic

Authored by xazax.hun on Nov 26 2019, 9:22 AM.

Details

Summary

I also added a missing const to a BugReporter API.

There are some TODOs for potential future updates if we want richer notes during reporting.

Diff Detail

Event Timeline

xazax.hun created this revision.Nov 26 2019, 9:22 AM
NoQ added a comment.Dec 2 2019, 12:41 PM

Sooooo... note tags?

clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp
234

Will we ever emit a report against an escaped symbol? If not, there will never be a report to attach the note to.

235–236

If the function is implemented as an eager state split, having a note is great and easy to implement.

If it's implemented as a Schrödinger state split, then i think we should still emit a note at the collapse point (especially given that it may happen in a deeper stack frame than the call that produced the symbol). But this makes me recognize the need for note tags in evalAssume.

513

I wonder how terrible would it be to allow bug visitors (or note tags, which are just an "API sugar" for bug visitors) to mutate the uniqueing location. Because such information is naturally available in the visitor.

It most likely won't work because the information is necessary before the visitors are run. But it would have been nice though, so i feel as if we're missing an opportunity here.

I was thinking about note tags, but since we iterate on the argument/parameter pairs and might change the state in each iteration we would need to add a new transition after each change. I was wondering if using note tags would worth the cost (both in performance due to the additional number of nodes and in code complexity due to making transitions more often).

Sorry, my previous comment might not be clear. The point is, the same call might both acquire and release handles (there are such calls in Fuchsia), so we might end up adding more than one note for a call for which we would need to add more than one transitions. If you think it is still cleaner that way, I could do that as well :)

xazax.hun marked 3 inline comments as done.Dec 2 2019, 1:58 PM
xazax.hun added inline comments.
clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp
234

I think there might be cases where we go from an escaped symbol to released and so on. But you are right, it would not contribute to understanding the actual bug report.

235–236

I agree.

513

On one hand, it would be nice :) On the other hand we could have more than one visitor and it would be a hell to debug when the visitors disagree an the uniqueing location. If we can come up with an API that is not that easy to misuse, I am in favor of a change like that.

NoQ added a comment.Dec 2 2019, 8:04 PM

the same call might both acquire and release handles (there are such calls in Fuchsia), so we might end up adding more than one note for a call for which we would need to add more than one transitions

Hmm, would this be too functional?:

std::vector<std::function> notes;
for (arg : args) {
  if (isAcquired(param)) {
    State = State->set(arg, Acquired);
    notes.push_back([](Report) {
      if (Report.isInteresting(arg))
        return "Handle acquired here";
    });
  }
  if (isReleased(param)) {
    State = State->set(arg, Released);
    notes.push_back([](Report) {
      if (Report.isInteresting(arg))
        return "Handle released here";
    });
  }
}

C.addTransition(State, C.getNoteTag(
    // We might as well add a C.getNoteTag() overload
    // to do this automatically.
    [std::move(notes)](Report) {
      for (note : notes)
        note();
    }));

Or, well, yeah, chain the nodes together; you anyway have to do this occasionally due to clumsiness of the rest of the API.

NoQ added a comment.Dec 2 2019, 11:03 PM

note()

Of course i mean something like this:

std::string text = note();
if (!text.empty())
  return text;

(and return something like "" on the other return path in the inner lambdas).

xazax.hun updated this revision to Diff 231955.Dec 3 2019, 11:32 AM
  • Use the new notes API for path notes.

You were right, the new API looks cleaner even in this case :)

Just a small side note. If the state was same as the previous we do not end up creating a new ExplodedNode right? Is this also the case when we add a NoteTag?

xazax.hun marked an inline comment as done.Dec 3 2019, 11:38 AM
xazax.hun added inline comments.
clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp
358–374

Hmm, so we have C++14 now and I can do a move capture right? Will update in the next iteration.

Just a small side note. If the state was same as the previous we do not end up creating a new ExplodedNode right? Is this also the case when we add a NoteTag?

It only happens for evaluation when you cannot evaluate something. Other than that Pre/PostCall working fine to add a node with the NoteTag.

NoQ added a comment.Dec 3 2019, 12:35 PM

Yay, it actually works!

I only have minor comments and ignorable hand-waving now.

Just a small side note. If the state was same as the previous we do not end up creating a new ExplodedNode right? Is this also the case when we add a NoteTag?

It only happens for evaluation when you cannot evaluate something. Other than that Pre/PostCall working fine to add a node with the NoteTag.

Tag pointers are part of the ProgramPoint's identity, which in turn are part of the ExplodedNode's identity. If you make a new note tag and transition into it for the first time, the new node is guaranteed to be fresh.

clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp
210

N = N->getFirstPred().

219

return N;? (eliminates a variable)

223

N = N->getFirstPred().

359–362

Ugh, it sucks that we have to do this by hand.

Why would the symbol be interesting if the bugtype is wrong? You imagine something like division by a zero handle? Do we really mind the updates to the handle highlighted in this case? I guess we do.

Maybe we should make "note tag tags" to avoid users writing this by hand. Like, note tags are tagged with note tag tags and the report is also tagged with a note tag tag and the tag is invoked only if the tag tag on the report matches the tag tag on the tag. Setting a tag tag on the report will be equivalent to calling an "addVisitor" on the report in the visitor API, which was actually a good part of the visitor API.

Just thinking out loud, please ignore. This code is a nice example to generalize from.

365

Another option here is to concatenate the notes if there are indeed two interesting handles at once: `Handle is allocated through 2nd parameter; Handle is released through 3rd parameter". In this checker it probably never happens, as every report is about exactly one handle (is it true for leak reports though?).

xazax.hun updated this revision to Diff 231989.Dec 3 2019, 2:13 PM
xazax.hun marked 5 inline comments as done.
  • Address review comments.
clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp
359–362

I totally agree :) But if we do something like this please do not call the tag tag a tag. It will confuse people. :D

365

I think this is true for leaks as well, as we will generate a new report for each of the symbols.

xazax.hun added a comment.EditedDec 3 2019, 2:17 PM
In D70725#1767673, @NoQ wrote:

Just a small side note. If the state was same as the previous we do not end up creating a new ExplodedNode right? Is this also the case when we add a NoteTag?

It only happens for evaluation when you cannot evaluate something. Other than that Pre/PostCall working fine to add a node with the NoteTag.

Tag pointers are part of the ProgramPoint's identity, which in turn are part of the ExplodedNode's identity. If you make a new note tag and transition into it for the first time, the new node is guaranteed to be fresh.

This is what I suspected. I wonder if all the checks end up using NoteTags will the increased number of nodes ever be a concern? Maybe if the state is the same as the previous we should just not add the note tag?

For example, my checker before the note tag would only generate new nodes if there was an actual change to the state. After this change it probably ends up adding new nodes with empty note tags all the time. This could be bad both for performance and debugging. What do you think?

NoQ added a comment.EditedDec 3 2019, 2:29 PM

Mmm, right, i guess you should also skip adding the tag if the notes array is empty.

There might be more complicated use-cases (especially in ExprEngine itself) where you may end up adding a tag even if the state doesn't change. For instance, when changing an Environment value from absent to UnknownVal, the state doesn't get actually updated. Or making range assumptions over UnknownVal. But i'd argue that we do indeed want the note tag in such cases because it is still worth annotating the ExplodedGraph with an explanation of what's happening. Eg., a note "Assuming a condition is false" makes sense even if the condition is an UnknownVal and no actual constraints are added.

Another example: in a recent StreamChecker review we've been talking about a peculiar stream function that by design closes a file descriptor if it's open but does nothing if the descriptor is already closed. I believe this event deserves a note "The descriptor remains closed" (possibly prunable) even though there is no change in the state. This is something we couldn't do with our usual visitor-based approach which involves observing changes in the state (we technically could, via pattern-matching over the program point, but that'd directly duplicate a lot more of checker logic than usual).

xazax.hun updated this revision to Diff 231996.Dec 3 2019, 2:45 PM
  • Only add note tag if we have actual notes.
In D70725#1767942, @NoQ wrote:

Mmm, right, i guess you should also skip adding the tag if the notes array is empty.

There might be more complicated use-cases (especially in ExprEngine itself) where you may end up adding a tag even if the state doesn't change. For instance, when changing an Environment value from absent to UnknownVal, the state doesn't get actually updated. Or making range assumptions over UnknownVal. But i'd argue that we do indeed want the note tag in such cases because it is still worth annotating the ExplodedGraph with an explanation of what's happening. Eg., a note "Assuming a condition is false" makes sense even if the condition is an UnknownVal and no actual constraints are added.

Another example: in a recent StreamChecker review we've been talking about a peculiar stream function that by design closes a file descriptor if it's open but does nothing if the descriptor is already closed. I believe this event deserves a note "The descriptor remains closed" (possibly prunable) even though there is no change in the state. This is something we couldn't do with our usual visitor-based approach which involves observing changes in the state (we technically could, via pattern-matching over the program point, but that'd directly duplicate a lot more of checker logic than usual).

I see, that makes sense. I updated the code. I was hoping for a low hanging fruit to make this more user friendly :)

xazax.hun marked an inline comment as done.Dec 3 2019, 2:48 PM
xazax.hun added inline comments.
clang/lib/StaticAnalyzer/Checkers/FuchsiaHandleChecker.cpp
307

I will capitalize this name.

NoQ accepted this revision.Dec 9 2019, 7:36 PM

Looks great, thanks!~

This revision is now accepted and ready to land.Dec 9 2019, 7:36 PM

Thanks for the reviews!

This revision was automatically updated to reflect the committed changes.