From 507a88c39bb1089b9d44facb7dd3449a9b5a3e10 Mon Sep 17 00:00:00 2001 From: Jay Conrod Date: Thu, 1 Oct 2020 13:37:06 -0400 Subject: cmd/go/internal/modfetch: always extract module directories in place Previously by default, we extracted modules to a temporary directory, then renamed it into place. This failed with ERROR_ACCESS_DENIED on Windows if another process (usually an anti-virus scanner) opened files in the temporary directory. Since Go 1.15, users have been able to set GODEBUG=modcacheunzipinplace=1 to opt into new behavior: we extract modules at their final location, and we create and later delete a .partial file to prevent the directory from being used if we crash. .partial files are recognized by Go 1.14.2 and later. With this change, the new behavior is the only behavior. modcacheunzipinplace is no longer recognized. Fixes #36568 Change-Id: Iff19fca5cd6eaa3597975a69fa05c4cb1b834bd6 Reviewed-on: https://go-review.googlesource.com/c/go/+/258798 Run-TryBot: Jay Conrod TryBot-Result: Go Bot Trust: Jay Conrod Reviewed-by: Bryan C. Mills --- src/cmd/go/internal/modfetch/fetch.go | 105 ++++++++++------------------------ 1 file changed, 31 insertions(+), 74 deletions(-) (limited to 'src/cmd/go/internal/modfetch/fetch.go') diff --git a/src/cmd/go/internal/modfetch/fetch.go b/src/cmd/go/internal/modfetch/fetch.go index 01d8f007ac..1d90002faa 100644 --- a/src/cmd/go/internal/modfetch/fetch.go +++ b/src/cmd/go/internal/modfetch/fetch.go @@ -63,12 +63,9 @@ func download(ctx context.Context, mod module.Version) (dir string, err error) { ctx, span := trace.StartSpan(ctx, "modfetch.download "+mod.String()) defer span.Done() - // If the directory exists, and no .partial file exists, the module has - // already been completely extracted. .partial files may be created when a - // module zip directory is extracted in place instead of being extracted to a - // temporary directory and renamed. dir, err = DownloadDir(mod) if err == nil { + // The directory has already been completely extracted (no .partial file exists). return dir, nil } else if dir == "" || !errors.Is(err, os.ErrNotExist) { return "", err @@ -88,6 +85,9 @@ func download(ctx context.Context, mod module.Version) (dir string, err error) { } defer unlock() + ctx, span = trace.StartSpan(ctx, "unzip "+zipfile) + defer span.Done() + // Check whether the directory was populated while we were waiting on the lock. _, dirErr := DownloadDir(mod) if dirErr == nil { @@ -95,10 +95,11 @@ func download(ctx context.Context, mod module.Version) (dir string, err error) { } _, dirExists := dirErr.(*DownloadDirPartialError) - // Clean up any remaining temporary directories from previous runs, as well - // as partially extracted diectories created by future versions of cmd/go. - // This is only safe to do because the lock file ensures that their writers - // are no longer active. + // Clean up any remaining temporary directories created by old versions + // (before 1.16), as well as partially extracted directories (indicated by + // DownloadDirPartialError, usually because of a .partial file). This is only + // safe to do because the lock file ensures that their writers are no longer + // active. parentDir := filepath.Dir(dir) tmpPrefix := filepath.Base(dir) + ".tmp-" if old, err := filepath.Glob(filepath.Join(parentDir, tmpPrefix+"*")); err == nil { @@ -116,88 +117,44 @@ func download(ctx context.Context, mod module.Version) (dir string, err error) { if err != nil { return "", err } - if err := os.Remove(partialPath); err != nil && !os.IsNotExist(err) { - return "", err - } - // Extract the module zip directory. + // Extract the module zip directory at its final location. // - // By default, we extract to a temporary directory, then atomically rename to - // its final location. We use the existence of the source directory to signal - // that it has been extracted successfully (see DownloadDir). If someone - // deletes the entire directory (e.g., as an attempt to prune out file - // corruption), the module cache will still be left in a recoverable - // state. + // To prevent other processes from reading the directory if we crash, + // create a .partial file before extracting the directory, and delete + // the .partial file afterward (all while holding the lock). // - // Unfortunately, os.Rename may fail with ERROR_ACCESS_DENIED on Windows if - // another process opens files in the temporary directory. This is partially - // mitigated by using robustio.Rename, which retries os.Rename for a short - // time. + // Before Go 1.16, we extracted to a temporary directory with a random name + // then renamed it into place with os.Rename. On Windows, this failed with + // ERROR_ACCESS_DENIED when another process (usually an anti-virus scanner) + // opened files in the temporary directory. // - // To avoid this error completely, if unzipInPlace is set, we instead create a - // .partial file (indicating the directory isn't fully extracted), then we - // extract the directory at its final location, then we delete the .partial - // file. This is not the default behavior because older versions of Go may - // simply stat the directory to check whether it exists without looking for a - // .partial file. If multiple versions run concurrently, the older version may - // assume a partially extracted directory is complete. - // TODO(golang.org/issue/36568): when these older versions are no longer - // supported, remove the old default behavior and the unzipInPlace flag. + // Go 1.14.2 and higher respect .partial files. Older versions may use + // partially extracted directories. 'go mod verify' can detect this, + // and 'go clean -modcache' can fix it. if err := os.MkdirAll(parentDir, 0777); err != nil { return "", err } - - ctx, span = trace.StartSpan(ctx, "unzip "+zipfile) - if unzipInPlace { - if err := ioutil.WriteFile(partialPath, nil, 0666); err != nil { - return "", err - } - if err := modzip.Unzip(dir, mod, zipfile); err != nil { - fmt.Fprintf(os.Stderr, "-> %s\n", err) - if rmErr := RemoveAll(dir); rmErr == nil { - os.Remove(partialPath) - } - return "", err - } - if err := os.Remove(partialPath); err != nil { - return "", err - } - } else { - tmpDir, err := ioutil.TempDir(parentDir, tmpPrefix) - if err != nil { - return "", err - } - if err := modzip.Unzip(tmpDir, mod, zipfile); err != nil { - fmt.Fprintf(os.Stderr, "-> %s\n", err) - RemoveAll(tmpDir) - return "", err - } - if err := robustio.Rename(tmpDir, dir); err != nil { - RemoveAll(tmpDir) - return "", err + if err := ioutil.WriteFile(partialPath, nil, 0666); err != nil { + return "", err + } + if err := modzip.Unzip(dir, mod, zipfile); err != nil { + fmt.Fprintf(os.Stderr, "-> %s\n", err) + if rmErr := RemoveAll(dir); rmErr == nil { + os.Remove(partialPath) } + return "", err + } + if err := os.Remove(partialPath); err != nil { + return "", err } - defer span.Done() if !cfg.ModCacheRW { - // Make dir read-only only *after* renaming it. - // os.Rename was observed to fail for read-only directories on macOS. makeDirsReadOnly(dir) } return dir, nil } -var unzipInPlace bool - -func init() { - for _, f := range strings.Split(os.Getenv("GODEBUG"), ",") { - if f == "modcacheunzipinplace=1" { - unzipInPlace = true - break - } - } -} - var downloadZipCache par.Cache // DownloadZip downloads the specific module version to the -- cgit v1.2.1 From c9211577eb77df9c51f0565f1da7d20ff91d59df Mon Sep 17 00:00:00 2001 From: "Bryan C. Mills" Date: Fri, 2 Oct 2020 16:25:17 -0400 Subject: cmd/go/internal/modfetch: remove error return from Lookup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We generally don't care about errors in resolving a repo if the result we're looking for is already in the module cache. Moreover, we can avoid some expense in initializing the repo if all of the methods we plan to call on it hit in the cache — especially when using GOPROXY=direct. This also incidentally fixes a possible (but rare) bug in Download: we had forgotten to reset the downloaded file in case the Zip method returned an error after writing a nonzero number of bytes. For #37438 Change-Id: Ib64f10f763f6d1936536b8e1f7d31ed1b463e955 Reviewed-on: https://go-review.googlesource.com/c/go/+/259158 Trust: Bryan C. Mills Trust: Jay Conrod Run-TryBot: Bryan C. Mills TryBot-Result: Go Bot Reviewed-by: Michael Matloob Reviewed-by: Jay Conrod --- src/cmd/go/internal/modfetch/fetch.go | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-) (limited to 'src/cmd/go/internal/modfetch/fetch.go') diff --git a/src/cmd/go/internal/modfetch/fetch.go b/src/cmd/go/internal/modfetch/fetch.go index 1d90002faa..599419977a 100644 --- a/src/cmd/go/internal/modfetch/fetch.go +++ b/src/cmd/go/internal/modfetch/fetch.go @@ -233,12 +233,28 @@ func downloadZip(ctx context.Context, mod module.Version, zipfile string) (err e } }() + var unrecoverableErr error err = TryProxies(func(proxy string) error { - repo, err := Lookup(proxy, mod.Path) + if unrecoverableErr != nil { + return unrecoverableErr + } + repo := Lookup(proxy, mod.Path) + err := repo.Zip(f, mod.Version) if err != nil { - return err + // Zip may have partially written to f before failing. + // (Perhaps the server crashed while sending the file?) + // Since we allow fallback on error in some cases, we need to fix up the + // file to be empty again for the next attempt. + if _, err := f.Seek(0, io.SeekStart); err != nil { + unrecoverableErr = err + return err + } + if err := f.Truncate(0); err != nil { + unrecoverableErr = err + return err + } } - return repo.Zip(f, mod.Version) + return err }) if err != nil { return err -- cgit v1.2.1 From 7bb721b9384bdd196befeaed593b185f7f2a5589 Mon Sep 17 00:00:00 2001 From: Russ Cox Date: Tue, 7 Jul 2020 13:49:21 -0400 Subject: all: update references to symbols moved from os to io/fs The old os references are still valid, but update our code to reflect best practices and get used to the new locations. Code compiled with the bootstrap toolchain (cmd/asm, cmd/dist, cmd/compile, debug/elf) must remain Go 1.4-compatible and is excluded. For #41190. Change-Id: I8f9526977867c10a221e2f392f78d7dec073f1bd Reviewed-on: https://go-review.googlesource.com/c/go/+/243907 Trust: Russ Cox Run-TryBot: Russ Cox TryBot-Result: Go Bot Reviewed-by: Rob Pike --- src/cmd/go/internal/modfetch/fetch.go | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) (limited to 'src/cmd/go/internal/modfetch/fetch.go') diff --git a/src/cmd/go/internal/modfetch/fetch.go b/src/cmd/go/internal/modfetch/fetch.go index 599419977a..6ff455e89c 100644 --- a/src/cmd/go/internal/modfetch/fetch.go +++ b/src/cmd/go/internal/modfetch/fetch.go @@ -11,6 +11,7 @@ import ( "errors" "fmt" "io" + "io/fs" "io/ioutil" "os" "path/filepath" @@ -67,7 +68,7 @@ func download(ctx context.Context, mod module.Version) (dir string, err error) { if err == nil { // The directory has already been completely extracted (no .partial file exists). return dir, nil - } else if dir == "" || !errors.Is(err, os.ErrNotExist) { + } else if dir == "" || !errors.Is(err, fs.ErrNotExist) { return "", err } @@ -314,10 +315,10 @@ func downloadZip(ctx context.Context, mod module.Version, zipfile string) (err e func makeDirsReadOnly(dir string) { type pathMode struct { path string - mode os.FileMode + mode fs.FileMode } var dirs []pathMode // in lexical order - filepath.Walk(dir, func(path string, info os.FileInfo, err error) error { + filepath.Walk(dir, func(path string, info fs.FileInfo, err error) error { if err == nil && info.Mode()&0222 != 0 { if info.IsDir() { dirs = append(dirs, pathMode{path, info.Mode()}) @@ -336,7 +337,7 @@ func makeDirsReadOnly(dir string) { // any permission changes needed to do so. func RemoveAll(dir string) error { // Module cache has 0555 directories; make them writable in order to remove content. - filepath.Walk(dir, func(path string, info os.FileInfo, err error) error { + filepath.Walk(dir, func(path string, info fs.FileInfo, err error) error { if err != nil { return nil // ignore errors walking in file system } @@ -441,7 +442,7 @@ func checkMod(mod module.Version) { } data, err := renameio.ReadFile(ziphash) if err != nil { - if errors.Is(err, os.ErrNotExist) { + if errors.Is(err, fs.ErrNotExist) { // This can happen if someone does rm -rf GOPATH/src/cache/download. So it goes. return } -- cgit v1.2.1 From 1b09d430678d4a6f73b2443463d11f75851aba8a Mon Sep 17 00:00:00 2001 From: Russ Cox Date: Fri, 16 Oct 2020 00:49:02 -0400 Subject: all: update references to symbols moved from io/ioutil to io The old ioutil references are still valid, but update our code to reflect best practices and get used to the new locations. Code compiled with the bootstrap toolchain (cmd/asm, cmd/dist, cmd/compile, debug/elf) must remain Go 1.4-compatible and is excluded. Also excluded vendored code. For #41190. Change-Id: I6d86f2bf7bc37a9d904b6cee3fe0c7af6d94d5b1 Reviewed-on: https://go-review.googlesource.com/c/go/+/263142 Trust: Russ Cox Run-TryBot: Russ Cox TryBot-Result: Go Bot Reviewed-by: Emmanuel Odeke --- src/cmd/go/internal/modfetch/fetch.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'src/cmd/go/internal/modfetch/fetch.go') diff --git a/src/cmd/go/internal/modfetch/fetch.go b/src/cmd/go/internal/modfetch/fetch.go index 6ff455e89c..40196c4e9a 100644 --- a/src/cmd/go/internal/modfetch/fetch.go +++ b/src/cmd/go/internal/modfetch/fetch.go @@ -461,7 +461,7 @@ func checkMod(mod module.Version) { // goModSum returns the checksum for the go.mod contents. func goModSum(data []byte) (string, error) { return dirhash.Hash1([]string{"go.mod"}, func(string) (io.ReadCloser, error) { - return ioutil.NopCloser(bytes.NewReader(data)), nil + return io.NopCloser(bytes.NewReader(data)), nil }) } -- cgit v1.2.1 From 5cd4390f3853b8d0d2d962f7acdac87c0eba3d77 Mon Sep 17 00:00:00 2001 From: Jay Conrod Date: Tue, 13 Oct 2020 18:19:21 -0400 Subject: cmd/go: don't fetch files missing sums in readonly mode If the go command needs a .mod or .zip file in -mod=readonly mode (now the default), and that file doesn't have a hash in the main module's go.sum file, the go command will now report an error before fetching the file, rather than at the end when failing to update go.sum. The error says specifically which entry is missing. If this error is encountered when loading the build list, it will suggest 'go mod tidy'. If this error is encountered when loading a specific package (an import or command line argument), the error will mention that package and will suggest 'go mod tidy' or 'go get -d'. Fixes #41934 Fixes #41935 Change-Id: I96ec2ef9258bd4bade9915c43d47e6243c376a81 Reviewed-on: https://go-review.googlesource.com/c/go/+/262341 Reviewed-by: Bryan C. Mills Trust: Jay Conrod --- src/cmd/go/internal/modfetch/fetch.go | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) (limited to 'src/cmd/go/internal/modfetch/fetch.go') diff --git a/src/cmd/go/internal/modfetch/fetch.go b/src/cmd/go/internal/modfetch/fetch.go index 40196c4e9a..25e9fb62c1 100644 --- a/src/cmd/go/internal/modfetch/fetch.go +++ b/src/cmd/go/internal/modfetch/fetch.go @@ -428,6 +428,28 @@ func readGoSum(dst map[module.Version][]string, file string, data []byte) error return nil } +// HaveSum returns true if the go.sum file contains an entry for mod. +// The entry's hash must be generated with a known hash algorithm. +// mod.Version may have a "/go.mod" suffix to distinguish sums for +// .mod and .zip files. +func HaveSum(mod module.Version) bool { + goSum.mu.Lock() + defer goSum.mu.Unlock() + inited, err := initGoSum() + if err != nil || !inited { + return false + } + for _, h := range goSum.m[mod] { + if !strings.HasPrefix(h, "h1:") { + continue + } + if !goSum.status[modSum{mod, h}].dirty { + return true + } + } + return false +} + // checkMod checks the given module's checksum. func checkMod(mod module.Version) { if cfg.GOMODCACHE == "" { -- cgit v1.2.1