From 6768df2770b9bb69aae5256e3c0b526ab5af25d7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Tue, 15 Dec 2020 01:04:04 +0100 Subject: [PATCH 1/3] Do not use unsafe "delete" without Change the only users of the updateref.Delete() method (aside from its own tests) to use the safer "Update" instead. Why not teach Delete() to also take ? The next commit will do that & change these to use *.Delete() back, but this refactoring was easier to do atomically this way. --- internal/git/updateref/updateref_test.go | 4 +++- .../gitaly/service/cleanup/internalrefs/cleaner.go | 10 ++++++++-- internal/gitaly/service/ref/delete_refs.go | 6 +++++- 3 files changed, 16 insertions(+), 4 deletions(-) diff --git a/internal/git/updateref/updateref_test.go b/internal/git/updateref/updateref_test.go index 270c04de340..ba6074a2754 100644 --- a/internal/git/updateref/updateref_test.go +++ b/internal/git/updateref/updateref_test.go @@ -105,7 +105,9 @@ func TestDelete(t *testing.T) { ref := "refs/heads/feature" - require.NoError(t, updater.Delete(ref)) + refValue, err := git.NewRepository(testRepo).GetReference(ctx, ref) + require.NoError(t, err) + require.NoError(t, updater.Delete(ref, refValue.Target)) require.NoError(t, updater.Wait()) // check the ref was removed diff --git a/internal/gitaly/service/cleanup/internalrefs/cleaner.go b/internal/gitaly/service/cleanup/internalrefs/cleaner.go index 8e75649da86..6479c37786e 100644 --- a/internal/gitaly/service/cleanup/internalrefs/cleaner.go +++ b/internal/gitaly/service/cleanup/internalrefs/cleaner.go @@ -10,6 +10,7 @@ import ( "github.com/grpc-ecosystem/go-grpc-middleware/logging/logrus/ctxlogrus" log "github.com/sirupsen/logrus" "gitlab.com/gitlab-org/gitaly/internal/git" + "gitlab.com/gitlab-org/gitaly/internal/git/repository" "gitlab.com/gitlab-org/gitaly/internal/git/updateref" "gitlab.com/gitlab-org/gitaly/proto/go/gitalypb" ) @@ -32,6 +33,7 @@ type ForEachFunc func(ctx context.Context, oldOID, newOID string, isInternalRef // references pointing to a commit that has been rewritten will simply be // removed. type Cleaner struct { + repo repository.GitRepo ctx context.Context forEach ForEachFunc @@ -57,7 +59,7 @@ func NewCleaner(ctx context.Context, repo *gitalypb.Repository, forEach ForEachF return nil, err } - return &Cleaner{ctx: ctx, table: table, updater: updater, forEach: forEach}, nil + return &Cleaner{repo: repo, ctx: ctx, table: table, updater: updater, forEach: forEach}, nil } // ApplyObjectMap processes an object map file generated by git filter-repo, or @@ -117,7 +119,11 @@ func (c *Cleaner) processEntry(ctx context.Context, oldSHA, newSHA string) error // Remove the internal refs pointing to oldSHA for _, ref := range refs { - if err := c.updater.Delete(ref); err != nil { + refValue, err := git.NewRepository(c.repo).GetReference(c.ctx, ref) + if err != nil { + return err + } + if err := c.updater.Update(ref, git.NullSHA, refValue.Target); err != nil { return err } } diff --git a/internal/gitaly/service/ref/delete_refs.go b/internal/gitaly/service/ref/delete_refs.go index 6444c5743a0..838d22699e9 100644 --- a/internal/gitaly/service/ref/delete_refs.go +++ b/internal/gitaly/service/ref/delete_refs.go @@ -34,7 +34,11 @@ func (s *server) DeleteRefs(ctx context.Context, in *gitalypb.DeleteRefsRequest) } for _, ref := range refnames { - if err := updater.Delete(ref); err != nil { + refValue, err := git.NewRepository(in.Repository).GetReference(ctx, ref) + if err != nil { + return &gitalypb.DeleteRefsResponse{GitError: err.Error()}, nil + } + if err := updater.Update(ref, git.NullSHA, refValue.Target); err != nil { return &gitalypb.DeleteRefsResponse{GitError: err.Error()}, nil } } -- GitLab From c4ee34eb64b613305fe92811d40b048708837315 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Tue, 15 Dec 2020 01:08:38 +0100 Subject: [PATCH 2/3] Move now-safe Update() deletion use to now-safe Delete() Change the two users of Update(ref, , ) to use the shorter form of Delete(ref, ) instead, which is now just as safe since we're making it take a mandatory oldvalue after the last commit's refactoring. --- internal/git/updateref/updateref.go | 4 ++-- internal/gitaly/service/cleanup/internalrefs/cleaner.go | 2 +- internal/gitaly/service/ref/delete_refs.go | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/internal/git/updateref/updateref.go b/internal/git/updateref/updateref.go index 7230acc05e4..c141b4eacfa 100644 --- a/internal/git/updateref/updateref.go +++ b/internal/git/updateref/updateref.go @@ -79,8 +79,8 @@ func (u *Updater) Update(ref, newvalue, oldvalue string) error { } // Delete commands the reference to be removed from the repository -func (u *Updater) Delete(ref string) error { - _, err := fmt.Fprintf(u.cmd, "delete %s\x00\x00", ref) +func (u *Updater) Delete(ref string, oldvalue string) error { + _, err := fmt.Fprintf(u.cmd, "delete %s\x00%s\x00", ref, oldvalue) return err } diff --git a/internal/gitaly/service/cleanup/internalrefs/cleaner.go b/internal/gitaly/service/cleanup/internalrefs/cleaner.go index 6479c37786e..b9c17ed8d4c 100644 --- a/internal/gitaly/service/cleanup/internalrefs/cleaner.go +++ b/internal/gitaly/service/cleanup/internalrefs/cleaner.go @@ -123,7 +123,7 @@ func (c *Cleaner) processEntry(ctx context.Context, oldSHA, newSHA string) error if err != nil { return err } - if err := c.updater.Update(ref, git.NullSHA, refValue.Target); err != nil { + if err := c.updater.Delete(ref, refValue.Target); err != nil { return err } } diff --git a/internal/gitaly/service/ref/delete_refs.go b/internal/gitaly/service/ref/delete_refs.go index 838d22699e9..6c5e196e784 100644 --- a/internal/gitaly/service/ref/delete_refs.go +++ b/internal/gitaly/service/ref/delete_refs.go @@ -38,7 +38,7 @@ func (s *server) DeleteRefs(ctx context.Context, in *gitalypb.DeleteRefsRequest) if err != nil { return &gitalypb.DeleteRefsResponse{GitError: err.Error()}, nil } - if err := updater.Update(ref, git.NullSHA, refValue.Target); err != nil { + if err := updater.Delete(ref, refValue.Target); err != nil { return &gitalypb.DeleteRefsResponse{GitError: err.Error()}, nil } } -- GitLab From aec19f1a1dc88c2bbaeebdd5f255a19f24620fc0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Mon, 14 Dec 2020 18:23:02 +0100 Subject: [PATCH 3/3] Updater: dispatch to update-ref create/delete/update Instead of always issuing "update" commands, use the "create" and "delete" verbs too. This doesn't make any difference in the result as git does the same dispatching internally, but makes things a bit more obvious if you're tracing the output. --- .../avar-use-update-ref-delete-and-create.yml | 5 +++++ internal/git/updateref/updateref.go | 14 ++++++++++++++ .../gitaly/service/operations/update_with_hooks.go | 2 +- 3 files changed, 20 insertions(+), 1 deletion(-) create mode 100644 changelogs/unreleased/avar-use-update-ref-delete-and-create.yml diff --git a/changelogs/unreleased/avar-use-update-ref-delete-and-create.yml b/changelogs/unreleased/avar-use-update-ref-delete-and-create.yml new file mode 100644 index 00000000000..240c413358c --- /dev/null +++ b/changelogs/unreleased/avar-use-update-ref-delete-and-create.yml @@ -0,0 +1,5 @@ +--- +title: 'Cleaner & DeleteRefs: use safer ''update-ref delete'' with argument' +merge_request: 2929 +author: +type: fixed diff --git a/internal/git/updateref/updateref.go b/internal/git/updateref/updateref.go index c141b4eacfa..f820986262e 100644 --- a/internal/git/updateref/updateref.go +++ b/internal/git/updateref/updateref.go @@ -65,6 +65,20 @@ func New(ctx context.Context, repo repository.GitRepo, opts ...UpdaterOpt) (*Upd return &Updater{repo: repo, cmd: cmd}, nil } +// CreateUpdateDelete dispatches to Create()/Update()/Delete() as needed +// +// The underlying "update-ref" does the same dispatching depending on +// what it get passed. +func (u *Updater) CreateUpdateDelete(ref, newvalue, oldvalue string) error { + if newvalue == git.NullSHA { + return u.Delete(ref, oldvalue) + } else if oldvalue == git.NullSHA { + return u.Create(ref, newvalue) + } else { + return u.Update(ref, newvalue, oldvalue) + } +} + // Create commands the reference to be created with the sha specified in value func (u *Updater) Create(ref, value string) error { _, err := fmt.Fprintf(u.cmd, "create %s\x00%s\x00", ref, value) diff --git a/internal/gitaly/service/operations/update_with_hooks.go b/internal/gitaly/service/operations/update_with_hooks.go index ba90634bc46..937b00f4011 100644 --- a/internal/gitaly/service/operations/update_with_hooks.go +++ b/internal/gitaly/service/operations/update_with_hooks.go @@ -80,7 +80,7 @@ func (s *Server) updateReferenceWithHooks(ctx context.Context, repo *gitalypb.Re return err } - if err := updater.Update(reference, newrev, oldrev); err != nil { + if err := updater.CreateUpdateDelete(reference, newrev, oldrev); err != nil { return err } -- GitLab