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

@@ -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 {

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) {

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) {

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)
 	}

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

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()
+}

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 }))
+}

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