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 <[email protected]>
Reviewed-by: Jay Conrod <[email protected]>

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
+}

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)
 	}
 }

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,

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)

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)
 	}
 }
 

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=