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 <bcmills@google.com>
Reviewed-by: Jay Conrod <jayconrod@google.com>
}
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
+}
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"
}
}
- data, err := ioutil.ReadFile(gomod)
+ data, err := lockedfile.Read(gomod)
if err != nil {
base.Fatalf("go: %v", err)
}
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)
}
}
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,
"cmd/go/internal/base"
"cmd/go/internal/cfg"
+ "cmd/go/internal/lockedfile"
"cmd/go/internal/par"
"cmd/go/internal/renameio"
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
}
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)
import (
"bytes"
"encoding/json"
+ "errors"
"fmt"
"go/build"
"internal/lazyregexp"
"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"
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)
}
}
--- /dev/null
+# 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=