[PATCH] D154603: [analyzer][clangsa] Add new option to alpha.security.cert.InvalidPtrChecker

[Balázs Benics via Phabricator via cfe-commits]

steakhal added a comment.

In D154603#4609872 <https://reviews.llvm.org/D154603#4609872>, @gamesh411 wrote:

> - try to consolidate the multiple warnings coming from this checker's `checkLocation` callback
>
> category based filtering ( example from lib/StaticAnalyzer/Checkers/GenericTaintChecker.cpp:167 ):
>
>   If (!BR.isInteresting(CallLocation) ||
>     BR.getBugType().getCategory() != categories::TaintedData) { //but this would be InvalidPtr BugType's category, namely memory_error
>     return "";
>   }
>
> or checker based filtering ( example from lib/StaticAnalyzer/Checkers/SmartPtrModeling.cpp:397 )
>
>   if (&BR.getBugType() != smartptr::getNullDereferenceBugType() || // this is a comparison of the address of a static bugtype
>       !BR.isInteresting(ThisRegion))
>
> This second one gives a more precise filtering, but the implementation-specific detail of storing the bugtype by reference is what seems to make this work, which I find hacky.

If the checker issues a NoteTag, it makes sense in certain situations to make sure that it acts on only reports issued by that checker. The standard way of achieving that is by comparing the tags, as you do in the second example.
There is nothing wrong with this, if that particular checker issued that NoteTag in the first place. It's marginally debatable, if it was not issued by the given checker but rather something else. That would suggest to me some logic flaw, or coupling issue. For cross-checker cases, I think the bug category would be the better option, but I would still need to think about that, so not set in stone :D

FYI I haven't looked at the patch yet, but I wanted to answer your question.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154603