From 0835c735343e0d8e375f0b980b358619ff0853a2 Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Mon, 11 Jul 2016 16:38:22 -0600 Subject: [PATCH] imports: special case rand.Read, prevent math/rand by chance In Go 1.7, math/rand.Read was added. Previously, the only package containing "rand.Read" was "crypto/rand". goimports was updated to know that, and zstdlib.go contains a note that it's ambiguous: "rand.Perm": "math/rand", "rand.Prime": "crypto/rand", "rand.Rand": "math/rand", // "rand.Read" is ambiguous "rand.Reader": "crypto/rand", "rand.Seed": "math/rand", "rand.Source": "math/rand", The intention originally was that such ambiguous things would never be resolved, even randomly. But a later change added support for build.Default.SrcDirs, which meant GOROOT was also searched for ambiguous things. Or maybe I forget the history. In any case, when goimports tried to resolve "rand.Read", the findImportStdlib check was returning nothing, which lead to the $GOROOT being searched, where math/rand was picked by chance. That's a dangerous default when the intentional might've been crypto/rand. Special case it and prefer crypto/rand if there's no more specific clue either way. Change-Id: Ib5f8f297f72fa309d5ca9b15a37493df2e17567c Reviewed-on: https://go-review.googlesource.com/24847 Run-TryBot: Brad Fitzpatrick TryBot-Result: Gobot Gobot Reviewed-by: Andrew Gerrand --- imports/fix.go | 19 ++++++++- imports/fix_test.go | 93 ++++++++++++++++++++++++++++++++++----------- 2 files changed, 88 insertions(+), 24 deletions(-) diff --git a/imports/fix.go b/imports/fix.go index be838299..5e260ccc 100644 --- a/imports/fix.go +++ b/imports/fix.go @@ -369,6 +369,16 @@ func findImportGoPath(pkgName string, symbols map[string]bool, filename string) if pkg, rename, ok := findImportStdlib(pkgName, symbols); ok { return pkg, rename, nil } + if pkgName == "rand" && symbols["Read"] { + // Special-case rand.Read. + // + // If findImportStdlib didn't find it above, don't go + // searching for it, lest it find and pick math/rand + // in GOROOT (new as of Go 1.6) + // + // crypto/rand is the safer choice. + return "", false, nil + } // TODO(sameer): look at the import lines for other Go files in the // local directory, since the user is likely to import the same packages @@ -459,8 +469,12 @@ func (fn visitFn) Visit(node ast.Node) ast.Visitor { func findImportStdlib(shortPkg string, symbols map[string]bool) (importPath string, rename, ok bool) { for symbol := range symbols { - path := stdlib[shortPkg+"."+symbol] + key := shortPkg + "." + symbol + path := stdlib[key] if path == "" { + if key == "rand.Read" { + continue + } return "", false, false } if importPath != "" && importPath != path { @@ -469,5 +483,8 @@ func findImportStdlib(shortPkg string, symbols map[string]bool) (importPath stri } importPath = path } + if importPath == "" && shortPkg == "rand" && symbols["Read"] { + return "crypto/rand", false, true + } return importPath, false, importPath != "" } diff --git a/imports/fix_test.go b/imports/fix_test.go index 78a50a8c..3454e228 100644 --- a/imports/fix_test.go +++ b/imports/fix_test.go @@ -1008,38 +1008,85 @@ type Buffer2 struct {} } } -func TestFindImportInternal(t *testing.T) { +func withEmptyGoPath(fn func()) { pkgIndexOnce = &sync.Once{} oldGOPATH := build.Default.GOPATH build.Default.GOPATH = "" defer func() { build.Default.GOPATH = oldGOPATH }() + fn() +} - // Check for src/internal/race, not just src/internal, - // so that we can run this test also against go1.5 - // (which doesn't contain that file). - _, err := os.Stat(filepath.Join(runtime.GOROOT(), "src/internal/race")) - if err != nil { - t.Skip(err) - } +func TestFindImportInternal(t *testing.T) { + withEmptyGoPath(func() { + // Check for src/internal/race, not just src/internal, + // so that we can run this test also against go1.5 + // (which doesn't contain that file). + _, err := os.Stat(filepath.Join(runtime.GOROOT(), "src/internal/race")) + if err != nil { + t.Skip(err) + } - got, rename, err := findImportGoPath("race", map[string]bool{"Acquire": true}, filepath.Join(runtime.GOROOT(), "src/math/x.go")) - if err != nil { - t.Fatal(err) - } - if got != "internal/race" || rename { - t.Errorf(`findImportGoPath("race", Acquire ...)=%q, %t, want "internal/race", false`, got, rename) - } + got, rename, err := findImportGoPath("race", map[string]bool{"Acquire": true}, filepath.Join(runtime.GOROOT(), "src/math/x.go")) + if err != nil { + t.Fatal(err) + } + if got != "internal/race" || rename { + t.Errorf(`findImportGoPath("race", Acquire ...)=%q, %t, want "internal/race", false`, got, rename) + } - // should not be able to use internal from outside that tree - got, rename, err = findImportGoPath("race", map[string]bool{"Acquire": true}, filepath.Join(runtime.GOROOT(), "x.go")) - if err != nil { - t.Fatal(err) - } - if got != "" || rename { - t.Errorf(`findImportGoPath("race", Acquire ...)=%q, %t, want "", false`, got, rename) - } + // should not be able to use internal from outside that tree + got, rename, err = findImportGoPath("race", map[string]bool{"Acquire": true}, filepath.Join(runtime.GOROOT(), "x.go")) + if err != nil { + t.Fatal(err) + } + if got != "" || rename { + t.Errorf(`findImportGoPath("race", Acquire ...)=%q, %t, want "", false`, got, rename) + } + }) +} + +// rand.Read should prefer crypto/rand.Read, not math/rand.Read. +func TestFindImportRandRead(t *testing.T) { + withEmptyGoPath(func() { + file := filepath.Join(runtime.GOROOT(), "src/foo/x.go") // dummy + tests := []struct { + syms []string + want string + }{ + { + syms: []string{"Read"}, + want: "crypto/rand", + }, + { + syms: []string{"Read", "NewZipf"}, + want: "math/rand", + }, + { + syms: []string{"NewZipf"}, + want: "math/rand", + }, + { + syms: []string{"Read", "Prime"}, + want: "crypto/rand", + }, + } + for _, tt := range tests { + m := map[string]bool{} + for _, sym := range tt.syms { + m[sym] = true + } + got, _, err := findImportGoPath("rand", m, file) + if err != nil { + t.Errorf("for %q: %v", tt.syms, err) + continue + } + if got != tt.want { + t.Errorf("for %q, findImportGoPath = %q; want %q", tt.syms, got, tt.want) + } + } + }) } func TestFindImportVendor(t *testing.T) {