cmd/dist: save and restore original permissions in makeGOROOTUnwritable

Also log a message and skip the Chmods if running as root.

Updates #30316

Change-Id: Ifb68d06ce845275a72d64c808407e8609df270bc
Reviewed-on: https://go-review.googlesource.com/c/go/+/206757
Run-TryBot: Bryan C. Mills <bcmills@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>

diff --git a/src/cmd/dist/test.go b/src/cmd/dist/test.go
index e0fa51f1469b4fe897bb77a3f210aac21d0c0962..84ad5fd59dc9ae163c4f5a8c9ebd90cf7d012d44 100644 (file)
--- a/src/cmd/dist/test.go
+++ b/src/cmd/dist/test.go
@@ -192,8 +192,13 @@ func (t *tester) run() {
        }
 
        // On a few builders, make GOROOT unwritable to catch tests writing to it.
+       restoreGOROOT := func() {}
        if strings.HasPrefix(os.Getenv("GO_BUILDER_NAME"), "linux-") {
-               t.makeGOROOTUnwritable()
+               if os.Getuid() == 0 {
+                       log.Printf("Not making GOROOT unwritable: running as root, so permissions would have no effect.")
+               } else {
+                       restoreGOROOT = t.makeGOROOTUnwritable()
+               }
        }
 
        for _, dt := range t.tests {
@@ -208,12 +213,15 @@ func (t *tester) run() {
                        if t.keepGoing {
                                log.Printf("Failed: %v", err)
                        } else {
+                               restoreGOROOT()
                                log.Fatalf("Failed: %v", err)
                        }
                }
        }
        t.runPending(nil)
+       restoreGOROOT()
        timelog("end", "dist test")
+
        if t.failed {
                fmt.Println("\nFAILED")
                os.Exit(1)
@@ -1423,32 +1431,45 @@ func (t *tester) packageHasBenchmarks(pkg string) bool {
 
 // makeGOROOTUnwritable makes all $GOROOT files & directories non-writable to
 // check that no tests accidentally write to $GOROOT.
-func (t *tester) makeGOROOTUnwritable() {
-       if os.Getenv("GO_BUILDER_NAME") == "" {
-               panic("not a builder")
-       }
-       if os.Getenv("GOROOT") == "" {
+func (t *tester) makeGOROOTUnwritable() (undo func()) {
+       dir := os.Getenv("GOROOT")
+       if dir == "" {
                panic("GOROOT not set")
        }
-       err := filepath.Walk(os.Getenv("GOROOT"), func(name string, fi os.FileInfo, err error) error {
-               if err != nil {
-                       return err
-               }
-               if !fi.Mode().IsRegular() && !fi.IsDir() {
-                       return nil
+
+       type pathMode struct {
+               path string
+               mode os.FileMode
+       }
+       var dirs []pathMode // in lexical order
+
+       undo = func() {
+               for i := range dirs {
+                       os.Chmod(dirs[i].path, dirs[i].mode) // best effort
                }
-               mode := fi.Mode()
-               newMode := mode & ^os.FileMode(0222)
-               if newMode != mode {
-                       if err := os.Chmod(name, newMode); err != nil {
-                               return err
+       }
+
+       filepath.Walk(dir, func(path string, info os.FileInfo, err error) error {
+               if err == nil {
+                       mode := info.Mode()
+                       if mode&0222 != 0 && (mode.IsDir() || mode.IsRegular()) {
+                               dirs = append(dirs, pathMode{path, mode})
                        }
                }
                return nil
        })
-       if err != nil {
-               log.Fatalf("making builder's files read-only: %v", err)
+
+       // Run over list backward to chmod children before parents.
+       for i := len(dirs) - 1; i >= 0; i-- {
+               err := os.Chmod(dirs[i].path, dirs[i].mode&^0222)
+               if err != nil {
+                       dirs = dirs[i:] // Only undo what we did so far.
+                       undo()
+                       log.Fatalf("failed to make GOROOT read-only: %v", err)
+               }
        }
+
+       return undo
 }
 
 // shouldUsePrecompiledStdTest reports whether "dist test" should use