Prevent nil panics in housekeeping.Perform

6 files changed, 52 insertions, 15 deletions

diff --git a/changelogs/unreleased/jv-walker-nil-checks.yml b/changelogs/unreleased/jv-walker-nil-checks.yml
new file mode 100644
index 000000000..9329f746e
--- /dev/null
+++ b/changelogs/unreleased/jv-walker-nil-checks.yml
@@ -0,0 +1,5 @@
+---
+title: Prevent nil panics in housekeeping.Perform
+merge_request: 1492
+author:
+type: fixed
diff --git a/internal/helper/housekeeping/housekeeping.go b/internal/helper/housekeeping/housekeeping.go
index 5d05b0c06..41ee05247 100644
--- a/internal/helper/housekeeping/housekeeping.go
+++ b/internal/helper/housekeeping/housekeeping.go
@@ -15,24 +15,32 @@ const deleteTempFilesOlderThanDuration = 7 * 24 * time.Hour
 
 // Perform will perform housekeeping duties on a repository
 func Perform(ctx context.Context, repoPath string) error {
-	logger := grpc_logrus.Extract(ctx).WithField("system", "housekeeping")
+	logger := myLogger(ctx)
 
 	filesMarkedForRemoval := 0
 	unremovableFiles := 0
 
-	err := filepath.Walk(repoPath, func(path string, info os.FileInfo, _ error) error {
+	err := filepath.Walk(repoPath, func(path string, info os.FileInfo, err error) error {
+		if info == nil {
+			logger.WithFields(log.Fields{
+				"path": path,
+			}).WithError(err).Error("nil FileInfo in housekeeping.Perform")
+
+			return nil
+		}
+
 		if repoPath == path {
 			// Never consider the root path
 			return nil
 		}
 
-		if info == nil || !shouldRemove(path, info.ModTime(), info.Mode()) {
+		if !shouldRemove(path, info.ModTime(), info.Mode()) {
 			return nil
 		}
 
 		filesMarkedForRemoval++
 
-		if err := forceRemove(path); err != nil {
+		if err := forceRemove(ctx, path); err != nil {
 			unremovableFiles++
 			logger.WithError(err).WithField("path", path).Warn("unable to remove stray file")
 		}
@@ -55,18 +63,31 @@ func Perform(ctx context.Context, repoPath string) error {
 	return err
 }
 
+func myLogger(ctx context.Context) *log.Entry {
+	return grpc_logrus.Extract(ctx).WithField("system", "housekeeping")
+}
+
 // FixDirectoryPermissions does a recursive directory walk to look for
 // directories that cannot be accessed by the current user, and tries to
 // fix those with chmod. The motivating problem is that directories with mode
 // 0 break os.RemoveAll.
-func FixDirectoryPermissions(path string) error {
-	return fixDirectoryPermissions(path, make(map[string]struct{}))
+func FixDirectoryPermissions(ctx context.Context, path string) error {
+	return fixDirectoryPermissions(ctx, path, make(map[string]struct{}))
 }
 
 const minimumDirPerm = 0700
 
-func fixDirectoryPermissions(path string, retriedPaths map[string]struct{}) error {
+func fixDirectoryPermissions(ctx context.Context, path string, retriedPaths map[string]struct{}) error {
+	logger := myLogger(ctx)
 	return filepath.Walk(path, func(path string, info os.FileInfo, errIncoming error) error {
+		if info == nil {
+			logger.WithFields(log.Fields{
+				"path": path,
+			}).WithError(errIncoming).Error("nil FileInfo in housekeeping.fixDirectoryPermissions")
+
+			return nil
+		}
+
 		if !info.IsDir() || info.Mode()&minimumDirPerm == minimumDirPerm {
 			return nil
 		}
@@ -77,7 +98,7 @@ func fixDirectoryPermissions(path string, retriedPaths map[string]struct{}) erro
 
 		if _, retried := retriedPaths[path]; !retried && os.IsPermission(errIncoming) {
 			retriedPaths[path] = struct{}{}
-			return fixDirectoryPermissions(path, retriedPaths)
+			return fixDirectoryPermissions(ctx, path, retriedPaths)
 		}
 
 		return nil
@@ -85,14 +106,14 @@ func fixDirectoryPermissions(path string, retriedPaths map[string]struct{}) erro
 }
 
 // Delete a directory structure while ensuring the current user has permission to delete the directory structure
-func forceRemove(path string) error {
+func forceRemove(ctx context.Context, path string) error {
 	err := os.RemoveAll(path)
 	if err == nil {
 		return nil
 	}
 
 	// Delete failed. Try again after chmod'ing directories recursively
-	if err := FixDirectoryPermissions(path); err != nil {
+	if err := FixDirectoryPermissions(ctx, path); err != nil {
 		return err
 	}
 
diff --git a/internal/helper/housekeeping/housekeeping_test.go b/internal/helper/housekeeping/housekeeping_test.go
index 21c486db5..93f86046e 100644
--- a/internal/helper/housekeeping/housekeeping_test.go
+++ b/internal/helper/housekeeping/housekeeping_test.go
@@ -9,6 +9,8 @@ import (
 	"time"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"gitlab.com/gitlab-org/gitaly/internal/testhelper"
 )
 
 type entryFinalState int
@@ -271,6 +273,12 @@ func TestShouldUnlink(t *testing.T) {
 	}
 }
 
+func TestPerformRepoDoesNotExist(t *testing.T) {
+	ctx, cancel := testhelper.Context()
+	defer cancel()
+	require.NoError(t, Perform(ctx, "/does/not/exist"))
+}
+
 // This test exists only ever for manual testing purposes.
 // Set it up as follows:
 /*
diff --git a/internal/service/repository/rebase_in_progress.go b/internal/service/repository/rebase_in_progress.go
index 563ed6385..d129741e0 100644
--- a/internal/service/repository/rebase_in_progress.go
+++ b/internal/service/repository/rebase_in_progress.go
@@ -31,14 +31,14 @@ func (s *server) IsRebaseInProgress(ctx context.Context, req *gitalypb.IsRebaseI
 		return nil, err
 	}
 
-	inProg, err := freshWorktree(repoPath, rebaseWorktreePrefix, req.GetRebaseId())
+	inProg, err := freshWorktree(ctx, repoPath, rebaseWorktreePrefix, req.GetRebaseId())
 	if err != nil {
 		return nil, err
 	}
 	return &gitalypb.IsRebaseInProgressResponse{InProgress: inProg}, nil
 }
 
-func freshWorktree(repoPath, prefix, id string) (bool, error) {
+func freshWorktree(ctx context.Context, repoPath, prefix, id string) (bool, error) {
 	worktreePath := path.Join(repoPath, worktreePrefix, fmt.Sprintf("%s-%s", prefix, id))
 
 	fs, err := os.Stat(worktreePath)
@@ -48,7 +48,7 @@ func freshWorktree(repoPath, prefix, id string) (bool, error) {
 
 	if time.Since(fs.ModTime()) > freshTimeout {
 		if err = os.RemoveAll(worktreePath); err != nil {
-			if err = housekeeping.FixDirectoryPermissions(worktreePath); err != nil {
+			if err = housekeeping.FixDirectoryPermissions(ctx, worktreePath); err != nil {
 				return false, err
 			}
 			err = os.RemoveAll(worktreePath)
diff --git a/internal/service/repository/squash_in_progress.go b/internal/service/repository/squash_in_progress.go
index e9a1fb889..20e25641a 100644
--- a/internal/service/repository/squash_in_progress.go
+++ b/internal/service/repository/squash_in_progress.go
@@ -25,7 +25,7 @@ func (s *server) IsSquashInProgress(ctx context.Context, req *gitalypb.IsSquashI
 		return nil, err
 	}
 
-	inProg, err := freshWorktree(repoPath, squashWorktreePrefix, req.GetSquashId())
+	inProg, err := freshWorktree(ctx, repoPath, squashWorktreePrefix, req.GetSquashId())
 	if err != nil {
 		return nil, err
 	}
diff --git a/internal/tempdir/tempdir.go b/internal/tempdir/tempdir.go
index b24117726..8c5e4cdce 100644
--- a/internal/tempdir/tempdir.go
+++ b/internal/tempdir/tempdir.go
@@ -118,6 +118,9 @@ func StartCleaning() {
 type invalidCleanRoot string
 
 func clean(dir string) error {
+	ctx, cancel := context.WithCancel(context.Background())
+	defer cancel()
+
 	// If we start "cleaning up" the wrong directory we may delete user data
 	// which is Really Bad.
 	if !strings.HasSuffix(dir, tmpRootPrefix) {
@@ -139,7 +142,7 @@ func clean(dir string) error {
 		}
 
 		fullPath := filepath.Join(dir, info.Name())
-		if err := housekeeping.FixDirectoryPermissions(fullPath); err != nil {
+		if err := housekeeping.FixDirectoryPermissions(ctx, fullPath); err != nil {
 			return err
 		}