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

Reassign structlit test todos, add tests #258

Merged
merged 1 commit into from
Jan 13, 2021

Conversation

mlevesquedion
Copy link
Contributor

@mlevesquedion mlevesquedion commented Jan 13, 2021

Consider this piece of SSA graph:

image

The corresponding code is:

func TestStructHoldingSourceAndInnocIsTainted(s core.Source, i core.Innocuous) {
	h := Holder{
		s,
		i,
	}
	core.Sink(h) // TODO want "a source has reached a sink"
}

Currently, the assigned issue is #97. While I think fixing #97 may solve this case, when I originally wrote these tests the intent was to capture the fact that if you put a source value into a locally allocated struct, that struct should be tainted. Now that I'm taking a fresh look at this, I believe the test case I wanted to write was:

type InterfaceHolder struct {
	i interface{}
}

func TestStructLiteralContainingTaintedInterfaceIsTainted(s core.Source) {
	ih := InterfaceHolder{
		s,
	}
	core.Sink(ih) // TODO(#212) want "a source has reached a sink"
}

The SSA graph for this one is basically the same, with an extra MakeInterface step (and without the path involving the core.Innocuous value).

In all of these cases (but this is especially clear in the cases involving interface{} fields), propagation doesn't make it through the Alloc, so taint doesn't reach the sink. To me, this seems like it's a closer fit for #212.

  • (N/A) [ ] Running against a large codebase such as Kubernetes does not error out. (See DEVELOPING.md for instructions on how to do that.)
  • (N/A) [ ] Appropriate changes to README are included in PR

@mlevesquedion mlevesquedion merged commit e685de7 into google:master Jan 13, 2021
@mlevesquedion mlevesquedion deleted the reassign-todos branch January 13, 2021 18:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants