Skip to content

Commit

Permalink
Disable possible git concurrency during fsck
Browse files Browse the repository at this point in the history
By using the default queue some git operations might still have been
running when gopass did attempt to update exported keys. Disabling
the queue in the inner loop makes more sense since it's more predictable
and less brittle and using the queue there wouldn't help much anyway.

Fixes gopasspw#2459

RELEASE_NOTES=[BUGFIX] Fix possible concurrency issues in fsck.

Signed-off-by: Dominik Schulz <dominik.schulz@gauner.org>
  • Loading branch information
dominikschulz committed Dec 23, 2022
1 parent 310159b commit ea78b1e
Show file tree
Hide file tree
Showing 2 changed files with 30 additions and 11 deletions.
5 changes: 3 additions & 2 deletions internal/queue/background.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,15 +29,16 @@ type Queuer interface {
Idle(time.Duration) error
}

// WithQueue adds the given queue to the context.
// WithQueue adds the given queue to the context. Add a nil
// queue to disable queuing in this context.
func WithQueue(ctx context.Context, q *Queue) context.Context {
return context.WithValue(ctx, ctxKeyQueue, q)
}

// GetQueue returns an existing queue from the context or
// returns a noop one.
func GetQueue(ctx context.Context) Queuer {
if q, ok := ctx.Value(ctxKeyQueue).(*Queue); ok {
if q, ok := ctx.Value(ctxKeyQueue).(*Queue); ok && q != nil {
return q
}

Expand Down
36 changes: 27 additions & 9 deletions internal/store/leaf/fsck.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
"github.com/gopasspw/gopass/internal/backend"
"github.com/gopasspw/gopass/internal/diff"
"github.com/gopasspw/gopass/internal/out"
"github.com/gopasspw/gopass/internal/queue"
"github.com/gopasspw/gopass/internal/store"
"github.com/gopasspw/gopass/pkg/ctxutil"
"github.com/gopasspw/gopass/pkg/debug"
Expand All @@ -33,26 +34,49 @@ func (s *Store) Fsck(ctx context.Context, path string) error {
return fmt.Errorf("storage backend compaction failed: %w", err)
}

pcb := ctxutil.GetProgressCallback(ctx)

// then we'll make sure all the secrets are readable by us and every
// valid recipient
if path != "" {
out.Printf(ctx, "Checking all secrets matching %s", path)
}

if err := s.fsckLoop(ctx, path); err != nil {
return err
}

if err := s.storage.Push(ctx, "", ""); err != nil {
if !errors.Is(err, store.ErrGitNoRemote) {
out.Printf(ctx, "RCS Push failed: %s", err)
}
}

return nil
}

func (s *Store) fsckLoop(ctx context.Context, path string) error {
pcb := ctxutil.GetProgressCallback(ctx)

// disable network ops, we will push at the end. pushing on possibly
// every single secret could be terribly slow.
ctx = ctxutil.WithNoNetwork(ctx, true)

// disable the queue, for batch operations this is not necessary / wanted
// since different git processes might step onto each others toes.
ctx = queue.WithQueue(ctx, nil)

names, err := s.List(ctx, path)
if err != nil {
return fmt.Errorf("failed to list entries for %s: %w", path, err)
}
debug.Log("names (%d): %q", len(names), names)
sort.Strings(names)

for _, name := range names {
pcb()
if strings.HasPrefix(name, s.alias+"/") {
name = strings.TrimPrefix(name, s.alias+"/")
}
ctx := ctxutil.WithNoNetwork(ctx, true)

debug.Log("[%s] Checking %s", path, name)

if err := s.fsckCheckEntry(ctx, name); err != nil {
Expand All @@ -64,12 +88,6 @@ func (s *Store) Fsck(ctx context.Context, path string) error {
out.Errorf(ctx, "Failed to update public keys: %s", err)
}

if err := s.storage.Push(ctx, "", ""); err != nil {
if !errors.Is(err, store.ErrGitNoRemote) {
out.Printf(ctx, "RCS Push failed: %s", err)
}
}

return nil
}

Expand Down

0 comments on commit ea78b1e

Please sign in to comment.