Repository: google/go-flow-levee
Pull request #301 (2021-04-09 20:10:37)

Detect sources from inlined tuple call.
## Summary

When a call that returns multiple values, one of which is a source, is inlined into a sink call, levee currently does not detect the source at all, unless it is a pointer.

My proposed solution is to get rid of "unless it is a pointer".

## Example

```
func MaybeSource() (core.Source, error) {
	return core.Source{}, nil
}

func MaybeSourcePtr() (*core.Source, error) {
	return &core.Source{}, nil
}

func TestInlinedTupleCall() {
	core.Sink(MaybeSource())    // we want a report here, but levee isn't currently producing one
	core.Sink(MaybeSourcePtr()) // levee is producing a report here, which is what we want
}
```

The SSA is:

```
func TestInlinedTupleCall()
0: entry
	 0(*ssa.Call           ): t0 = MaybeSource()
	 1(*ssa.Extract        ): t1 = extract t0 #0
	 2(*ssa.Extract        ): t2 = extract t0 #1
	 3(*ssa.Alloc          ): t3 = new [2]interface{} (varargs)
	 4(*ssa.IndexAddr      ): t4 = &t3[0:int]
	 5(*ssa.MakeInterface  ): t5 = make interface{} <- levee_analysistest/example/core.Source (t1)
	 6(*ssa.Store          ): *t4 = t5
	 7(*ssa.IndexAddr      ): t6 = &t3[1:int]
	 8(*ssa.ChangeInterface): t7 = change interface interface{} <- error (t2)
	 9(*ssa.Store          ): *t6 = t7
	10(*ssa.Slice          ): t8 = slice t3[:]
	11(*ssa.Call           ): t9 = levee_analysistest/example/core.Sink(t8...)
	12(*ssa.Call           ): t10 = MaybeSourcePtr()
	13(*ssa.Extract        ): t11 = extract t10 #0
	14(*ssa.Extract        ): t12 = extract t10 #1
	15(*ssa.Alloc          ): t13 = new [2]interface{} (varargs)
	16(*ssa.IndexAddr      ): t14 = &t13[0:int]
	17(*ssa.MakeInterface  ): t15 = make interface{} <- *levee_analysistest/example/core.Source (t11)
	18(*ssa.Store          ): *t14 = t15
	19(*ssa.IndexAddr      ): t16 = &t13[1:int]
	20(*ssa.ChangeInterface): t17 = change interface interface{} <- error (t12)
	21(*ssa.Store          ): *t16 = t17
	22(*ssa.Slice          ): t18 = slice t13[:]
	23(*ssa.Call           ): t19 = levee_analysistest/example/core.Sink(t18...)
	24(*ssa.Return         ): return
```

Modulo variable names, the SSA is the same for both calls. The difference in behavior is due to source identification. In the `MaybeSourcePtr` case, the `Extract` is identified as a source because it has pointer type. Not so for `MaybeSource`.

