From 0d25292fbc05a71d21a458cec780e795d294e63e Mon Sep 17 00:00:00 2001 From: zeripath Date: Mon, 14 Nov 2022 02:58:32 +0000 Subject: [PATCH] Prevent panic in doctor command when running default checks (#21791) (#21807) Backport #21791 There was a bug introduced in #21352 due to a change of behaviour caused by #19280. This causes a panic on running the default doctor checks because the panic introduced by #19280 assumes that the only way opts.StdOut and opts.Stderr can be set in RunOpts is deliberately. Unfortunately, when running a git.Command the provided RunOpts can be set, therefore if you share a common set of RunOpts these two values can be set by the previous commands. This PR stops using common RunOpts for the commands in that doctor check but secondly stops RunCommand variants from changing the provided RunOpts. Signed-off-by: Andrew Thornton --- modules/doctor/heads.go | 10 ++++------ modules/git/command.go | 26 ++++++++++++++++++++------ 2 files changed, 24 insertions(+), 12 deletions(-) diff --git a/modules/doctor/heads.go b/modules/doctor/heads.go index 7f3b2a8a02..b1bfd50b20 100644 --- a/modules/doctor/heads.go +++ b/modules/doctor/heads.go @@ -19,11 +19,9 @@ func synchronizeRepoHeads(ctx context.Context, logger log.Logger, autofix bool) numReposUpdated := 0 err := iterateRepositories(ctx, func(repo *repo_model.Repository) error { numRepos++ - runOpts := &git.RunOpts{Dir: repo.RepoPath()} + _, _, defaultBranchErr := git.NewCommand(ctx, "rev-parse").AddDashesAndList(repo.DefaultBranch).RunStdString(&git.RunOpts{Dir: repo.RepoPath()}) - _, _, defaultBranchErr := git.NewCommand(ctx, "rev-parse").AddDashesAndList(repo.DefaultBranch).RunStdString(runOpts) - - head, _, headErr := git.NewCommand(ctx, "symbolic-ref", "--short", "HEAD").RunStdString(runOpts) + head, _, headErr := git.NewCommand(ctx, "symbolic-ref", "--short", "HEAD").RunStdString(&git.RunOpts{Dir: repo.RepoPath()}) // what we expect: default branch is valid, and HEAD points to it if headErr == nil && defaultBranchErr == nil && head == repo.DefaultBranch { @@ -49,7 +47,7 @@ func synchronizeRepoHeads(ctx context.Context, logger log.Logger, autofix bool) } // otherwise, let's try fixing HEAD - err := git.NewCommand(ctx, "symbolic-ref").AddDashesAndList("HEAD", repo.DefaultBranch).Run(runOpts) + err := git.NewCommand(ctx, "symbolic-ref").AddDashesAndList("HEAD", git.BranchPrefix+repo.DefaultBranch).Run(&git.RunOpts{Dir: repo.RepoPath()}) if err != nil { logger.Warn("Failed to fix HEAD for %s/%s: %v", repo.OwnerName, repo.Name, err) return nil @@ -65,7 +63,7 @@ func synchronizeRepoHeads(ctx context.Context, logger log.Logger, autofix bool) logger.Info("Out of %d repos, HEADs for %d are now fixed and HEADS for %d are still broken", numRepos, numReposUpdated, numDefaultBranchesBroken+numHeadsBroken-numReposUpdated) } else { if numHeadsBroken == 0 && numDefaultBranchesBroken == 0 { - logger.Info("All %d repos have their HEADs in the correct state") + logger.Info("All %d repos have their HEADs in the correct state", numRepos) } else { if numHeadsBroken == 0 && numDefaultBranchesBroken != 0 { logger.Critical("Default branches are broken for %d/%d repos", numDefaultBranchesBroken, numRepos) diff --git a/modules/git/command.go b/modules/git/command.go index abf40b0cd7..0d94494f11 100644 --- a/modules/git/command.go +++ b/modules/git/command.go @@ -202,8 +202,11 @@ func (c *Command) Run(opts *RunOpts) error { if opts == nil { opts = &RunOpts{} } - if opts.Timeout <= 0 { - opts.Timeout = defaultCommandExecutionTimeout + + // We must not change the provided options + timeout := opts.Timeout + if timeout <= 0 { + timeout = defaultCommandExecutionTimeout } if len(opts.Dir) == 0 { @@ -238,7 +241,7 @@ func (c *Command) Run(opts *RunOpts) error { if opts.UseContextTimeout { ctx, cancel, finished = process.GetManager().AddContext(c.parentContext, desc) } else { - ctx, cancel, finished = process.GetManager().AddContextTimeout(c.parentContext, opts.Timeout, desc) + ctx, cancel, finished = process.GetManager().AddContextTimeout(c.parentContext, timeout, desc) } defer finished() @@ -339,9 +342,20 @@ func (c *Command) RunStdBytes(opts *RunOpts) (stdout, stderr []byte, runErr RunS } stdoutBuf := &bytes.Buffer{} stderrBuf := &bytes.Buffer{} - opts.Stdout = stdoutBuf - opts.Stderr = stderrBuf - err := c.Run(opts) + + // We must not change the provided options as it could break future calls - therefore make a copy. + newOpts := &RunOpts{ + Env: opts.Env, + Timeout: opts.Timeout, + UseContextTimeout: opts.UseContextTimeout, + Dir: opts.Dir, + Stdout: stdoutBuf, + Stderr: stderrBuf, + Stdin: opts.Stdin, + PipelineFunc: opts.PipelineFunc, + } + + err := c.Run(newOpts) stderr = stderrBuf.Bytes() if err != nil { return nil, stderr, &runStdError{err: err, stderr: bytesToString(stderr)}