[PATCH] D144269: [Analyzer] Show "taint originated here" note of alpha.security.taint.TaintPropagation checker at the correct place

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added a comment.

In D144269#4143066 <https://reviews.llvm.org/D144269#4143066>, @NoQ wrote:

> The challenging part with note tags is how do you figure out whether your bug report is taint-related. The traditional solution is to check the `BugType` but in this case an indeterminate amount of checkers may emit taint-related reports.

Yeah, this is why we created a new type. Not sure what is the better infrastructure design, whether to create a subtype of `BugType` or `BugReport`, but it fundamentally achieves the same thing.

In D144269#4146809 <https://reviews.llvm.org/D144269#4146809>, @dkrupp wrote:

> My fear with the interestingness is that it may propagating backwards according to different "rules" than whot the taintedness is popagating in the foward direction even with the "extensions" pointed out by steakhal.
> So the two types of propagation may be or can get out of sync.
>
> So if the above is not a concern and you think implementing this with interestingness is more elegant, idiomatic and causes less maintenance burden, I am happy to create an alternative patch with that solution.

@dkrupp and I discussed in detail whether to use FlowID's (what is currently implemented in the patch) or something similar, or reuse interestingness. Here's why we decided against reusing interestiness as is.

Interestingness, as it stands now, mostly expresses data-dependency, and is propageted with using the analyzers usualy somewhat conservative approach. While the length of a string is strictly speaking data dependent on the actual string, I don't think analyzer currently understand that. We approach taint very differently, and propagete it in some sense more liberally.

As I best recall, however, interestingness may be propagated through other means as well. If we reused interestingness, I fear that the interestiness set could actually be greater than the actual //interesting tainted// set, causing more notes to be emitted than needed.

For these reasons, which I admit are a result of some speculation, we concluded that interstingness as it is and taint are two different properties that are best separated.

In D144269#4143066 <https://reviews.llvm.org/D144269#4143066>, @NoQ wrote:

> I think now's a good time to include a "generic data map"-like data structure in `PathSensitiveBugReport` objects, so that checkers could put some data there during `emitReport()`, which can be picked up by note tags and potentially mutated in the process.

Maybe a new interestingness kind (D65723 <https://reviews.llvm.org/D65723>)? Not sure how this design aged, but we don't really need to store an ID for this, so a simple interestingness flag (just not the default `Thorough` interestiness) is good enough.

In D144269#4146809 <https://reviews.llvm.org/D144269#4146809>, @dkrupp wrote:

> The tricky part was is to how to show only that single "Taint originated here" note tag at the taint source only which is relevant to the report. This is done by remembering the unique flowid in the
> NoteTag in the forward analysis direction (see GenericTaintChecker:cpp:859) `InjectionTag = createTaintOriginTag(C, TaintFlowId);` and then filtering out the irrelevant 
> NoteTags when the the report is generated (when the lamdba callback is called). See that flowid which reaches the sink is backpropagated in the PathSensitiveBugreport (see GenericTaintCHekcer.cpp:167).
>
> FlowIds are unique and increased at every taint source (GenericTaintChecker:869) and it is stored as an additional simple `int` in the program state along with the already existing (Taint.cpp:22) TaintTagType.

If you propagate this property during analysis, those IDs may be needed, but a simple flag should suffice when BugReporter does it.


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

https://reviews.llvm.org/D144269