Page MenuHomePhabricator

Added remove taint support to ProgramState.
Needs ReviewPublic

Authored by franchiotta on Jul 31 2015, 5:25 PM.
This revision needs review, but there are no reviewers specified.

Details

Reviewers
None
Summary

Currently there is support for just adding taint on ProgramState, this would allow to mark symbols as non-tainted.

Diff Detail

Event Timeline

franchiotta retitled this revision from to Added remove taint support to ProgramState..
franchiotta updated this object.
franchiotta added a subscriber: cfe-commits.

I don't remember why the 'tainted' methods were added to ProgramState in the first place, but it doesn't seem quite right. Taint can easily be modeled as a set of APIs that modify and produce new ProgramStates, but I don't see why it should be part of ProgramState itself.

In general I'm not a fan of the taint methods being directly on ProgramState, but this extends the current pattern. We could move the APIs elsewhere later.

include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h
371–372

The parameters from the two lines should line up.

lib/StaticAnalyzer/Core/ProgramState.cpp
679–713

This seems reasonable enough. It mostly mirrors the other taint operations we have, and there is noticeably a fair amount of copy-paste here that I wonder could be better factored. removeTaint looks pretty much identical to addTaint except that it uses the remove function on the TaintMap. I'm particularly concerned about the copy-paste for the first removeTaint, as that is a non-trivial amount of logic. Could this case possibly be refactored a bit with addTaint?

I don't remember why the 'tainted' methods were added to ProgramState in the first place, but it doesn't seem quite right. Taint can easily be modeled as a set of APIs that modify and produce new ProgramStates, but I don't see why it should be part of ProgramState itself.

Yes, I agree with you Ted. We can leave as it is just for now, and then I can work on that patch. May I assign you as subscriber when submitting it?

include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h
371–372

Alright

lib/StaticAnalyzer/Core/ProgramState.cpp
679–713

Yes, sure! Let me upload a new patch for this.

franchiotta updated this revision to Diff 31289.Aug 3 2015, 6:01 PM

Basically, I moved the logic of AddTaint out of the method, and placed it on a generic method which allows to add or remove over the map based on a parameter value. All this taint logic will be placed on a external API in a future.

Updated refactoring.

krememek added inline comments.Aug 24 2015, 9:36 PM
include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h
451–455

Can we add documentation comments for these? Seems like generally useful utility methods. We could also probably just make these public.

lib/StaticAnalyzer/Core/ProgramState.cpp
650–651

Is this even needed? I think Environment::getSVal() already handles parenthesis and other ignored expressions. This looks like dead code.

This can probably just be an inline method in ProgramState.h, that just forwards to getSVal(S, LCtx).getAsSymbol().

Alternatively, if this is only called once, we don't need to add a method at all, since it is just a one liner.

657–658

This looks fishy. 'addTaint' returns a new state, but then the return value is ignored, and 'this' is returned.

657–660

This is just a one-liner. Do we really need this method at all? It is only called once.

685–689

This looks fishy. 'removeTaint' returns a new state, but then the return value is ignored. 'ProgramState' values are immutable, so this method appears to do nothing.

krememek added inline comments.Aug 24 2015, 9:37 PM
include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h
451–455

Actually, I'm wondering if we really need to add these at all. They are just one liners that easily could be written where they are used.

franchiotta added inline comments.Oct 3 2015, 7:39 AM
include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h
451–455

Right. Removing these methods, and adding the one-liners directly where they are used.

lib/StaticAnalyzer/Core/ProgramState.cpp
650–651

Yes, you are right. It is not needed since it is handle by ignoreTransparentExprs method in Environment module. I will not add this method at all.

657–658

Yes, it does.. I will return at the time the last addTaint is invoked.

657–660

We don't. I will add the one-liner directly where it is used.

685–689

Yes, you are right. I will return at the time the last addTaint is invoked.

franchiotta updated this revision to Diff 36431.Oct 3 2015, 8:07 AM
franchiotta updated this object.

Some changes made based on the received suggestions.

NoQ added a subscriber: NoQ.Dec 7 2015, 8:37 AM
MTC added a subscriber: MTC.Oct 19 2017, 1:28 AM