Repository: google/go-flow-levee
Pull request #156 (2020-10-19 20:07:39)

Add tests for named return values
This PR adds tests for named return values.

In this case:
```go
func TestNamedReturnValue() (s core.Source) {
	core.Sink(s) // want "a source has reached a sink"
	return
}
```
`s` will get an `Alloc`, as though it had been declared in the body. In the function's `ssa`, `s` is listed as a local variable.

If we change `core.Source` to `*core.Source`, we no longer expect a report. Indeed, the code becomes roughly equivalent to:
```go
func Test() {
	var s *core.Source
	_ = s
}
```
In this case, `s` does not get an `Alloc` (because nothing needs to be allocated), and in fact `s` is not even listed as a local variable (because it is simply nil).

If a value were assigned to the pointer return value, then it would be detected by other means.

- [x] Tests pass
- [x] Running against a large codebase such as [Kubernetes](https://github.com/kubernetes/kubernetes) does not error out.
- [x] Appropriate changes to README are included in PR


Patch:
diff --git a/internal/pkg/fieldtags/analyzer.go b/internal/pkg/fieldtags/analyzer.go
index 542d93d..9d50553 100644
--- a/internal/pkg/fieldtags/analyzer.go
+++ b/internal/pkg/fieldtags/analyzer.go
@@ -86,61 +86,6 @@ func run(pass *analysis.Pass) (interface{}, error) {
 	return ResultType(taggedFields), nil
 }
 
-func extractTags(field *ast.Field) (tags []fieldTag) {
-	if field.Tag == nil {
-		return
-	}
-
-	wholeTag := field.Tag.Value
-
-	// TODO: consider refactoring this logic into a regex matcher
-	i := 1 // skip initial quote
-	j := 1
-	for j < len(wholeTag) {
-		for j < len(wholeTag) && wholeTag[j] != ':' {
-			j++
-		}
-		key := wholeTag[i:j]
-		if key == "" {
-			return
-		}
-
-		i = j + 1 // skip colon
-		if i >= len(wholeTag) {
-			return
-		}
-		if wholeTag[i] == '\\' {
-			i++
-		}
-		i++ // skip quote
-
-		j = i
-		for j < len(wholeTag) && wholeTag[j] != '"' {
-			// skip escape character
-			if wholeTag[j] == '\\' {
-				j++
-			}
-			j++
-		}
-		value := wholeTag[i:j]
-		// remove trailing escaped quote if present
-		value = strings.TrimSuffix(value, `\"`)
-		if value == "" {
-			return
-		}
-
-		// value may be a list of comma separated values
-		for _, v := range strings.Split(value, ",") {
-			tags = append(tags, fieldTag{key: key, val: v})
-		}
-
-		i = j + 2 // skip closing quote and space
-		j = i
-	}
-
-	return tags
-}
-
 // IsSourceFieldAddr determines whether a ssa.FieldAddr is a source, that is whether it refers to a field previously identified as a source.
 func (r ResultType) IsSourceFieldAddr(fa *ssa.FieldAddr) bool {
 	// incantation plundered from the docstring for ssa.FieldAddr.Field
diff --git a/internal/pkg/sanitizer/sanitizer.go b/internal/pkg/sanitizer/sanitizer.go
index b703d45..fd05ab9 100644
--- a/internal/pkg/sanitizer/sanitizer.go
+++ b/internal/pkg/sanitizer/sanitizer.go
@@ -38,8 +38,8 @@ func (s Sanitizer) Dominates(target ssa.Instruction) bool {
 		return false
 	}
 
-	if !s.Call.Block().Dominates(target.Block()) {
-		return false
+	if s.Call.Block().Dominates(target.Block()) && s.Call.Block() != target.Block() {
+		return true
 	}
 
 	if s.Call.Block() == target.Block() {


Test patch:
diff --git a/internal/pkg/levee/levee_test.go b/internal/pkg/levee/levee_test.go
index 68b30fb..a491c4c 100644
--- a/internal/pkg/levee/levee_test.go
+++ b/internal/pkg/levee/levee_test.go
@@ -16,8 +16,10 @@ package internal
 
 import (
 	"flag"
+	"fmt"
 	"io/ioutil"
 	"path/filepath"
+	"strings"
 	"testing"
 
 	"github.com/google/go-flow-levee/internal/pkg/debug"
@@ -46,7 +48,28 @@ func findTestPatterns(t *testing.T, testsDir string) (patterns []string) {
 		t.Fatalf("Failed to read tests dir (%s): %v", testsDir, err)
 	}
 	for _, f := range files {
-		patterns = append(patterns, filepath.Join(testsDir, f.Name()))
+		path := filepath.Join(testsDir, f.Name())
+		// Make sure the directory contains a go file to avoid failure on empty directories.
+		if err := checkForGoFiles(path); err != nil {
+			t.Fatalf("Could not verify presence of Go files in test directory: %v", err)
+		}
+
+		patterns = append(patterns, path)
 	}
 	return
 }
+
+func checkForGoFiles(path string) error {
+	pkgFiles, err := ioutil.ReadDir(path)
+	if err != nil {
+		return fmt.Errorf("failed to examine test directory (%s): %v", path, err)
+	}
+
+	for _, file := range pkgFiles {
+		if strings.HasSuffix(file.Name(), ".go") {
+			return nil
+		}
+	}
+	return fmt.Errorf("found no Go files in test directory (%s)", path)
+
+}
diff --git a/internal/pkg/levee/testdata/src/example.com/tests/namedreturn/tests.go b/internal/pkg/levee/testdata/src/example.com/tests/namedreturn/tests.go
new file mode 100644
index 0000000..89600e9
--- /dev/null
+++ b/internal/pkg/levee/testdata/src/example.com/tests/namedreturn/tests.go
@@ -0,0 +1,35 @@
+// 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 namedreturn
+
+import (
+	"example.com/core"
+)
+
+func TestNamedReturnValue() (s core.Source) {
+	core.Sink(s) // want "a source has reached a sink"
+	return
+}
+
+func TestNamedReturnValuePointer() (s *core.Source) {
+	core.Sink(s) // we do not report here, because no Source value has been created, so s must be nil
+	return
+}
+
+func TestNamedReturnValuePointerWithInitialization() (s *core.Source) {
+	s = &core.Source{}
+	core.Sink(s) // want "a source has reached a sink"
+	return
+}
diff --git a/internal/pkg/levee/testdata/src/example.com/tests/sanitization/tests.go b/internal/pkg/levee/testdata/src/example.com/tests/sanitization/tests.go
index 5dc8787..b887894 100644
--- a/internal/pkg/levee/testdata/src/example.com/tests/sanitization/tests.go
+++ b/internal/pkg/levee/testdata/src/example.com/tests/sanitization/tests.go
@@ -57,3 +57,83 @@ func TestIncorrectSanitizationByValue(s core.Source) {
 	core.Sanitize(s)
 	core.Sink(s) // TODO want "a source has reached a sink"
 }
+
+func TestOnlySanitizedIfLoopIsTaken() {
+	var e interface{} = core.Source{}
+	for false {
+		e = core.Sanitize(e)[0]
+	}
+	core.Sink(e) // want "a source has reached a sink"
+}
+
+func TestTaintedInLoopAndSanitizedAfterLoop() {
+	var e interface{}
+	for false {
+		e = core.Source{}
+	}
+	e = core.Sanitize(e)[0]
+	core.Sink(e)
+}
+
+func TestMaybeTaintedInLoopButSanitizedBeforeLoopExit() {
+	var e interface{}
+	for false {
+		if false {
+			e = core.Source{}
+		}
+		e = core.Sanitize(e)[0]
+	}
+	// TODO want no report here
+	core.Sink(e) // want "a source has reached a sink"
+}
+
+func TestTaintedInIfButSanitizedBeforeIfExit() {
+	var e interface{}
+	if false {
+		e = core.Source{}
+		e = core.Sanitize(e)[0]
+	}
+	// TODO want no report here
+	core.Sink(e) // want "a source has reached a sink"
+}
+
+func TestPointerTaintedInIfButSanitizedBeforeIfExit() {
+	var e interface{}
+	if false {
+		s := &core.Source{}
+		core.SanitizePtr(s)
+		e = s
+	}
+	// TODO want no report here
+	core.Sink(e) // want "a source has reached a sink"
+}
+
+func TestSanitizedBeforeSinkInLoop() {
+	var e interface{}
+	for false {
+		e = core.Source{}
+		e = core.Sanitize(e)[0]
+		core.Sink(e)
+	}
+}
+
+func TestSanitizedBeforeMaybeSinkingMaybeTaintedValue() {
+	var obj interface{}
+	if false {
+		obj = core.Source{}
+	} else {
+		obj = 10
+	}
+
+	obj = core.Sanitize(obj)[0]
+
+	if false {
+		core.Sink(obj)
+	}
+}
+
+func TestSanitizedAfterSink() {
+	s := core.Source{}
+	core.Sink(s) // want "a source has reached a sink"
+	core.SanitizePtr(&s)
+}
diff --git a/internal/pkg/sanitizer/sanitizer_test.go b/internal/pkg/sanitizer/sanitizer_test.go
index 491bc84..8c55031 100644
--- a/internal/pkg/sanitizer/sanitizer_test.go
+++ b/internal/pkg/sanitizer/sanitizer_test.go
@@ -15,7 +15,6 @@
 package sanitizer
 
 import (
-	"reflect"
 	"testing"
 
 	"golang.org/x/tools/go/analysis"
@@ -25,60 +24,43 @@ import (
 )
 
 var testAnalyzer = &analysis.Analyzer{
-	Name:       "domination",
-	Run:        run,
-	Doc:        "test harness for domination logic",
-	Requires:   []*analysis.Analyzer{buildssa.Analyzer},
-	ResultType: reflect.TypeOf([]*ssa.Call{}),
+	Name:     "domination",
+	Run:      run,
+	Doc:      "test harness for domination logic",
+	Requires: []*analysis.Analyzer{buildssa.Analyzer},
 }
 
 func run(pass *analysis.Pass) (interface{}, error) {
 	in := pass.ResultOf[buildssa.Analyzer].(*buildssa.SSA)
-	var calls []*ssa.Call
+	var sanitizers []Sanitizer
+	var sinks []*ssa.Call
 	for _, fn := range in.SrcFuncs {
 		for _, b := range fn.Blocks {
 			for _, i := range b.Instrs {
 				if c, ok := i.(*ssa.Call); ok {
-					calls = append(calls, c)
+					name := c.Call.StaticCallee().Name()
+					switch name {
+					case "scrub":
+						sanitizers = append(sanitizers, Sanitizer{c})
+					case "Print":
+						sinks = append(sinks, c)
+					}
 				}
 			}
 		}
 	}
 
-	return calls, nil
+	for _, sink := range sinks {
+		for _, san := range sanitizers {
+			if san.Dominates(sink) {
+				pass.Reportf(sink.Pos(), "dominated")
+			}
+		}
+	}
+	return nil, nil
 }
 
 func TestDomination(t *testing.T) {
 	dir := analysistest.TestData()
-	r := analysistest.Run(t, dir, testAnalyzer)
-	if len(r) != 1 {
-		t.Fatalf("Got %d results, wanted one", len(r))
-	}
-	calls, ok := r[0].Result.([]*ssa.Call)
-	if !ok {
-		t.Fatalf("Got result of type %T, wanted []*ssa.Call", calls)
-	}
-
-	/*
-		    Call Sequence:
-
-				call time.Now()
-				call (time.Time).Weekday(t0)
-				call scrub("P@ssword1":string) // Sanitizer - index 2
-				call log.Print(t7...) // Dominated sink
-				call log.Print(t13...) // Non dominated sink due to an if statement.
-
-	*/
-
-	sanitizer := &Sanitizer{calls[2]}
-	dominatedSink := calls[3]
-	nonDominatedSink := calls[4]
-
-	if !sanitizer.Dominates(dominatedSink) {
-		t.Fatalf("Sanitizer %v should dominate the leak %v due to an if statement", sanitizer.Call, dominatedSink)
-	}
-
-	if sanitizer.Dominates(nonDominatedSink) {
-		t.Fatalf("Sanitizer %v should not dominate the leak %v due to no if statement", sanitizer.Call, nonDominatedSink)
-	}
+	analysistest.Run(t, dir, testAnalyzer)
 }
diff --git a/internal/pkg/sanitizer/testdata/tests.go b/internal/pkg/sanitizer/testdata/tests.go
index ac9a826..0679b6f 100644
--- a/internal/pkg/sanitizer/testdata/tests.go
+++ b/internal/pkg/sanitizer/testdata/tests.go
@@ -20,11 +20,31 @@ import (
 	"time"
 )
 
-func notDominated() {
+func dominatedSameBlock() {
+	pwd := "password"
+	pwd = scrub(pwd)
+	log.Print(pwd) // want "dominated"
+}
+
+func notDominatedSameBlock() {
+	pwd := "password"
+	log.Print(pwd)
+	pwd = scrub(pwd)
+}
+
+func dominatedDifferentBlocks() {
+	pwd := "password"
+	pwd = scrub(pwd)
+	if time.Now().Weekday() == time.Monday {
+		log.Print(pwd) // want "dominated"
+	}
+}
+
+func notDominatedDifferentBlocks() {
 	pwd := "P@ssword1"
 	if time.Now().Weekday() == time.Monday {
 		pwd = scrub(pwd)
-		log.Print(pwd)
+		log.Print(pwd) // want "dominated"
 	}
 
 	log.Print(pwd)
diff --git a/internal/pkg/source/testdata/src/analyzertest/sourcetest/identification.go b/internal/pkg/source/testdata/src/analyzertest/sourcetest/identification.go
index d109d88..fffd2dd 100644
--- a/internal/pkg/source/testdata/src/analyzertest/sourcetest/identification.go
+++ b/internal/pkg/source/testdata/src/analyzertest/sourcetest/identification.go
@@ -80,6 +80,13 @@ func TestSourceParameters(val Source, ptr *Source) { // want "source identified"
 
 }
 
+// A report should be emitted for val, because it will have an Alloc in the body.
+// A report should *not* be emitted for ptr, because it will not, by itself, lead to
+// the creation of a Source value.
+func TestNamedReturnValues() (val Source, ptr *Source) { // want "source identified"
+	return
+}
+
 func TestSourceExtracts() {
 	s, err := CreateSource() // want "source identified"
 	sptr, err := NewSource() // want "source identified"