From 466013f11a6d70354321e291fdef692b1dae4b3c Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 2 Mar 2021 12:53:52 -0800
Subject: [PATCH] Prevent external connections from being mutated on held locks

Summary: Ref T13627. This makes the API harder to misuse: setting an external connection on a held lock isn't a meaningful operation. Prevent it.

Test Plan: Added a failing test, made it pass.

Maniphest Tasks: T13627

Differential Revision: https://secure.phabricator.com/D21584
---
 .../DiffusionRepositoryClusterEngine.php      |  2 +-
 .../PhabricatorStorageManagementWorkflow.php  |  2 +-
 .../util/PhabricatorGlobalLock.php            | 15 +++++++++--
 .../PhabricatorGlobalLockTestCase.php         | 26 ++++++++++++++++++-
 4 files changed, 40 insertions(+), 5 deletions(-)

diff --git a/src/applications/diffusion/protocol/DiffusionRepositoryClusterEngine.php b/src/applications/diffusion/protocol/DiffusionRepositoryClusterEngine.php
index 2d815917a4..9df37c7fb5 100644
--- a/src/applications/diffusion/protocol/DiffusionRepositoryClusterEngine.php
+++ b/src/applications/diffusion/protocol/DiffusionRepositoryClusterEngine.php
@@ -311,7 +311,7 @@ final class DiffusionRepositoryClusterEngine extends Phobject {
     $write_lock = PhabricatorRepositoryWorkingCopyVersion::getWriteLock(
       $repository_phid);
 
-    $write_lock->useSpecificConnection($locked_connection);
+    $write_lock->setExternalConnection($locked_connection);
 
     $this->logLine(
       pht(
diff --git a/src/infrastructure/storage/management/workflow/PhabricatorStorageManagementWorkflow.php b/src/infrastructure/storage/management/workflow/PhabricatorStorageManagementWorkflow.php
index 490036e36d..71f6374b2a 100644
--- a/src/infrastructure/storage/management/workflow/PhabricatorStorageManagementWorkflow.php
+++ b/src/infrastructure/storage/management/workflow/PhabricatorStorageManagementWorkflow.php
@@ -1229,7 +1229,7 @@ abstract class PhabricatorStorageManagementWorkflow
     // log table yet, or may need to adjust it.
 
     return PhabricatorGlobalLock::newLock('adjust', $parameters)
-      ->useSpecificConnection($api->getConn(null))
+      ->setExternalConnection($api->getConn(null))
       ->setDisableLogging(true)
       ->lock();
   }
diff --git a/src/infrastructure/util/PhabricatorGlobalLock.php b/src/infrastructure/util/PhabricatorGlobalLock.php
index cf380c3726..3bf5ab0c54 100644
--- a/src/infrastructure/util/PhabricatorGlobalLock.php
+++ b/src/infrastructure/util/PhabricatorGlobalLock.php
@@ -91,8 +91,13 @@ final class PhabricatorGlobalLock extends PhutilLock {
    * @param  AphrontDatabaseConnection
    * @return this
    */
-  public function useSpecificConnection(AphrontDatabaseConnection $conn) {
-    $this->conn = $conn;
+  public function setExternalConnection(AphrontDatabaseConnection $conn) {
+    if ($this->conn) {
+      throw new Exception(
+        pht(
+          'Lock is already held, and must be released before the '.
+          'connection may be changed.'));
+    }
     $this->externalConnection = $conn;
     return $this;
   }
@@ -118,6 +123,12 @@ final class PhabricatorGlobalLock extends PhutilLock {
   protected function doLock($wait) {
     $conn = $this->conn;
 
+    if (!$conn) {
+      if ($this->externalConnection) {
+        $conn = $this->externalConnection;
+      }
+    }
+
     if (!$conn) {
       // Try to reuse a connection from the connection pool.
       $conn = array_pop(self::$pool);
diff --git a/src/infrastructure/util/__tests__/PhabricatorGlobalLockTestCase.php b/src/infrastructure/util/__tests__/PhabricatorGlobalLockTestCase.php
index 4e24cdfed4..8d5176f391 100644
--- a/src/infrastructure/util/__tests__/PhabricatorGlobalLockTestCase.php
+++ b/src/infrastructure/util/__tests__/PhabricatorGlobalLockTestCase.php
@@ -54,7 +54,7 @@ final class PhabricatorGlobalLockTestCase
 
     $lock_name = $this->newLockName();
     $lock = PhabricatorGlobalLock::newLock($lock_name);
-    $lock->useSpecificConnection($conn);
+    $lock->setExternalConnection($conn);
     $lock->lock();
 
     $this->assertEqual(
@@ -85,6 +85,30 @@ final class PhabricatorGlobalLockTestCase
     PhabricatorGlobalLock::clearConnectionPool();
   }
 
+  public function testExternalConnectionMutationScope() {
+    $conn = id(new HarbormasterScratchTable())
+      ->establishConnection('w');
+
+    $lock_name = $this->newLockName();
+    $lock = PhabricatorGlobalLock::newLock($lock_name);
+    $lock->lock();
+
+    $caught = null;
+    try {
+      $lock->setExternalConnection($conn);
+    } catch (Exception $ex) {
+      $caught = $ex;
+    } catch (Throwable $ex) {
+      $caught = $ex;
+    }
+
+    $lock->unlock();
+
+    $this->assertTrue(
+      ($caught instanceof Exception),
+      pht('Changing connection while locked is forbidden.'));
+  }
+
   private function newLockName() {
     return 'testlock-'.Filesystem::readRandomCharacters(16);
   }