Merge pull request #17926 from nextcloud/dav-upload-shared-lock-expired

re-acquired expired shared locks on large file uploads

2 files changed, 36 insertions, 3 deletions

diff --git a/apps/dav/lib/Connector/Sabre/File.php b/apps/dav/lib/Connector/Sabre/File.php
index dd25b046bcf..2d019b46b6a 100644
--- a/apps/dav/lib/Connector/Sabre/File.php
+++ b/apps/dav/lib/Connector/Sabre/File.php
@@ -252,10 +252,22 @@ class File extends Node implements IFile {
 				try {
 					$this->changeLock(ILockingProvider::LOCK_EXCLUSIVE);
 				} catch (LockedException $e) {
-					if ($needsPartFile) {
-						$partStorage->unlink($internalPartPath);
+					// during very large uploads, the shared lock we got at the start might have been expired
+					// meaning that the above lock can fail not just only because somebody else got a shared lock
+					// or because there is no existing shared lock to make exclusive
+					//
+					// Thus we try to get a new exclusive lock, if the original lock failed because of a different shared
+					// lock this will still fail, if our original shared lock expired the new lock will be successful and
+					// the entire operation will be safe
+
+					try {
+						$this->acquireLock(ILockingProvider::LOCK_EXCLUSIVE);
+					} catch (LockedException $e) {
+						if ($needsPartFile) {
+							$partStorage->unlink($internalPartPath);
+						}
+						throw new FileLocked($e->getMessage(), $e->getCode(), $e);
 					}
-					throw new FileLocked($e->getMessage(), $e->getCode(), $e);
 				}
 
 				// rename to correct path
diff --git a/apps/dav/tests/unit/Connector/Sabre/FileTest.php b/apps/dav/tests/unit/Connector/Sabre/FileTest.php
index 3e3e80f7525..c579a904260 100644
--- a/apps/dav/tests/unit/Connector/Sabre/FileTest.php
+++ b/apps/dav/tests/unit/Connector/Sabre/FileTest.php
@@ -1201,4 +1201,25 @@ class FileTest extends TestCase {
 
 		$this->assertEquals('new content', $view->file_get_contents('root/file.txt'));
 	}
+
+	public function testPutLockExpired() {
+		$view = new \OC\Files\View('/' . $this->user . '/files/');
+
+		$path = 'test-locking.txt';
+		$info = new \OC\Files\FileInfo(
+			'/' . $this->user . '/files/' . $path,
+			$this->getMockStorage(),
+			null,
+			['permissions' => \OCP\Constants::PERMISSION_ALL],
+			null
+		);
+
+		$file = new \OCA\DAV\Connector\Sabre\File($view, $info);
+
+		// don't lock before the PUT to simulate an expired shared lock
+		$this->assertNotEmpty($file->put($this->getStream('test data')));
+
+		// afterMethod unlocks
+		$view->unlockFile($path, ILockingProvider::LOCK_SHARED);
+	}
 }