From 7771aaacf68629d55edde6c08bc80cffc69e5534 Mon Sep 17 00:00:00 2001
From: WeidiDeng <weidi_deng@icloud.com>
Date: Wed, 10 Nov 2021 16:49:09 +0800
Subject: [PATCH] vfs: fix writing to a read only directory creating spurious
 directory entries

Before this fix, when a write to a read only directory failed, rclone
would leav spurious directory entries in the directory.

This confuses `rclone serve webdav` into giving this error

    http: superfluous response.WriteHeader

This fixes the VFS layer to remove any directory entries where the
file creation did not succeed.

Fixes #5702
---
 vfs/file.go       | 10 ++++----
 vfs/write.go      |  5 ++++
 vfs/write_test.go | 58 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 69 insertions(+), 4 deletions(-)

diff --git a/vfs/file.go b/vfs/file.go
index 09b8a85d8..e679984d8 100644
--- a/vfs/file.go
+++ b/vfs/file.go
@@ -615,10 +615,6 @@ func (f *File) Remove() (err error) {
 		wasWriting = d.vfs.cache.Remove(f.Path())
 	}
 
-	// Remove the item from the directory listing
-	// called with File.mu released
-	d.delObject(f.Name())
-
 	f.muRW.Lock() // muRW must be locked before mu to avoid
 	f.mu.Lock()   // deadlock in RWFileHandle.openPending and .close
 	if f.o != nil {
@@ -635,6 +631,12 @@ func (f *File) Remove() (err error) {
 			fs.Debugf(f._path(), "File.Remove file error: %v", err)
 		}
 	}
+
+	// Remove the item from the directory listing
+	// called with File.mu released when there is no error removing the underlying file
+	if err == nil {
+		d.delObject(f.Name())
+	}
 	return err
 }
 
diff --git a/vfs/write.go b/vfs/write.go
index e18f1d001..b820d8172 100644
--- a/vfs/write.go
+++ b/vfs/write.go
@@ -203,6 +203,11 @@ func (fh *WriteFileHandle) close() (err error) {
 	if err == nil {
 		fh.file.setObject(fh.o)
 		err = writeCloseErr
+	} else {
+		// Remove vfs file entry when no object is present
+		if fh.file.getObject() == nil {
+			_ = fh.file.Remove()
+		}
 	}
 	return err
 }
diff --git a/vfs/write_test.go b/vfs/write_test.go
index b059cdad1..629628555 100644
--- a/vfs/write_test.go
+++ b/vfs/write_test.go
@@ -5,6 +5,7 @@ import (
 	"errors"
 	"io"
 	"os"
+	"runtime"
 	"sync"
 	"testing"
 	"time"
@@ -28,6 +29,63 @@ func writeHandleCreate(t *testing.T) (r *fstest.Run, vfs *VFS, fh *WriteFileHand
 	return r, vfs, fh
 }
 
+// Test write when underlying storage is readonly, must be run as non-root
+func TestWriteFileHandleReadonly(t *testing.T) {
+	if runtime.GOOS == "windows" {
+		t.Skipf("Skipping test on %s", runtime.GOOS)
+	}
+	if *fstest.RemoteName != "" {
+		t.Skip("Skipping test on non local remote")
+	}
+	r, vfs, fh := writeHandleCreate(t)
+
+	// Write a file, so underlying remote will be created
+	_, err := fh.Write([]byte("hello"))
+	assert.NoError(t, err)
+
+	err = fh.Close()
+	assert.NoError(t, err)
+
+	var info os.FileInfo
+	info, err = os.Stat(r.FremoteName)
+	assert.NoError(t, err)
+
+	// Remove write permission
+	oldMode := info.Mode()
+	err = os.Chmod(r.FremoteName, oldMode^(oldMode&0222))
+	assert.NoError(t, err)
+
+	var h Handle
+	h, err = vfs.OpenFile("file2", os.O_WRONLY|os.O_CREATE, 0777)
+	require.NoError(t, err)
+
+	var ok bool
+	fh, ok = h.(*WriteFileHandle)
+	require.True(t, ok)
+
+	// error is propagated to Close()
+	_, err = fh.Write([]byte("hello"))
+	assert.NoError(t, err)
+
+	err = fh.Close()
+	assert.NotNil(t, err)
+
+	// Remove should fail
+	err = vfs.Remove("file1")
+	assert.NotNil(t, err)
+
+	// Only file1 should exist
+	_, err = vfs.Stat("file1")
+	assert.NoError(t, err)
+
+	_, err = vfs.Stat("file2")
+	assert.Equal(t, true, errors.Is(err, os.ErrNotExist))
+
+	// Restore old permission
+	err = os.Chmod(r.FremoteName, oldMode)
+	assert.NoError(t, err)
+}
+
 func TestWriteFileHandleMethods(t *testing.T) {
 	r, vfs, fh := writeHandleCreate(t)