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

Add support for flow through content of global variables #16667

Merged
merged 3 commits into from
Jun 18, 2024

Conversation

smowton
Copy link
Contributor

@smowton smowton commented Jun 4, 2024

Note flow via something like sideEffect(&x) is still broken. I think this is because we're lacking a store-step for *x = source(), which should result in x having taint with access path [PointerContent]. This might also involve address operations being able to read taint (i.e. to note that if &x is tainted with [PointerContent] then x is tainted without an access path), but I'm not sure -- in any event I don't address the matter here. I'd suggest tackling that issue by cribbing off C/C++, which has surely addressed (no pun intended) this exact issue.

@smowton smowton requested a review from a team as a code owner June 4, 2024 11:14
@github-actions github-actions bot added the Go label Jun 4, 2024
Copy link
Contributor

@owen-mc owen-mc 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. I think this needs a QA run. Also, please add failing tests for the cases you mentioned in the PR description, with a brief comment explaining why they fail.

@smowton
Copy link
Contributor Author

smowton commented Jun 5, 2024

The failing case already exists-- it's the one that still has the MISSING: tag where the others are removed.

@smowton
Copy link
Contributor Author

smowton commented Jun 5, 2024

(Will DCA then QA)

@smowton
Copy link
Contributor Author

smowton commented Jun 6, 2024

DCA: one new result (TP, stack trace exposure), no performance effects. Proceeding to QA.

@smowton smowton force-pushed the smowton/fix/global-variable-side-effect branch from 8527054 to b440ae2 Compare June 6, 2024 10:10
@smowton
Copy link
Contributor Author

smowton commented Jun 17, 2024

There are too many new results to sensibly check them all, but I checked a sample result from each of the 5 repositories with the most new results, and found they were all doing as intended. Some appeared to be TPs, and some FPs for unrelated reasons, e.g. taint-tracking losing sight of the fact that only one field of a struct carried tainted data, that don't reflect on the change made here.

Performance results were unremarkable. Therefore I recommend merging this.

@smowton smowton force-pushed the smowton/fix/global-variable-side-effect branch from b440ae2 to 4da5d66 Compare June 17, 2024 15:49
@smowton
Copy link
Contributor Author

smowton commented Jun 17, 2024

@owen-mc I've rebased to re-test and added test expectation changes and a change note. Please review and merge if you're happy.

@owen-mc owen-mc merged commit 9403bf2 into github:main Jun 18, 2024
14 checks passed
@owen-mc
Copy link
Contributor

owen-mc commented Jun 18, 2024

I've made an issue to follow up the missing pointer content store step.

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.

None yet

2 participants