- [x] Running against a large codebase such as [Kubernetes](https://github.com/kubernetes/kubernetes) does not error out. (See [DEVELOPING.md](https://github.com/google/go-flow-levee/blob/master/DEVELOPING.md) for instructions on how to do that.)
- (N/A) [ ] Appropriate changes to README are included in PR



Patch:
diff --git a/internal/pkg/source/source.go b/internal/pkg/source/source.go
index 9a70ce2..321ca9c 100644
--- a/internal/pkg/source/source.go
+++ b/internal/pkg/source/source.go
@@ -147,12 +147,13 @@ func isSourceNode(n ssa.Node, conf *config.Config, propagators fieldpropagator.R
 		return !v.CommaOk && sourcetype.IsSourceType(conf, taggedFields, v.AssertedType)
 
 	// An Extract is used to obtain a value from an instruction that returns multiple values.
-	// If the extracted value is a Pointer to a Source, it won't have an Alloc, so we need to
-	// identify the Source from the Extract.
+	// In some cases, an extracted value isn't tied to any other instruction that could be used
+	// to detect a source value, so the Extract itself has to be used. Specifically:
+	// - If the extracted value is a Pointer to a Source
+	// - If the extracted value is inlined into a call
 	case *ssa.Extract:
 		t := v.Tuple.Type().(*types.Tuple).At(v.Index).Type()
-		_, ok := t.(*types.Pointer)
-		return ok && sourcetype.IsSourceType(conf, taggedFields, t)
+		return sourcetype.IsSourceType(conf, taggedFields, t)
 
 	// Unary operator <- can receive sources from a channel.
 	case *ssa.UnOp:


Test patch:
diff --git a/internal/pkg/levee/testdata/src/levee_analysistest/example/tests/inlining/tests.go b/internal/pkg/levee/testdata/src/levee_analysistest/example/tests/inlining/tests.go
index 3135c05..78d165a 100644
--- a/internal/pkg/levee/testdata/src/levee_analysistest/example/tests/inlining/tests.go
+++ b/internal/pkg/levee/testdata/src/levee_analysistest/example/tests/inlining/tests.go
@@ -22,10 +22,23 @@ func NewSource() *core.Source {
 	return &core.Source{}
 }
 
+func MaybeSource() (core.Source, error) {
+	return core.Source{}, nil
+}
+
+func MaybeSourcePtr() (*core.Source, error) {
+	return &core.Source{}, nil
+}
+
 func TestInlinedCall() {
 	core.Sink(NewSource()) // want "a source has reached a sink"
 }
 
+func TestInlinedTupleCall() {
+	core.Sink(MaybeSource())    // want "a source has reached a sink"
+	core.Sink(MaybeSourcePtr()) // want "a source has reached a sink"
+}
+
 func TestInlinedRecv(sources <-chan core.Source) {
 	core.Sink(<-sources) // want "a source has reached a sink"
 }
diff --git a/internal/pkg/source/testdata/src/analyzertest/sourcetest/identification.go b/internal/pkg/source/testdata/src/analyzertest/sourcetest/identification.go
index 61dc95e..827cde4 100644
--- a/internal/pkg/source/testdata/src/analyzertest/sourcetest/identification.go
+++ b/internal/pkg/source/testdata/src/analyzertest/sourcetest/identification.go
@@ -88,15 +88,24 @@ func TestNamedReturnValues() (val Source, ptr *Source) { // want "source identif
 }
 
 func TestSourceExtracts() {
-	s, err := CreateSource() // want "source identified"
+	// We expect two reports for this case, because creating s
+	// will require an Extract and an Alloc.
+	s, err := CreateSource() // want "source identified" "source identified"
 	sptr, err := NewSource() // want "source identified"
 
-	// we expect two reports for the following cases, since the map is a Source
-	mapSource, ok := map[string]Source{}[""]     // want "source identified" "source identified"
+	// We expect three reports for this case, because:
+	// 1. the map is a Source
+	// 2. there is an Extract for the mapSource value
+	// 3. there is an Alloc for the mapSource value
+	mapSource, ok := map[string]Source{}[""] // want "source identified" "source identified" "source identified"
+
+	// We expect two reports here, for the map and the Extract.
+	// (There won't be an Alloc because mapSourcePtr is a pointer.)
 	mapSourcePtr, ok := map[string]*Source{}[""] // want "source identified" "source identified"
 
-	// we expect two reports for the following cases, since the chan is a Source
-	chanSource, ok := <-(make(chan Source))     // want "source identified" "source identified"
+	// These two cases are similar to the map cases above.
+	// The reasoning behind the number of expected reports is the same.
+	chanSource, ok := <-(make(chan Source))     // want "source identified" "source identified" "source identified"
 	chanSourcePtr, ok := <-(make(chan *Source)) // want "source identified" "source identified"
 
 	_, _, _, _, _, _, _, _ = s, sptr, mapSource, chanSource, mapSourcePtr, chanSourcePtr, err, ok