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.

Sign up for GitHub

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

Jump to bottom

Fix: Only traverse to reference args #185

Merged
merged 23 commits into from
Dec 4, 2020
Merged

Fix: Only traverse to reference args #185

merged 23 commits into from
Dec 4, 2020

Conversation

mlevesquedion
Copy link
Contributor

@mlevesquedion mlevesquedion commented Nov 12, 2020 •
edited
Loading

This PR modifies the propagation for Calls so that only reference arguments (as determined by pointer.CanPoint) are tainted. It makes no sense to taint non-reference arguments, because the called function will receive a copy of them, so the original values in the caller can't be tainted.

New tests are added to verify this behavior.

  • Tests pass
  • Running against a large codebase such as Kubernetes does not error out.
  • (N/A) [ ] Appropriate changes to README are included in PR

@mlevesquedion mlevesquedion marked this pull request as draft November 12, 2020 14:59
@mlevesquedion mlevesquedion force-pushed the only-traverse-to-reference-args branch 2 times, most recently from 0cb7942 to 3a67a23 Compare November 17, 2020 17:11
@mlevesquedion mlevesquedion force-pushed the only-traverse-to-reference-args branch 3 times, most recently from 49b7f71 to 7ad0085 Compare November 17, 2020 17:33
mlevesquedion
mlevesquedion commented Nov 17, 2020
View reviewed changes
internal/pkg/levee/testdata/src/example.com/tests/collections/maps.go
@@ -53,7 +53,9 @@ func TestMapRemainsTaintedWhenSourceIsDeleted(s core.Source) {
core.Sink(m) // want "a source has reached a sink"
}

func TestDeletingFromTaintedMapDoesNotTaintTheKey(key string, sources map[string]core.Source) {
func TestDeletingFromTaintedMapDoesNotTaintKey(key *string, sources map[*string]core.Source) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The modifications in this PR caused this test to pass spuriously. The key now needs to be some kind of pointer in order to demonstrate the incorrect behavior.

@mlevesquedion mlevesquedion force-pushed the only-traverse-to-reference-args branch from 7ad0085 to d179003 Compare November 17, 2020 17:41
@mlevesquedion mlevesquedion marked this pull request as ready for review November 17, 2020 17:43
PurelyApplied
PurelyApplied requested changes Nov 18, 2020
View reviewed changes
Copy link
Collaborator

@PurelyApplied PurelyApplied left a comment

Choose a reason for hiding this comment

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

This PR feels like it's doing a bit too much.

  • Like reflect, I don't think we have or should make any promise about tracking taint across use of unsafe. At best, that feels out of scope for this PR.
  • For what is pointer-like, I think pointer should be authoritative. Our use case includes also non-pointer wrappers of pointer-like types, so we have two extra cases to check. That delineation could be more clear.

internal/pkg/source/source.go Outdated Show resolved Hide resolved
internal/pkg/source/source.go Outdated Show resolved Hide resolved
internal/pkg/levee/testdata/src/example.com/tests/colocation/tests.go Outdated Show resolved Hide resolved
internal/pkg/source/source.go Show resolved Hide resolved
@mlevesquedion
Copy link
Contributor Author

Thank you for your review!

Like reflect, I don't think we have or should make any promise about tracking taint across use of unsafe.

For reflect.Value, we get that behavior via pointer.CanPoint. I think tracking taint through reflect.Value is legitimate, since those can be freely converted to and from interface{} values. Whether we want to model reflection is a different question and I think we agree that the answer is currently "no". My reasoning was similar in the case of unsafe.Pointer, I thought since those can be converted to and from pointers, then they should be considered pointers as well, but I think the reality is a bit more subtle, so I agree that this doesn't need to be in this PR.

This was referenced Nov 18, 2020
Merged
Merged
@mlevesquedion mlevesquedion mentioned this pull request Dec 3, 2020
Merged
1 task
PurelyApplied
PurelyApplied approved these changes Dec 3, 2020
View reviewed changes
Copy link
Collaborator

@PurelyApplied PurelyApplied 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! Only two minor question/comment items.

@mlevesquedion mlevesquedion merged commit 9650d8f into google:master Dec 4, 2020
@mlevesquedion mlevesquedion deleted the only-traverse-to-reference-args branch December 4, 2020 17:48
@mlevesquedion mlevesquedion changed the title Only traverse to reference args Fix: Only traverse to reference args Feb 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PurelyApplied PurelyApplied PurelyApplied approved these changes

@vinayakankugoyal vinayakankugoyal Awaiting requested review from vinayakankugoyal

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mlevesquedion @PurelyApplied