From 7556a70280c8a6b681572f26ff06bb60e4ab6888 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Thu, 7 May 2015 15:58:35 -0700
Subject: [PATCH] Prevent Files from requiring infinite policy checks

Summary:
Fixes T6726. Currently, a file may be attached to itself (or to other files, ultimately forming a loop). In this case, we currently run around the loop forever trying to load all the files.

Instead, decline to load objects if we're inside a query which is already loading them. This produces the right policy result //and// completes in finite time.

Test Plan:
  - Looped two files by writing `{F123}` and `{F124}` on the other files, respectively.
  - Loaded `F123`.
  - Saw long hang; used `debug.time-limit` to see huge stack trace instead.
  - Wrote patch.
  - `F123` now loads correctly.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T6726

Differential Revision: https://secure.phabricator.com/D12756
---
 .../phid/query/PhabricatorObjectQuery.php     | 12 +++++++
 .../policy/PhabricatorPolicyAwareQuery.php    | 34 +++++++++++++++++++
 2 files changed, 46 insertions(+)

diff --git a/src/applications/phid/query/PhabricatorObjectQuery.php b/src/applications/phid/query/PhabricatorObjectQuery.php
index 49d238c6a9..fbec78b29b 100644
--- a/src/applications/phid/query/PhabricatorObjectQuery.php
+++ b/src/applications/phid/query/PhabricatorObjectQuery.php
@@ -104,6 +104,16 @@ final class PhabricatorObjectQuery
   }
 
   private function loadObjectsByPHID(array $types, array $phids) {
+    // Don't try to load PHIDs which are already "in flight"; this prevents us
+    // from recursing indefinitely if policy checks or edges form a loop. We
+    // will decline to load the corresponding objects.
+    $in_flight = $this->getPHIDsInFlight();
+    foreach ($phids as $key => $phid) {
+      if (isset($in_flight[$phid])) {
+        unset($phids[$key]);
+      }
+    }
+
     $results = array();
 
     $workspace = $this->getObjectsFromWorkspace($phids);
@@ -119,6 +129,8 @@ final class PhabricatorObjectQuery
       return $results;
     }
 
+    $this->putPHIDsInFlight($phids);
+
     $groups = array();
     foreach ($phids as $phid) {
       $type = phid_get_type($phid);
diff --git a/src/infrastructure/query/policy/PhabricatorPolicyAwareQuery.php b/src/infrastructure/query/policy/PhabricatorPolicyAwareQuery.php
index 1f353d7dfb..e1b2028a19 100644
--- a/src/infrastructure/query/policy/PhabricatorPolicyAwareQuery.php
+++ b/src/infrastructure/query/policy/PhabricatorPolicyAwareQuery.php
@@ -33,6 +33,7 @@ abstract class PhabricatorPolicyAwareQuery extends PhabricatorOffsetPagedQuery {
   private $rawResultLimit;
   private $capabilities;
   private $workspace = array();
+  private $inFlightPHIDs = array();
   private $policyFilteredPHIDs = array();
   private $canUseApplication;
 
@@ -468,6 +469,39 @@ abstract class PhabricatorPolicyAwareQuery extends PhabricatorOffsetPagedQuery {
   }
 
 
+  /**
+   * Mark PHIDs as in flight.
+   *
+   * PHIDs which are "in flight" are actively being queried for. Using this
+   * list can prevent infinite query loops by aborting queries which cycle.
+   *
+   * @param list<phid> List of PHIDs which are now in flight.
+   * @return this
+   */
+  public function putPHIDsInFlight(array $phids) {
+    foreach ($phids as $phid) {
+      $this->inFlightPHIDs[$phid] = $phid;
+    }
+    return $this;
+  }
+
+
+  /**
+   * Get PHIDs which are currently in flight.
+   *
+   * PHIDs which are "in flight" are actively being queried for.
+   *
+   * @return map<phid, phid> PHIDs currently in flight.
+   */
+  public function getPHIDsInFlight() {
+    $results = $this->inFlightPHIDs;
+    if ($this->getParentQuery()) {
+      $results += $this->getParentQuery()->getPHIDsInFlight();
+    }
+    return $results;
+  }
+
+
 /* -(  Policy Query Implementation  )---------------------------------------- */