cmd/go/internal/modindex: clean modroot and pkgdir for openIndexPackage

GetPackage is sometimes called with a modroot or pkgdir that ends with a
path separator, and sometimes without. Clean them before passing them to
openIndexPackage to deduplicate calls to mcache.Do and action entry
files.

This shouldn't affect #71698 but was discovered while debugging that
issue.

Change-Id: I6a7fa4de93f45801504abea11bd97f6c6577f296
Reviewed-on: https://go-review.googlesource.com/c/go/+/652435
Reviewed-by: Alan Donovan <adonovan@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>

diff --git a/src/cmd/go/internal/modindex/read.go b/src/cmd/go/internal/modindex/read.go
index 76216f35ba149f0815093b506516905d8fe0f9e9..913f49994f28e6adfe78d38ec9da2510a5be5a92 100644 (file)
--- a/src/cmd/go/internal/modindex/read.go
+++ b/src/cmd/go/internal/modindex/read.go
@@ -146,6 +146,8 @@ func GetPackage(modroot, pkgdir string) (*IndexPackage, error) {
        if strings.Contains(filepath.ToSlash(pkgdir), "internal/fips140/v") {
                return nil, errFIPS140
        }
+       modroot = filepath.Clean(modroot)
+       pkgdir = filepath.Clean(pkgdir)
        return openIndexPackage(modroot, pkgdir)
 }
 

diff --git a/src/cmd/go/testdata/script/list_modindex_dupactionid.txt b/src/cmd/go/testdata/script/list_modindex_dupactionid.txt
new file mode 100644 (file)
index 0000000..0a9d32b
--- /dev/null
+++ b/src/cmd/go/testdata/script/list_modindex_dupactionid.txt
@@ -0,0 +1,100 @@
+# This is a regression test for #652435. It checks that we don't generate
+# multiple action entry ids for the same index file. That was happening
+# previously because we sometimes generated the action id with unclean
+# paths (and the rest of the time with clean paths) for the same package.
+# This test will use a go program ('check') to check the cache that there are
+# no two action entry files that point to the same object id.
+
+[short] skip 'builds and runs a go program'
+sleep 2s # Sleep so that the unpacked files are > 2 seconds old. The index won't be used if the modified times on the files are newer.
+go build -o check$GOEXE check.go
+cd mod
+env GOCACHE=$WORK/newcache # Run list command in a clean cache.
+go list all
+exec ../check$GOEXE
+
+-- mod/go.mod --
+module example.com/foo
+-- mod/foo.go --
+package foo
+-- check.go --
+package main
+
+import (
+       "errors"
+       "log"
+       "os"
+       "path/filepath"
+       "strings"
+)
+
+func main() {
+       cachedir := os.Getenv("GOCACHE")
+       if cachedir == "" {
+               log.Fatal("GOCACHE env var is empty; expected it to be set")
+       }
+
+       // Read the top level cache directory. The cache directory contains directories with
+       // each of the possible two hex digit prefixes (00-FF) of a cache entry's id.
+       // Those directories in turn contain files with the hex id followed by either
+       // "-a" for the action entries or "-d" for the object entries. We want to check
+       // if two action entries point to the same object entry so we'll iterate through
+       // all the "-a" files and see if any two of them refer to the same object id
+       // (corresponding to a "-d" file).
+       dirs, err := os.ReadDir(cachedir)
+       if err != nil {
+               log.Fatal(err)
+       }
+
+       seen := map[string]string{} // object id -> action id
+
+       for _, entry := range dirs {
+               if entry.IsDir() && len(entry.Name()) == 2 {
+                       prefixdir := filepath.Join(cachedir, entry.Name())
+                       entries, err := os.ReadDir(prefixdir)
+                       if err != nil {
+                               log.Fatal(err)
+                       }
+
+                       for _, entry := range entries {
+                               if !strings.HasSuffix(entry.Name(), "-a") {
+                                       // not an action id entry
+                                       continue
+                               }
+                               actionEntryFile := filepath.Join(prefixdir, entry.Name())
+                               objid, err := objectid(actionEntryFile)
+                               if err != nil {
+                                       log.Fatal(err)
+                               }
+                               if other, ok := seen[objid]; ok {
+                                       log.Printf("found two action entry files (%s, %s) pointing to the same object id: %s", other, entry.Name(), objid)
+                                       os.Exit(1)
+                               }
+                               seen[objid] = entry.Name()
+                       }
+               }
+       }
+}
+
+// objectid returns the object id that the given actionEntryFile points to.
+func objectid(actionEntryFile string) (string, error) {
+       // See cmd/go/internal/cache.(*DiskCache).get for the code that reads
+       // from the action entry files. The following is based on that function.
+       const (
+               HashSize  = 32
+               hexSize   = HashSize * 2
+               entrySize = 2 + 1 + hexSize + 1 + hexSize + 1 + 20 + 1 + 20 + 1
+       )
+
+       entry, err := os.ReadFile(actionEntryFile)
+       if err != nil {
+               return "", err
+       }
+       if len(entry) < entrySize {
+               return "", errors.New("entry file incomplete")
+       }
+       if entry[0] != 'v' || entry[1] != '1' || entry[2] != ' ' || entry[3+hexSize] != ' ' || entry[3+hexSize+1+hexSize] != ' ' || entry[3+hexSize+1+hexSize+1+20] != ' ' || entry[entrySize-1] != '\n' {
+               return "", errors.New("invalid header")
+       }
+       return string(entry[3+hexSize+1 : 3+hexSize+1+hexSize]), nil
+}