From ec19d6751219a30e3422e520b745f58f8e10a8d9 Mon Sep 17 00:00:00 2001 From: Michael Eischer Date: Sun, 23 Mar 2025 20:04:45 +0100 Subject: [PATCH] ui/termstatus: fix race condition in StdioWrapper --- changelog/unreleased/issue-5259 | 16 ++++++++++++++++ internal/ui/termstatus/stdio_wrapper.go | 8 ++++++++ internal/ui/termstatus/stdio_wrapper_test.go | 18 ++++++++++++++++++ 3 files changed, 42 insertions(+) create mode 100644 changelog/unreleased/issue-5259 diff --git a/changelog/unreleased/issue-5259 b/changelog/unreleased/issue-5259 new file mode 100644 index 000000000..bc9af11d4 --- /dev/null +++ b/changelog/unreleased/issue-5259 @@ -0,0 +1,16 @@ +Bugfix: Fix rare crash in command output + +Some commands could in rare cases crash when trying to print status messages +and request retries at the same time. This resulted in an error like the following: + +``` +panic: runtime error: slice bounds out of range [468:156] +[...] +github.com/restic/restic/internal/ui/termstatus.(*lineWriter).Write(...) + /restic/internal/ui/termstatus/stdio_wrapper.go:36 +0x136 +``` + +This has been fixed. + +https://github.com/restic/restic/issues/5259 +https://github.com/restic/restic/pull/5300 diff --git a/internal/ui/termstatus/stdio_wrapper.go b/internal/ui/termstatus/stdio_wrapper.go index 233610ba3..4909c0fa3 100644 --- a/internal/ui/termstatus/stdio_wrapper.go +++ b/internal/ui/termstatus/stdio_wrapper.go @@ -3,6 +3,7 @@ package termstatus import ( "bytes" "io" + "sync" ) // WrapStdio returns line-buffering replacements for os.Stdout and os.Stderr. @@ -12,6 +13,7 @@ func WrapStdio(term *Terminal) (stdout, stderr io.WriteCloser) { } type lineWriter struct { + m sync.Mutex buf bytes.Buffer print func(string) } @@ -23,6 +25,9 @@ func newLineWriter(print func(string)) *lineWriter { } func (w *lineWriter) Write(data []byte) (n int, err error) { + w.m.Lock() + defer w.m.Unlock() + n, err = w.buf.Write(data) if err != nil { return n, err @@ -40,6 +45,9 @@ func (w *lineWriter) Write(data []byte) (n int, err error) { } func (w *lineWriter) Close() error { + w.m.Lock() + defer w.m.Unlock() + if w.buf.Len() > 0 { w.print(string(append(w.buf.Bytes(), '\n'))) } diff --git a/internal/ui/termstatus/stdio_wrapper_test.go b/internal/ui/termstatus/stdio_wrapper_test.go index 1e214f1f4..12ec1b846 100644 --- a/internal/ui/termstatus/stdio_wrapper_test.go +++ b/internal/ui/termstatus/stdio_wrapper_test.go @@ -1,10 +1,13 @@ package termstatus import ( + "context" "strings" "testing" "github.com/google/go-cmp/cmp" + rtest "github.com/restic/restic/internal/test" + "golang.org/x/sync/errgroup" ) func TestStdioWrapper(t *testing.T) { @@ -82,3 +85,18 @@ func TestStdioWrapper(t *testing.T) { }) } } + +func TestStdioWrapperConcurrentWrites(t *testing.T) { + // tests for race conditions when run with `go test -race ./internal/ui/termstatus` + w := newLineWriter(func(_ string) {}) + + wg, _ := errgroup.WithContext(context.TODO()) + for range 5 { + wg.Go(func() error { + _, err := w.Write([]byte("test\n")) + return err + }) + } + rtest.OK(t, wg.Wait()) + rtest.OK(t, w.Close()) +}