Repository: google/go-flow-levee
Pull request #23 (2020-06-02 00:04:10)

Extract varargs' related logic into a separate package.
Extract varargs' related logic into a separate package.


Patch:
diff --git a/internal/levee.go b/internal/levee.go
index b665bbd..73642c3 100644
--- a/internal/levee.go
+++ b/internal/levee.go
@@ -23,6 +23,7 @@ import (
 	"golang.org/x/tools/go/ssa"
 
 	"github.com/google/go-flow-levee/internal/pkg/source"
+	"github.com/google/go-flow-levee/internal/pkg/varargs"
 )
 
 var Analyzer = &analysis.Analyzer{
@@ -33,74 +34,6 @@ var Analyzer = &analysis.Analyzer{
 	Requires: []*analysis.Analyzer{source.Analyzer},
 }
 
-// varargs represents a variable length argument.
-// Concretely, it abstract over the fact that the varargs internally are represented by an ssa.Slice
-// which contains the underlying values of for vararg members.
-// Since many sink functions (ex. log.Info, fmt.Errorf) take a vararg argument, being able to
-// get the underlying values of the vararg members is important for this analyzer.
-type varargs struct {
-	slice   *ssa.Slice
-	sources []*source.Source
-}
-
-// newVarargs constructs varargs. SSA represents varargs as an ssa.Slice.
-func newVarargs(s *ssa.Slice, sources []*source.Source) *varargs {
-	a, ok := s.X.(*ssa.Alloc)
-	if !ok || a.Comment != "varargs" {
-		return nil
-	}
-	var (
-		referredSources []*source.Source
-	)
-
-	for _, r := range *a.Referrers() {
-		idx, ok := r.(*ssa.IndexAddr)
-		if !ok {
-			continue
-		}
-
-		if idx.Referrers() != nil && len(*idx.Referrers()) != 1 {
-			continue
-		}
-
-		// IndexAddr and Store instructions are inherently linked together.
-		// IndexAddr returns an address of an element within a Slice, which is followed by
-		// a Store instructions to place a value into the address provided by IndexAddr.
-		store := (*idx.Referrers())[0].(*ssa.Store)
-
-		for _, s := range sources {
-			if s.HasPathTo(store.Val.(ssa.Node)) {
-				referredSources = append(referredSources, s)
-				break
-			}
-		}
-	}
-
-	return &varargs{
-		slice:   s,
-		sources: referredSources,
-	}
-}
-
-func (v *varargs) referredByCallWithPattern(patterns []config.NameMatcher) *ssa.Call {
-	if v.slice.Referrers() == nil || len(*v.slice.Referrers()) != 1 {
-		return nil
-	}
-
-	c, ok := (*v.slice.Referrers())[0].(*ssa.Call)
-	if !ok || c.Call.StaticCallee() == nil {
-		return nil
-	}
-
-	for _, p := range patterns {
-		if p.MatchMethodName(c) {
-			return c
-		}
-	}
-
-	return nil
-}
-
 func run(pass *analysis.Pass) (interface{}, error) {
 	conf, err := config.ReadConfig()
 	if err != nil {
@@ -143,15 +76,11 @@ func run(pass *analysis.Pass) (interface{}, error) {
 
 					case conf.IsSink(v):
 						// TODO Only variadic sink arguments are currently detected.
-						if v.Call.Signature().Variadic() && len(v.Call.Args) > 0 {
-							lastArg := v.Call.Args[len(v.Call.Args)-1]
-							if varargs, ok := lastArg.(*ssa.Slice); ok {
-								if sinkVarargs := newVarargs(varargs, sources); sinkVarargs != nil {
-									for _, s := range sinkVarargs.sources {
-										if !s.IsSanitizedAt(v) {
-											report(pass, s, v)
-										}
-									}
+						if sinkVarargs := varargs.New(v); sinkVarargs != nil {
+							for _, s := range sources {
+								if sinkVarargs.ReferredBy(s) && !s.IsSanitizedAt(v) {
+									report(pass, s, v)
+									break
 								}
 							}
 						}
diff --git a/internal/pkg/varargs/varargs.go b/internal/pkg/varargs/varargs.go
new file mode 100644
index 0000000..9f8da4b
--- /dev/null
+++ b/internal/pkg/varargs/varargs.go
@@ -0,0 +1,83 @@
+// Copyright 2020 Google LLC
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// https://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+// Package varargs contains functionality for dealing with variable length arguments.
+package varargs
+
+import (
+	"golang.org/x/tools/go/ssa"
+)
+
+// Varargs represents a variable length argument.
+// Concretely, it abstract over the fact that the varargs internally are represented by an ssa.Slice
+// which contains the underlying values of for vararg members.
+// Since many sink functions (ex. log.Info, fmt.Errorf) take a vararg argument, being able to
+// get the underlying values of the vararg members is important for the analysis.
+type Varargs struct {
+	stores []*ssa.Store
+}
+
+// New constructs varargs. SSA represents varargs as an ssa.Slice.
+func New(s *ssa.Call) *Varargs {
+	if !s.Call.Signature().Variadic() {
+		return nil
+	}
+	if len(s.Call.Args) == 0 {
+		return nil
+	}
+
+	lastArg := s.Call.Args[len(s.Call.Args)-1]
+	sl, ok := lastArg.(*ssa.Slice)
+	if !ok {
+		return nil
+	}
+
+	a, ok := sl.X.(*ssa.Alloc)
+	if !ok || a.Comment != "varargs" {
+		return nil
+	}
+
+	var stores []*ssa.Store
+
+	for _, r := range *a.Referrers() {
+		idx, ok := r.(*ssa.IndexAddr)
+		if !ok || idx.Referrers() == nil {
+			continue
+		}
+
+		// IndexAddr and Store instructions are inherently linked together.
+		// IndexAddr returns an address of an element within a Slice, which is followed by
+		// a Store instructions to place a value into the address provided by IndexAddr.
+		stores = append(stores, (*idx.Referrers())[0].(*ssa.Store))
+	}
+
+	return &Varargs{
+		stores: stores,
+	}
+}
+
+type referrer interface {
+	HasPathTo(node ssa.Node) bool
+}
+
+// ReferredBy determines if the supplied node refers the Vararg in question via a store instruction.
+func (v *Varargs) ReferredBy(r referrer) bool {
+	for _, s := range v.stores {
+		if r.HasPathTo(s.Val.(ssa.Node)) {
+			return true
+		}
+	}
+
+	return false
+}


Test patch:
diff --git a/internal/pkg/varargs/testdata/src/base/test.go b/internal/pkg/varargs/testdata/src/base/test.go
new file mode 100644
index 0000000..24c8ffd
--- /dev/null
+++ b/internal/pkg/varargs/testdata/src/base/test.go
@@ -0,0 +1,24 @@
+// Copyright 2020 Google LLC
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// https://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package base
+
+import "fmt"
+
+type source struct{}
+
+func f1() {
+	s := &source{}
+	fmt.Printf("%v", s)
+}
diff --git a/internal/pkg/varargs/testdata/src/empty/test.go b/internal/pkg/varargs/testdata/src/empty/test.go
new file mode 100644
index 0000000..3b68f95
--- /dev/null
+++ b/internal/pkg/varargs/testdata/src/empty/test.go
@@ -0,0 +1,21 @@
+// Copyright 2020 Google LLC
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// https://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package empty
+
+import "fmt"
+
+func f1() {
+	fmt.Print()
+}
diff --git a/internal/pkg/varargs/testdata/src/multiple/test.go b/internal/pkg/varargs/testdata/src/multiple/test.go
new file mode 100644
index 0000000..936f40a
--- /dev/null
+++ b/internal/pkg/varargs/testdata/src/multiple/test.go
@@ -0,0 +1,24 @@
+// Copyright 2020 Google LLC
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// https://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package multiple
+
+import "fmt"
+
+type source struct{}
+
+func f1() {
+	s := &source{}
+	fmt.Printf("%v, %v", s, s)
+}
diff --git a/internal/pkg/varargs/testdata/src/no-connection-to-source/test.go b/internal/pkg/varargs/testdata/src/no-connection-to-source/test.go
new file mode 100644
index 0000000..4aaa143
--- /dev/null
+++ b/internal/pkg/varargs/testdata/src/no-connection-to-source/test.go
@@ -0,0 +1,30 @@
+// Copyright 2020 Google LLC
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// https://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package source
+
+import (
+	"fmt"
+	"reflect"
+)
+
+type source struct{}
+
+func f1() {
+	s := &source{}
+	// Touching "s" via reflect to keep the compiler happy.
+	reflect.TypeOf(s)
+
+	fmt.Printf("%v", "hello")
+}
diff --git a/internal/pkg/varargs/varargs_test.go b/internal/pkg/varargs/varargs_test.go
new file mode 100644
index 0000000..78917b7
--- /dev/null
+++ b/internal/pkg/varargs/varargs_test.go
@@ -0,0 +1,159 @@
+// Copyright 2020 Google LLC
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// https://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package varargs
+
+import (
+	"go/types"
+	"reflect"
+	"testing"
+
+	"github.com/google/go-flow-levee/internal/pkg/source"
+
+	"golang.org/x/tools/go/analysis"
+	"golang.org/x/tools/go/analysis/analysistest"
+	"golang.org/x/tools/go/analysis/passes/buildssa"
+	"golang.org/x/tools/go/ssa"
+)
+
+type testConfig struct {
+	propagatorsPattern string
+	fieldsPattern      string
+	sanitizerPattern   string
+}
+
+func (c *testConfig) IsSource(t types.Type) bool {
+	return true
+}
+
+func (c *testConfig) IsSanitizer(call *ssa.Call) bool {
+	return false
+}
+
+func (c *testConfig) IsPropagator(call *ssa.Call) bool {
+	return false
+}
+
+func (c *testConfig) IsSourceFieldAddr(field *ssa.FieldAddr) bool {
+	return false
+}
+
+type analyzerResult struct {
+	allocations []*ssa.Alloc
+	calls       []*ssa.Call
+	store       []*ssa.Store
+}
+
+var testAnalyzer = &analysis.Analyzer{
+	Name:       "varargs",
+	Run:        run,
+	Doc:        "test harness for varargs",
+	Requires:   []*analysis.Analyzer{buildssa.Analyzer},
+	ResultType: reflect.TypeOf(analyzerResult{}),
+}
+
+func run(pass *analysis.Pass) (interface{}, error) {
+	in := pass.ResultOf[buildssa.Analyzer].(*buildssa.SSA)
+	var result analyzerResult
+	for _, fn := range in.SrcFuncs {
+		for _, b := range fn.Blocks {
+			for _, i := range b.Instrs {
+				switch v := i.(type) {
+				case *ssa.Alloc:
+					result.allocations = append(result.allocations, v)
+				case *ssa.Call:
+					result.calls = append(result.calls, v)
+				case *ssa.Store:
+					result.store = append(result.store, v)
+				}
+			}
+		}
+	}
+
+	return result, nil
+}
+
+func TestVarargs(t *testing.T) {
+	var testCases = []struct {
+		pattern               string
+		expectVararg          bool
+		callUnderTestIdx      int
+		allocUnderTestIdx     int
+		wantConnectionToAlloc bool
+		wantStores            int
+	}{
+		{
+			pattern:               "base",
+			callUnderTestIdx:      0,
+			wantConnectionToAlloc: true,
+			wantStores:            1,
+		},
+		{
+			pattern:               "empty",
+			callUnderTestIdx:      0,
+			wantConnectionToAlloc: true,
+			wantStores:            1,
+		},
+		{
+			pattern:               "multiple",
+			callUnderTestIdx:      0,
+			wantConnectionToAlloc: true,
+			wantStores:            2,
+		},
+		{
+			pattern:          "no-connection-to-source",
+			callUnderTestIdx: 0,
+			wantStores:       1,
+		},
+	}
+
+	for _, tt := range testCases {
+		t.Run(tt.pattern, func(t *testing.T) {
+			dir := analysistest.TestData()
+			r := analysistest.Run(t, dir, testAnalyzer, tt.pattern)
+			if len(r) != 1 {
+				t.Fatalf("Got %d results, wanted one", len(r))
+			}
+
+			a, ok := r[0].Result.(analyzerResult)
+			if !ok {
+				t.Fatalf("Got result of type %T, wanted analyzerResult", a)
+			}
+
+			got := New(a.calls[tt.callUnderTestIdx])
+			if got == nil && tt.expectVararg {
+				t.Fatal("Got nil wanted varargs")
+			}
+
+			if got == nil {
+				return
+			}
+
+			for i := 0; i < tt.wantStores; i++ {
+				if got.stores[i] != a.store[i] {
+					t.Fatalf("Expected %v == %v for store #%d. Referres: %v", got.stores[i], a.store[i], i, got.stores[i].Referrers())
+				}
+			}
+
+			s := source.New(a.allocations[tt.allocUnderTestIdx], &testConfig{})
+			if s == nil {
+				t.Fatal("Expected a source got got nil at a.allocations[0]")
+			}
+
+			if !got.ReferredBy(s) && tt.wantConnectionToAlloc {
+				t.Fatalf("Expected source %v to refer to vararg %v", s, got)
+			}
+		})
+	}
+}