From 208d7df87758249be8ba9833d714a5a605cf5c6c Mon Sep 17 00:00:00 2001 From: Nick Craig-Wood Date: Mon, 27 Jul 2026 18:21:43 +0100 Subject: [PATCH] vfs: don't crash the process if a backend panics on a background goroutine GHSA-6jcg-q3wp-x2f4 The VFS calls backends from goroutines of its own. A panic on any of these cannot be recovered. So a backend panicking on a single file killed the whole process, taking down a mount or every user of a serve command, even for servers such as serve http whose library recovers panics raised on its own request goroutines. Recover panics at those goroutines and log them with a stack trace. Where the surrounding code already handles a failure, recover around the backend call itself rather than the whole goroutine, so a panicking upload is retried like any other failed upload and a panicking download is reported to the waiters, instead of abandoning the work part way through and leaving the bookkeeping inconsistent. Addresses GHSA-6jcg-q3wp-x2f4 --- vfs/vfs.go | 1 + vfs/vfscache/cache.go | 3 ++ vfs/vfscache/downloaders/downloaders.go | 26 ++++++++++------ vfs/vfscache/item.go | 11 +++++-- vfs/vfscache/writeback/writeback.go | 4 ++- vfs/vfscommon/recover.go | 29 +++++++++++++++++ vfs/vfscommon/recover_test.go | 41 +++++++++++++++++++++++++ vfs/write.go | 21 ++++++++++--- 8 files changed, 118 insertions(+), 18 deletions(-) create mode 100644 vfs/vfscommon/recover.go create mode 100644 vfs/vfscommon/recover_test.go diff --git a/vfs/vfs.go b/vfs/vfs.go index aaf94d9bf..e1a118982 100644 --- a/vfs/vfs.go +++ b/vfs/vfs.go @@ -287,6 +287,7 @@ func New(ctx context.Context, f fs.Fs, opt *vfscommon.Options) *VFS { // refresh the directory cache for all directories func (vfs *VFS) refresh() { + defer vfscommon.RecoverPanic(vfs.f, nil) fs.Debugf(vfs.f, "Refreshing VFS directory cache") err := vfs.root.readDirTree() if err != nil { diff --git a/vfs/vfscache/cache.go b/vfs/vfscache/cache.go index 66852e9ec..eaa17b8a0 100644 --- a/vfs/vfscache/cache.go +++ b/vfs/vfscache/cache.go @@ -789,6 +789,9 @@ func (c *Cache) purgeOverQuota() { // clean empties the cache of stuff if it can func (c *Cache) clean(kicked bool) { + // Recover here rather than in the cleaner goroutine so that a panic + // cleaning one item does not stop the cache being cleaned ever again. + defer vfscommon.RecoverPanic(c.fremote, nil) // Cache may be empty so end _, err := os.Stat(c.root) if os.IsNotExist(err) { diff --git a/vfs/vfscache/downloaders/downloaders.go b/vfs/vfscache/downloaders/downloaders.go index 495fdf2bf..333edc8fc 100644 --- a/vfs/vfscache/downloaders/downloaders.go +++ b/vfs/vfscache/downloaders/downloaders.go @@ -406,6 +406,7 @@ func (dls *Downloaders) _dispatchWaiters() { // Send any waiters which have completed back to their callers and make sure // there is a downloader appropriate for each waiter func (dls *Downloaders) kickWaiters() (err error) { + defer vfscommon.RecoverPanic(dls.src, &err) dls.mu.Lock() defer dls.mu.Unlock() @@ -556,6 +557,7 @@ func (dl *downloader) open(offset int64) (err error) { // close the downloader func (dl *downloader) close(inErr error) (err error) { // defer log.Trace(dl.dls.src, "inErr=%v", err)("err=%v", &err) + defer vfscommon.RecoverPanic(dl.dls.src, &err) checkErr := func(e error) { if e == nil || errors.Is(err, asyncreader.ErrorStreamAbandoned) { return @@ -563,17 +565,20 @@ func (dl *downloader) close(inErr error) (err error) { err = e } dl.mu.Lock() - if dl.in != nil { - checkErr(dl.in.Close()) - dl.in = nil - } - if dl.tr != nil { - dl.tr.Done(dl.dls.ctx, inErr) - dl.tr = nil - } + defer dl.mu.Unlock() + // Mark closed and detach the reader and transfer before closing them: + // closing the reader runs backend code, and if that panics the recover + // above must not leave the downloader locked or looking still open. dl._closed = true - dl.mu.Unlock() - return nil + in, tr := dl.in, dl.tr + dl.in, dl.tr = nil, nil + if in != nil { + checkErr(in.Close()) + } + if tr != nil { + tr.Done(dl.dls.ctx, inErr) + } + return err } // closed returns true if the downloader has been closed already @@ -622,6 +627,7 @@ func (dl *downloader) stopAndClose(inErr error) (err error) { // Start downloading to the local file starting at offset until maxOffset. func (dl *downloader) download() (n int64, err error) { + defer vfscommon.RecoverPanic(dl.dls.src, &err) // defer log.Trace(dl.dls.src, "")("err=%v", &err) n, err = dl.in.WriteTo(dl) if err != nil && !errors.Is(err, asyncreader.ErrorStreamAbandoned) { diff --git a/vfs/vfscache/item.go b/vfs/vfscache/item.go index 7e643c054..475f97513 100644 --- a/vfs/vfscache/item.go +++ b/vfs/vfscache/item.go @@ -17,6 +17,7 @@ import ( "github.com/rclone/rclone/lib/ranges" "github.com/rclone/rclone/vfs/vfscache/downloaders" "github.com/rclone/rclone/vfs/vfscache/writeback" + "github.com/rclone/rclone/vfs/vfscommon" ) // NB as Cache and Item are tightly linked it is necessary to have a @@ -721,6 +722,7 @@ func (item *Item) Close(storeFn StoreFn) (err error) { // Grace period only applies to non-dirty files, so storeFn (only // needed for writeback) and syncWriteBack are not relevant. func (item *Item) closeAfterGrace() { + defer vfscommon.RecoverPanic(item.name, nil) item.mu.Lock() defer item.mu.Unlock() @@ -736,9 +738,14 @@ func (item *Item) closeAfterGrace() { // progress so a concurrent open waits rather than tripping over the // half-closed handle. item.closing = make(chan struct{}) + // Release the waiters however this ends: if a panic in the backend is + // recovered above, leaving item.closing open would block every later + // open of this file forever. + defer func() { + close(item.closing) + item.closing = nil + }() err := item._actualClose(nil, false) - close(item.closing) - item.closing = nil if err != nil { fs.Errorf(item.name, "vfs cache: close after grace period failed: %v", err) } diff --git a/vfs/vfscache/writeback/writeback.go b/vfs/vfscache/writeback/writeback.go index 76acbb446..3e3575158 100644 --- a/vfs/vfscache/writeback/writeback.go +++ b/vfs/vfscache/writeback/writeback.go @@ -352,7 +352,9 @@ func (wb *WriteBack) upload(ctx context.Context, wbItem *writeBackItem) { fs.Debugf(wbItem.name, "vfs cache: starting upload") wb.mu.Unlock() - err := putFn(ctx) + // Recover around the upload itself rather than the whole goroutine, so a + // panicking backend is retried like any other upload failure below. + err := vfscommon.RecoverCall(wbItem.name, func() error { return putFn(ctx) }) wb.mu.Lock() wbItem.cancel() // cancel context to release resources since store done diff --git a/vfs/vfscommon/recover.go b/vfs/vfscommon/recover.go new file mode 100644 index 000000000..1b630c208 --- /dev/null +++ b/vfs/vfscommon/recover.go @@ -0,0 +1,29 @@ +package vfscommon + +import ( + "fmt" + "runtime/debug" + + "github.com/rclone/rclone/fs" +) + +// RecoverPanic recovers a panic, logs it against o with a stack trace and, +// if err is not nil, stores it there. Use it as `defer RecoverPanic(o, &err)`. +func RecoverPanic(o any, err *error) { + if r := recover(); r != nil { + fs.Errorf(o, "panic: %v\n%s", r, debug.Stack()) + if err != nil { + *err = fmt.Errorf("panic: %v", r) + } + } +} + +// RecoverCall calls fn, converting a panic into an error. +// +// Prefer this to recovering a whole goroutine where the caller already +// handles an error, so a panic is retried or reported the same way a failure +// is, rather than abandoning the surrounding work part way through. +func RecoverCall(o any, fn func() error) (err error) { + defer RecoverPanic(o, &err) + return fn() +} diff --git a/vfs/vfscommon/recover_test.go b/vfs/vfscommon/recover_test.go new file mode 100644 index 000000000..01bd7c421 --- /dev/null +++ b/vfs/vfscommon/recover_test.go @@ -0,0 +1,41 @@ +package vfscommon + +import ( + "errors" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestRecoverPanic(t *testing.T) { + // Panic recovered into err + err := func() (err error) { + defer RecoverPanic("test", &err) + panic("boom") + }() + require.Error(t, err) + assert.ErrorContains(t, err, "boom") + + // No panic leaves err alone + err = func() (err error) { + defer RecoverPanic("test", nil) + return nil + }() + assert.NoError(t, err) + + // A nil error target is allowed, for goroutines with nothing to report to + assert.NotPanics(t, func() { + defer RecoverPanic("test", nil) + panic("boom") + }) +} + +func TestRecoverCall(t *testing.T) { + assert.ErrorContains(t, RecoverCall("test", func() error { panic("boom") }), "boom") + + // Ordinary errors and results pass through untouched + errFoo := errors.New("foo") + assert.Equal(t, errFoo, RecoverCall("test", func() error { return errFoo })) + assert.NoError(t, RecoverCall("test", func() error { return nil })) +} diff --git a/vfs/write.go b/vfs/write.go index fcc700edb..cc1578494 100644 --- a/vfs/write.go +++ b/vfs/write.go @@ -8,6 +8,7 @@ import ( "github.com/rclone/rclone/fs" "github.com/rclone/rclone/fs/operations" + "github.com/rclone/rclone/vfs/vfscommon" ) // WriteFileHandle is an open for write handle on a File @@ -69,15 +70,25 @@ func (fh *WriteFileHandle) openPending() (err error) { var pipeReader *io.PipeReader pipeReader, fh.pipeWriter = io.Pipe() go func() { + var ( + o fs.Object + err error + ) + // Report the outcome however this ends: if a panic escaped here the + // process would die, and if it were recovered without reporting then + // whoever is waiting on the result would wait forever. + defer func() { + // Close the pipeReader so the pipeWriter fails with ErrClosedPipe + _ = pipeReader.Close() + fh.o = o + fh.result <- err + }() + defer vfscommon.RecoverPanic(fh.remote, &err) // NB Rcat deals with Stats.Transferring, etc. - o, err := operations.Rcat(fh.file.ctx, fh.file.Fs(), fh.remote, pipeReader, time.Now(), nil) + o, err = operations.Rcat(fh.file.ctx, fh.file.Fs(), fh.remote, pipeReader, time.Now(), nil) if err != nil { fs.Errorf(fh.remote, "WriteFileHandle.New Rcat failed: %v", err) } - // Close the pipeReader so the pipeWriter fails with ErrClosedPipe - _ = pipeReader.Close() - fh.o = o - fh.result <- err }() fh.file.setSize(0) fh.truncated = true