Only allow copy/move via rclone when it can be done server-side

If copyDocument() and moveDocument() are slow, the binder transaction will time out and Android will kill RSAF. If we can't perform the operation quickly via server-side operations, let's just tell the client that the operation is unsupported. Android's DocumentsUI specifically handles this scenario and will do the copy/move operation itself, which isn't subject to this problem. Fixes: #124 Signed-off-by: Andrew Gunnerson <[email protected]>
chenxiaolong · Jan 18, 2025 · 21b3251 · 21b3251
1 parent 6233904
commit 21b3251
Show file tree

Hide file tree

Showing 4 changed files with 69 additions and 37 deletions.
diff --git a/app/src/androidTest/java/com/chiller3/rsaf/RcloneProviderTest.kt b/app/src/androidTest/java/com/chiller3/rsaf/RcloneProviderTest.kt
@@ -445,15 +445,30 @@ class RcloneProviderTest {
         }
     }
 
-    private fun copyMove(source: Uri, targetParent: Uri, copy: Boolean): Uri? =
-        if (copy) {
-            DocumentsContract.copyDocument(appContext.contentResolver, source, targetParent)
-        } else {
-            DocumentsContract.moveDocument(appContext.contentResolver, source, source.docParent(),
-                targetParent)
+    @Test
+    fun copyDocument() {
+        val file = File(rootDir, "file.txt").apply {
+            writeText("helloworld")
+        }
+
+        // We can't easily test a successful copy because it requires server-side copy support,
+        // which is not the case for local filesystem paths. However, we can test the detection of
+        // server-side copy support.
+
+        assertThrows(UnsupportedOperationException::class.java) {
+            DocumentsContract.copyDocument(
+                appContext.contentResolver,
+                docUriFromRoot("file.txt"),
+                docUriFromRoot(),
+            )
         }
 
-    private fun testCopyMove(copy: Boolean) {
+        assertTrue(file.exists())
+        assertFalse(File("file(1).txt").exists())
+    }
+
+    @Test
+    fun moveDocument() {
         val file = File(rootDir, "file.txt").apply {
             writeText("helloworld")
         }
@@ -462,38 +477,28 @@ class RcloneProviderTest {
             assertTrue(File(this, "file").createNewFile())
         }
 
-        // Copy/move file
-        var newUri = copyMove(
-            docUriFromRoot("file.txt"),
+        // Move file
+        var sourceDoc = docUriFromRoot("file.txt")
+        var newUri = DocumentsContract.moveDocument(
+            appContext.contentResolver,
+            sourceDoc,
+            sourceDoc.docParent(),
             docUriFromRoot(),
-            copy,
         )
         assertEquals("file(1).txt", newUri?.docBasename())
         assertEquals("helloworld", File(rootDir, "file(1).txt").readText())
-        if (!copy) {
-            assertFalse(file.exists())
-        }
-
-        // Copy/move directory
-        newUri = copyMove(
-            docUriFromRoot("dir"),
+        assertFalse(file.exists())
+
+        // Move directory
+        sourceDoc = docUriFromRoot("dir")
+        newUri = DocumentsContract.moveDocument(
+            appContext.contentResolver,
+            sourceDoc,
+            sourceDoc.docParent(),
             docUriFromRoot(),
-            copy,
         )
         assertEquals("dir(1)", newUri?.docBasename())
         assertTrue(File(File(rootDir, "dir(1)"), "file").exists())
-        if (!copy) {
-            assertFalse(dir.exists())
-        }
-    }
-
-    @Test
-    fun copyDocument() {
-        testCopyMove(true)
-    }
-
-    @Test
-    fun moveDocument() {
-        testCopyMove(false)
+        assertFalse(dir.exists())
     }
 }
diff --git a/app/src/main/java/com/chiller3/rsaf/rclone/RcloneProvider.kt b/app/src/main/java/com/chiller3/rsaf/rclone/RcloneProvider.kt
@@ -794,10 +794,26 @@ class RcloneProvider : DocumentsProvider(), SharedPreferences.OnSharedPreference
      */
     private fun copyOrMove(sourceDocumentId: String, targetParentDocumentId: String,
                            copy: Boolean): String {
+        val (sourceRemote, _) = splitRemote(sourceDocumentId)
+        val (targetRemote, _) = splitRemote(targetParentDocumentId)
+        if (sourceRemote != targetRemote) {
+            throw UnsupportedOperationException("Copying/moving across remotes is not supported")
+        }
+
+        val error = RbError()
+        val features = Rcbridge.rbRemoteFeatures(sourceRemote, error)
+            ?: throw error.toException("rbRemoteFeatures")
+        val serverSide = if (copy) { features.copy } else { features.move }
+        if (!serverSide) {
+            // Otherwise, it's really easy for the binder transaction to time out and cause Android
+            // to kill our process. copyDocument() and moveDocument() aren't really meant to block
+            // for very long.
+            throw UnsupportedOperationException("Copying/moving must be done server-side")
+        }
+
         val (sourceParentDocumentId, fileName) = splitPath(sourceDocumentId)
         val (baseName, ext) = splitExt(fileName, documentIsDir(sourceDocumentId))
         val targetBaseDocumentId = Rcbridge.rbPathJoin(targetParentDocumentId, baseName)
-        val error = RbError()
 
         return retryUnique(targetBaseDocumentId, ext, ConflictDetection.STAT) {
             if (!Rcbridge.rbDocCopyOrMove(sourceDocumentId, it, copy, error)) {

diff --git a/app/src/main/java/com/chiller3/rsaf/settings/EditRemoteViewModel.kt b/app/src/main/java/com/chiller3/rsaf/settings/EditRemoteViewModel.kt
@@ -8,8 +8,10 @@ package com.chiller3.rsaf.settings
 import android.util.Log
 import androidx.lifecycle.ViewModel
 import androidx.lifecycle.viewModelScope
+import com.chiller3.rsaf.binding.rcbridge.RbError
 import com.chiller3.rsaf.binding.rcbridge.RbRemoteFeaturesResult
 import com.chiller3.rsaf.binding.rcbridge.Rcbridge
+import com.chiller3.rsaf.extension.toException
 import com.chiller3.rsaf.rclone.RcloneConfig
 import com.chiller3.rsaf.rclone.RcloneRpc
 import com.chiller3.rsaf.rclone.VfsCache
@@ -101,7 +103,11 @@ class EditRemoteViewModel : ViewModel() {
                 if (_remoteConfig.value.features == null) {
                     withContext(Dispatchers.IO) {
                         _remoteConfig.update {
-                            it.copy(features = Rcbridge.rbRemoteFeatures("$remote:"))
+                            val error = RbError()
+                            val features = Rcbridge.rbRemoteFeatures("$remote:", error)
+                                ?: throw error.toException("rbRemoteFeatures")
+
+                            it.copy(features = features)
                         }
                     }
                 }

diff --git a/rcbridge/rcbridge.go b/rcbridge/rcbridge.go
@@ -595,25 +595,30 @@ func getVfsForDoc(doc string) (*vfs.VFS, string, error) {
 }
 
 type RbRemoteFeaturesResult struct {
+	Copy      bool
+	Move      bool
 	PutStream bool
 	About     bool
 }
 
 // Return whether the specified remote supports streaming.
-func RbRemoteFeatures(remote string) (*RbRemoteFeaturesResult, error) {
+func RbRemoteFeatures(remote string, errOut *RbError) *RbRemoteFeaturesResult {
 	f, err := getFs(remote)
 	if err != nil {
-		return nil, err
+		assignError(errOut, err, syscall.EINVAL)
+		return nil
 	}
 
 	features := f.Features()
 
 	result := RbRemoteFeaturesResult{
+		Copy:      features.Copy != nil,
+		Move:      features.Move != nil,
 		PutStream: features.PutStream != nil,
 		About:     features.About != nil,
 	}
 
-	return &result, nil
+	return &result
 }
 
 type RbRemoteSplitResult struct {