Dataflow: Flow-state changing steps should always be in path explanations #8381
Conversation
MathiasVP
force-pushed
the
flow-state-changing-steps-should-be-in-path-explanation
branch
from
cf76254 to
cd1f266
Compare
|
This PR took unbelievably many DCA attempts to run successfully. Here are the final DCA runs:
(They all look fine.) |
| @@ -1972,6 +1988,13 @@ private module Stage3 { | |||
| ap0 instanceof ApNil | |||
| ) | |||
| or | |||
| exists(NodeEx mid, FlowState state0, ApNil nil | | |||
There was a problem hiding this comment.
With this change, the code for stages 2-4 are no longer in sync. The local call context checks done by localFlowBigStep are also omitted.
There was a problem hiding this comment.
With this change, the code for stages 2-4 are no longer in sync.
Oops. That's true. I thought I had synced them up, but apparently, I hadn't 🤦.
The local call context checks done by localFlowBigStep are also omitted.
Ah, that's a good point. I guess this is what you fixed with this change in your alternative PR, right?
I must admit I don't fully understand how the dataflow library implements call contexts yet, so I'm not surprised I didn't handle this correctly 😄. Thanks for catching it!
There was a problem hiding this comment.
Ah, that's a good point. I guess this is what you fixed with this change in your alternative PR, right?
Yes.
| @@ -14,6 +14,8 @@ | |||
| | p;InnerHolder;false;getValue;();;Argument[-1];ReturnValue;taint | | |||
| | p;InnerHolder;false;setContext;(String);;Argument[0];Argument[-1];taint | | |||
| | p;Joiner;false;Joiner;(CharSequence);;Argument[0];Argument[-1];taint | | |||
| | p;Joiner;false;Joiner;(CharSequence);;Argument[1];Argument[-1];taint | | |||
There was a problem hiding this comment.
This can't be right; Argument[1] does not exist for a constructor with just one argument.
There was a problem hiding this comment.
Hm, yes. Good point. Since you've fixed this in #8474 I won't dive into why that happened (but I'm still kinda curious 🤔).
Thanks a lot for fixing this! In that case, I'll close this PR 👍. |
A taint step that changes the flow-state is almost always going to be interesting for users. So it shouldn't be stepped over by the big step relation.