Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Go: show FunctionModel steps in path summaries #13461

Merged

Conversation

owen-mc
Copy link
Contributor

@owen-mc owen-mc commented Jun 14, 2023

It was highlighted in #13415 that we don't put steps from functions modeled using DataFlow::FunctionModel or TaintTracking::FunctionModel in path summaries, which can be confusing. This PR does that, showing both inputs and outputs. Here are screenshots showing the option of just inputs as well.

Inputs only:
input-only

Both inputs and outputs:
both

Note: as discussed in #13415, step 3 in the second picture goes back to the definition of the variable being tainted, in a way which is quite counter-intuitive. This is a consequence of our use of def-use flow.

The downside of doing both is that any path which goes through the definition of sample (step 3 in the second picture) will have it in the path explanation, even if the path doesn't go through the function that has been modeled. This should be fixed by #13473.

@owen-mc
Copy link
Contributor Author

owen-mc commented Jun 15, 2023

Note: this is even better when combined with #13473. In that case we'd want to show the input and output nodes.

@owen-mc owen-mc force-pushed the go/show-functionmodel-steps-to-path-summaries branch from d2daab9 to e05ce1b Compare June 16, 2023 15:13
@owen-mc
Copy link
Contributor Author

owen-mc commented Jun 16, 2023

I've dropped the commit which made it only do inputs. I think inputs and outputs is definitely better given that #13473 should make the outputs more sensible.

None of the test changes include changes to the select results, just nodes and edges, as you'd expect. I checked through 3 or 4 of them in detail and they were all good.

@owen-mc owen-mc force-pushed the go/show-functionmodel-steps-to-path-summaries branch from e05ce1b to 387f523 Compare June 16, 2023 15:55
@owen-mc owen-mc force-pushed the go/show-functionmodel-steps-to-path-summaries branch from 387f523 to c0fea85 Compare June 20, 2023 12:33
@owen-mc owen-mc marked this pull request as ready for review June 20, 2023 12:34
@owen-mc owen-mc requested a review from a team as a code owner June 20, 2023 12:34
Copy link
Member

@mbg mbg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, subject to DCA results.

@owen-mc
Copy link
Contributor Author

owen-mc commented Jun 22, 2023

Nothing showed up in the performance evaluation so I'm merging.

@owen-mc owen-mc merged commit b3a19ef into github:main Jun 22, 2023
@owen-mc owen-mc deleted the go/show-functionmodel-steps-to-path-summaries branch June 22, 2023 09:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants