Fix directory permission walker for Go 1.10

2 files changed, 28 insertions, 42 deletions

diff --git a/internal/tempdir/tempdir.go b/internal/tempdir/tempdir.go
index 6cff3f8ba..69565c590 100644
--- a/internal/tempdir/tempdir.go
+++ b/internal/tempdir/tempdir.go
@@ -2,17 +2,16 @@ package tempdir
 
 import (
 	"context"
-	"fmt"
 	"io/ioutil"
 	"os"
 	"path"
-	"path/filepath"
 	"strings"
 	"time"
 
 	pb "gitlab.com/gitlab-org/gitaly-proto/go"
 	"gitlab.com/gitlab-org/gitaly/internal/config"
 	"gitlab.com/gitlab-org/gitaly/internal/helper"
+	"gitlab.com/gitlab-org/gitaly/internal/helper/housekeeping"
 
 	log "github.com/sirupsen/logrus"
 )
@@ -22,6 +21,8 @@ const (
 	// directory name that could be provided by a user. The '+' character is
 	// not allowed in GitLab namespaces or repositories.
 	tmpRootPrefix = "+gitaly/tmp"
+
+	maxAge = 7 * 24 * time.Hour
 )
 
 // New returns the path of a new temporary directory for use with the
@@ -76,6 +77,8 @@ func StartCleaning() {
 	}
 }
 
+type invalidCleanRoot string
+
 func clean(dir string) error {
 	// If we start "cleaning up" the wrong directory we may delete user data
 	// which is Really Bad.
@@ -84,37 +87,27 @@ func clean(dir string) error {
 		panic(invalidCleanRoot("invalid tempdir clean root: panicking to prevent data loss"))
 	}
 
-	return filepath.Walk(dir, cleanFunc)
-}
-
-const (
-	maxAge = 7 * 24 * time.Hour
-)
-
-type invalidCleanRoot string
-
-func cleanFunc(path string, info os.FileInfo, errIncoming error) error {
-	if errIncoming != nil && !os.IsNotExist(errIncoming) {
-		return fmt.Errorf("incoming %q: %v", path, errIncoming)
-	}
-
-	if info == nil {
+	entries, err := ioutil.ReadDir(dir)
+	if os.IsNotExist(err) {
 		return nil
 	}
+	if err != nil {
+		return err
+	}
 
-	if perm := info.Mode().Perm(); info.IsDir() && perm&0700 < 0700 {
-		// Fix directory read permissions
-		if err := os.Chmod(path, perm|0700); err != nil {
-			return err
+	for _, info := range entries {
+		if time.Since(info.ModTime()) < maxAge {
+			continue
 		}
-	}
 
-	if time.Since(info.ModTime()) < maxAge {
-		return nil
-	}
+		fullPath := path.Join(dir, info.Name())
+		if err := housekeeping.FixDirectoryPermissions(fullPath); err != nil {
+			return err
+		}
 
-	if err := os.Remove(path); err != nil && !info.IsDir() {
-		return err
+		if err := os.RemoveAll(fullPath); err != nil {
+			return err
+		}
 	}
 
 	return nil
diff --git a/internal/tempdir/tempdir_test.go b/internal/tempdir/tempdir_test.go
index 833292abb..e4dcc3e74 100644
--- a/internal/tempdir/tempdir_test.go
+++ b/internal/tempdir/tempdir_test.go
@@ -74,39 +74,32 @@ func TestCleanSuccess(t *testing.T) {
 	recent := time.Now()
 
 	makeDir(t, "a", old)
+	makeDir(t, "a/b", recent) // Messes up mtime of "a", we fix that below
 	makeDir(t, "c", recent)
 	makeDir(t, "f", old)
 
-	makeFile(t, "a/b", old)
+	makeFile(t, "a/b/g", old)
 	makeFile(t, "c/d", old)
 	makeFile(t, "e", recent)
 
 	// This is really evil and even breaks 'rm -rf'
+	require.NoError(t, chmod("a/b", 0), "apply evil permissions to 'a/b'")
 	require.NoError(t, chmod("a", 0), "apply evil permissions to 'a'")
 
+	require.NoError(t, chtimes("a", old), "reset mtime of 'a'")
+
 	assertEntries(t, "a", "c", "e", "f")
 
 	require.NoError(t, clean(cleanRoot), "walk first pass")
-	// 'a' won't get removed because it's mtime is bumped when 'a/b' is deleted
-	assertEntries(t, "a", "c", "e")
-
-	info, err := stat("a")
-	require.NoError(t, err)
-	require.Equal(t, os.FileMode(0700), info.Mode().Perm(), "permissions of 'a' should have been fixed")
-
-	_, err = stat("a/b")
-	require.True(t, os.IsNotExist(err), "entry 'a/b' should be gone")
-
-	require.NoError(t, clean(cleanRoot), "walk second pass")
-	assertEntries(t, "a", "c", "e")
+	assertEntries(t, "c", "e")
 }
 
 func chmod(p string, mode os.FileMode) error {
 	return os.Chmod(path.Join(cleanRoot, p), mode)
 }
 
-func stat(p string) (os.FileInfo, error) {
-	return os.Stat(path.Join(cleanRoot, p))
+func chtimes(p string, t time.Time) error {
+	return os.Chtimes(path.Join(cleanRoot, p), t, t)
 }
 
 func assertEntries(t *testing.T, entries ...string) {