From 21b3251494955a08ec24d824c056bb9785e0b44d Mon Sep 17 00:00:00 2001 From: Andrew Gunnerson Date: Thu, 16 Jan 2025 00:32:46 -0500 Subject: [PATCH] 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 --- .../com/chiller3/rsaf/RcloneProviderTest.kt | 69 ++++++++++--------- .../chiller3/rsaf/rclone/RcloneProvider.kt | 18 ++++- .../rsaf/settings/EditRemoteViewModel.kt | 8 ++- rcbridge/rcbridge.go | 11 ++- 4 files changed, 69 insertions(+), 37 deletions(-) diff --git a/app/src/androidTest/java/com/chiller3/rsaf/RcloneProviderTest.kt b/app/src/androidTest/java/com/chiller3/rsaf/RcloneProviderTest.kt index 3c08f8e..61e2bd2 100644 --- 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()) } } \ No newline at end of file diff --git a/app/src/main/java/com/chiller3/rsaf/rclone/RcloneProvider.kt b/app/src/main/java/com/chiller3/rsaf/rclone/RcloneProvider.kt index abbf8ef..f9f1a4b 100644 --- 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 index 242289e..bdabcaf 100644 --- 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 index 0530be4..bb48138 100644 --- 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 {