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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
