From db6b66edc87fcbb2cead03ed693ea8073536d622 Mon Sep 17 00:00:00 2001 From: "Bryan C. Mills" Date: Wed, 6 Nov 2019 11:42:29 -0500 Subject: [PATCH] cmd/go: use lockedfile instead of renameio for go.mod and go.sum files This change is based on the previous discussion in CL 202442. Fixes #34634 Change-Id: I1319aa26d5cfcd034bc576555787b3ca79968c38 Reviewed-on: https://go-review.googlesource.com/c/go/+/205637 Run-TryBot: Bryan C. Mills Reviewed-by: Jay Conrod --- src/cmd/go/internal/lockedfile/lockedfile.go | 65 ++++++++++++++ src/cmd/go/internal/modcmd/edit.go | 23 +++-- src/cmd/go/internal/modfetch/cache.go | 19 ++--- src/cmd/go/internal/modfetch/fetch.go | 84 ++++++++----------- src/cmd/go/internal/modload/init.go | 50 ++++++----- .../go/testdata/script/mod_permissions.txt | 57 +++++++++++++ 6 files changed, 209 insertions(+), 89 deletions(-) create mode 100644 src/cmd/go/testdata/script/mod_permissions.txt diff --git a/src/cmd/go/internal/lockedfile/lockedfile.go b/src/cmd/go/internal/lockedfile/lockedfile.go index bb184b1085..59b2dba44c 100644 --- a/src/cmd/go/internal/lockedfile/lockedfile.go +++ b/src/cmd/go/internal/lockedfile/lockedfile.go @@ -120,3 +120,68 @@ func Write(name string, content io.Reader, perm os.FileMode) (err error) { } return err } + +// Transform invokes t with the result of reading the named file, with its lock +// still held. +// +// If t returns a nil error, Transform then writes the returned contents back to +// the file, making a best effort to preserve existing contents on error. +// +// t must not modify the slice passed to it. +func Transform(name string, t func([]byte) ([]byte, error)) (err error) { + f, err := Edit(name) + if err != nil { + return err + } + defer f.Close() + + old, err := ioutil.ReadAll(f) + if err != nil { + return err + } + + new, err := t(old) + if err != nil { + return err + } + + if len(new) > len(old) { + // The overall file size is increasing, so write the tail first: if we're + // about to run out of space on the disk, we would rather detect that + // failure before we have overwritten the original contents. + if _, err := f.WriteAt(new[len(old):], int64(len(old))); err != nil { + // Make a best effort to remove the incomplete tail. + f.Truncate(int64(len(old))) + return err + } + } + + // We're about to overwrite the old contents. In case of failure, make a best + // effort to roll back before we close the file. + defer func() { + if err != nil { + if _, err := f.WriteAt(old, 0); err == nil { + f.Truncate(int64(len(old))) + } + } + }() + + if len(new) >= len(old) { + if _, err := f.WriteAt(new[:len(old)], 0); err != nil { + return err + } + } else { + if _, err := f.WriteAt(new, 0); err != nil { + return err + } + // The overall file size is decreasing, so shrink the file to its final size + // after writing. We do this after writing (instead of before) so that if + // the write fails, enough filesystem space will likely still be reserved + // to contain the previous contents. + if err := f.Truncate(int64(len(new))); err != nil { + return err + } + } + + return nil +} diff --git a/src/cmd/go/internal/modcmd/edit.go b/src/cmd/go/internal/modcmd/edit.go index 2a52f55404..ae8966bab1 100644 --- a/src/cmd/go/internal/modcmd/edit.go +++ b/src/cmd/go/internal/modcmd/edit.go @@ -9,12 +9,13 @@ package modcmd import ( "bytes" "encoding/json" + "errors" "fmt" - "io/ioutil" "os" "strings" "cmd/go/internal/base" + "cmd/go/internal/lockedfile" "cmd/go/internal/modfetch" "cmd/go/internal/modload" "cmd/go/internal/work" @@ -174,7 +175,7 @@ func runEdit(cmd *base.Command, args []string) { } } - data, err := ioutil.ReadFile(gomod) + data, err := lockedfile.Read(gomod) if err != nil { base.Fatalf("go: %v", err) } @@ -217,13 +218,19 @@ func runEdit(cmd *base.Command, args []string) { return } - unlock := modfetch.SideLock() - defer unlock() - lockedData, err := ioutil.ReadFile(gomod) - if err == nil && !bytes.Equal(lockedData, data) { - base.Fatalf("go: go.mod changed during editing; not overwriting") + // Make a best-effort attempt to acquire the side lock, only to exclude + // previous versions of the 'go' command from making simultaneous edits. + if unlock, err := modfetch.SideLock(); err == nil { + defer unlock() } - if err := ioutil.WriteFile(gomod, out, 0666); err != nil { + + err = lockedfile.Transform(gomod, func(lockedData []byte) ([]byte, error) { + if !bytes.Equal(lockedData, data) { + return nil, errors.New("go.mod changed during editing; not overwriting") + } + return out, nil + }) + if err != nil { base.Fatalf("go: %v", err) } } diff --git a/src/cmd/go/internal/modfetch/cache.go b/src/cmd/go/internal/modfetch/cache.go index 8d2bac5623..104fce86dd 100644 --- a/src/cmd/go/internal/modfetch/cache.go +++ b/src/cmd/go/internal/modfetch/cache.go @@ -95,22 +95,21 @@ func lockVersion(mod module.Version) (unlock func(), err error) { return lockedfile.MutexAt(path).Lock() } -// SideLock locks a file within the module cache that that guards edits to files -// outside the cache, such as go.sum and go.mod files in the user's working -// directory. It returns a function that must be called to unlock the file. -func SideLock() (unlock func()) { +// SideLock locks a file within the module cache that that previously guarded +// edits to files outside the cache, such as go.sum and go.mod files in the +// user's working directory. +// If err is nil, the caller MUST eventually call the unlock function. +func SideLock() (unlock func(), err error) { if PkgMod == "" { base.Fatalf("go: internal error: modfetch.PkgMod not set") } + path := filepath.Join(PkgMod, "cache", "lock") if err := os.MkdirAll(filepath.Dir(path), 0777); err != nil { - base.Fatalf("go: failed to create cache directory %s: %v", filepath.Dir(path), err) - } - unlock, err := lockedfile.MutexAt(path).Lock() - if err != nil { - base.Fatalf("go: failed to lock file at %v", path) + return nil, fmt.Errorf("failed to create cache directory: %w", err) } - return unlock + + return lockedfile.MutexAt(path).Lock() } // A cachingRepo is a cache around an underlying Repo, diff --git a/src/cmd/go/internal/modfetch/fetch.go b/src/cmd/go/internal/modfetch/fetch.go index 9db5d137d4..035bddca7a 100644 --- a/src/cmd/go/internal/modfetch/fetch.go +++ b/src/cmd/go/internal/modfetch/fetch.go @@ -19,6 +19,7 @@ import ( "cmd/go/internal/base" "cmd/go/internal/cfg" + "cmd/go/internal/lockedfile" "cmd/go/internal/par" "cmd/go/internal/renameio" @@ -296,7 +297,7 @@ func initGoSum() (bool, error) { goSum.m = make(map[module.Version][]string) goSum.checked = make(map[modSum]bool) - data, err := renameio.ReadFile(GoSumFile) + data, err := lockedfile.Read(GoSumFile) if err != nil && !os.IsNotExist(err) { return false, err } @@ -529,60 +530,45 @@ func WriteGoSum() { base.Fatalf("go: updates to go.sum needed, disabled by -mod=readonly") } - // We want to avoid races between creating the lockfile and deleting it, but - // we also don't want to leave a permanent lockfile in the user's repository. - // - // On top of that, if we crash while writing go.sum, we don't want to lose the - // sums that were already present in the file, so it's important that we write - // the file by renaming rather than truncating — which means that we can't - // lock the go.sum file itself. - // - // Instead, we'll lock a distinguished file in the cache directory: that will - // only race if the user runs `go clean -modcache` concurrently with a command - // that updates go.sum, and that's already racy to begin with. - // - // We'll end up slightly over-synchronizing go.sum writes if the user runs a - // bunch of go commands that update sums in separate modules simultaneously, - // but that's unlikely to matter in practice. - - unlock := SideLock() - defer unlock() + // Make a best-effort attempt to acquire the side lock, only to exclude + // previous versions of the 'go' command from making simultaneous edits. + if unlock, err := SideLock(); err == nil { + defer unlock() + } - if !goSum.overwrite { - // Re-read the go.sum file to incorporate any sums added by other processes - // in the meantime. - data, err := renameio.ReadFile(GoSumFile) - if err != nil && !os.IsNotExist(err) { - base.Fatalf("go: re-reading go.sum: %v", err) + err := lockedfile.Transform(GoSumFile, func(data []byte) ([]byte, error) { + if !goSum.overwrite { + // Incorporate any sums added by other processes in the meantime. + // Add only the sums that we actually checked: the user may have edited or + // truncated the file to remove erroneous hashes, and we shouldn't restore + // them without good reason. + goSum.m = make(map[module.Version][]string, len(goSum.m)) + readGoSum(goSum.m, GoSumFile, data) + for ms := range goSum.checked { + addModSumLocked(ms.mod, ms.sum) + goSum.dirty = true + } } - // Add only the sums that we actually checked: the user may have edited or - // truncated the file to remove erroneous hashes, and we shouldn't restore - // them without good reason. - goSum.m = make(map[module.Version][]string, len(goSum.m)) - readGoSum(goSum.m, GoSumFile, data) - for ms := range goSum.checked { - addModSumLocked(ms.mod, ms.sum) - goSum.dirty = true + var mods []module.Version + for m := range goSum.m { + mods = append(mods, m) } - } - - var mods []module.Version - for m := range goSum.m { - mods = append(mods, m) - } - module.Sort(mods) - var buf bytes.Buffer - for _, m := range mods { - list := goSum.m[m] - sort.Strings(list) - for _, h := range list { - fmt.Fprintf(&buf, "%s %s %s\n", m.Path, m.Version, h) + module.Sort(mods) + + var buf bytes.Buffer + for _, m := range mods { + list := goSum.m[m] + sort.Strings(list) + for _, h := range list { + fmt.Fprintf(&buf, "%s %s %s\n", m.Path, m.Version, h) + } } - } + return buf.Bytes(), nil + }) - if err := renameio.WriteFile(GoSumFile, buf.Bytes(), 0666); err != nil { - base.Fatalf("go: writing go.sum: %v", err) + if err != nil { + base.Fatalf("go: updating go.sum: %v", err) } goSum.checked = make(map[modSum]bool) diff --git a/src/cmd/go/internal/modload/init.go b/src/cmd/go/internal/modload/init.go index cbf3b0575a..26e482c9d7 100644 --- a/src/cmd/go/internal/modload/init.go +++ b/src/cmd/go/internal/modload/init.go @@ -7,6 +7,7 @@ package modload import ( "bytes" "encoding/json" + "errors" "fmt" "go/build" "internal/lazyregexp" @@ -22,6 +23,7 @@ import ( "cmd/go/internal/cache" "cmd/go/internal/cfg" "cmd/go/internal/load" + "cmd/go/internal/lockedfile" "cmd/go/internal/modconv" "cmd/go/internal/modfetch" "cmd/go/internal/modfetch/codehost" @@ -950,32 +952,36 @@ func WriteGoMod() { index = indexModFile(new, modFile, false) }() - unlock := modfetch.SideLock() - defer unlock() - - file := ModFilePath() - old, err := renameio.ReadFile(file) - if bytes.Equal(old, new) { - // The go.mod file is already equal to new, possibly as the result of some - // other process. - return + // Make a best-effort attempt to acquire the side lock, only to exclude + // previous versions of the 'go' command from making simultaneous edits. + if unlock, err := modfetch.SideLock(); err == nil { + defer unlock() } - if index != nil && !bytes.Equal(old, index.data) { - if err != nil { - base.Fatalf("go: can't determine whether go.mod has changed: %v", err) + errNoChange := errors.New("no update needed") + + err = lockedfile.Transform(ModFilePath(), func(old []byte) ([]byte, error) { + if bytes.Equal(old, new) { + // The go.mod file is already equal to new, possibly as the result of some + // other process. + return nil, errNoChange } - // The contents of the go.mod file have changed. In theory we could add all - // of the new modules to the build list, recompute, and check whether any - // module in *our* build list got bumped to a different version, but that's - // a lot of work for marginal benefit. Instead, fail the command: if users - // want to run concurrent commands, they need to start with a complete, - // consistent module definition. - base.Fatalf("go: updates to go.mod needed, but contents have changed") - } - if err := renameio.WriteFile(file, new, 0666); err != nil { - base.Fatalf("error writing go.mod: %v", err) + if index != nil && !bytes.Equal(old, index.data) { + // The contents of the go.mod file have changed. In theory we could add all + // of the new modules to the build list, recompute, and check whether any + // module in *our* build list got bumped to a different version, but that's + // a lot of work for marginal benefit. Instead, fail the command: if users + // want to run concurrent commands, they need to start with a complete, + // consistent module definition. + return nil, fmt.Errorf("existing contents have changed since last read") + } + + return new, nil + }) + + if err != nil && err != errNoChange { + base.Fatalf("go: updating go.mod: %v", err) } } diff --git a/src/cmd/go/testdata/script/mod_permissions.txt b/src/cmd/go/testdata/script/mod_permissions.txt new file mode 100644 index 0000000000..11fb4754f8 --- /dev/null +++ b/src/cmd/go/testdata/script/mod_permissions.txt @@ -0,0 +1,57 @@ +# Regression test for golang.org/issue/34634: permissions for the go.sum and +# go.mod files should be preserved when overwriting them. + +env GO111MODULE=on +[short] skip + +# Skip platforms that do not have Unix-style file permissions. +[windows] skip +[plan9] skip + +chmod 0640 go.mod +chmod 0604 go.sum +go mod edit -module=golang.org/issue/34634 + +go build . +cmp go.mod go.mod.want +cmp go.sum go.sum.want + +go run . +stdout 'go.mod: 0640' +stdout 'go.sum: 0604' + +-- read_perm.go -- +package main + +import ( + "fmt" + "os" + _ "rsc.io/sampler" +) + +func main() { + for _, name := range []string{"go.mod", "go.sum"} { + fi, err := os.Stat(name) + if err != nil { + fmt.Fprintf(os.Stderr, "%s: %v\n", err) + continue + } + fmt.Printf("%s: 0%o\n", name, fi.Mode().Perm()) + } +} +-- go.mod -- +module TODO + +go 1.14 +-- go.sum -- +-- go.mod.want -- +module golang.org/issue/34634 + +go 1.14 + +require rsc.io/sampler v1.99.99 +-- go.sum.want -- +golang.org/x/text v0.0.0-20170915032832-14c0d48ead0c h1:pvCbr/wm8HzDD3fVywevekufpn6tCGPY3spdHeZJEsw= +golang.org/x/text v0.0.0-20170915032832-14c0d48ead0c/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ= +rsc.io/sampler v1.99.99 h1:iMG9lbEG/8MdeR4lgL+Q8IcwbLNw7ijW7fTiK8Miqts= +rsc.io/sampler v1.99.99/go.mod h1:T1hPZKmBbMNahiBKFy5HrXp6adAjACjK9JXDnKaTXpA= -- 2.50.0