ensure that most thrift calls cannot access initializing mounts

Summary:
This updates the `EdenServer` class so that the existing `getMount()` and
`getMountPoints()` APIs only return mounts that have finished initializing.

These APIs are primarily used by the thrift interfaces.  In most cases the
callers did not intend to operate on mounts that were still initializing, and
doing so was unsafe.  The code could potentially dereference a null pointer if
it tried to access the mount's root inode before the root inode object had
been created.

New `getMountUnsafe()` and `getAllMountPoints()` APIs have been added for call
sites that explicitly want to be able to access mounts that may still be
initializing.  Currently the `listMounts()` thrift API is the only location
that needs this.

Reviewed By: strager

Differential Revision: D13981139

fbshipit-source-id: e6168d7a15694c79ca2bcc129dda46f82382e8e9

eden/fs/inodes/EdenMount.h

@@ -180,10 +180,21 @@ class EdenMount {
    * method is called, so this method should primarily only be used for
    * debugging & diagnostics.
    */
-  State getState() {
+  State getState() const {
     return state_.load(std::memory_order_acquire);
   }
 
+  /**
+   * Check if inode operations can be performed on this EdenMount.
+   *
+   * This returns false for mounts that are still initializing and do not have
+   * their root inode loaded yet.
+   */
+  bool isSafeForInodeAccess() const {
+    auto state = getState();
+    return !(state == State::UNINITIALIZED || state == State::INITIALIZING);
+  }
+
   /**
    * Get the FUSE channel for this mount point.
    *

eden/fs/service/EdenServer.cpp

@@ -965,6 +965,23 @@ void EdenServer::mountFinished(
 }
 
 EdenServer::MountList EdenServer::getMountPoints() const {
+  MountList results;
+  {
+    const auto mountPoints = mountPoints_.rlock();
+    for (const auto& entry : *mountPoints) {
+      const auto& mount = entry.second.edenMount;
+      // Avoid returning mount points that are still initializing and are not
+      // ready to perform inode operations yet.
+      if (!mount->isSafeForInodeAccess()) {
+        continue;
+      }
+      results.emplace_back(mount);
+    }
+  }
+  return results;
+}
+
+EdenServer::MountList EdenServer::getAllMountPoints() const {
   MountList results;
   {
     const auto mountPoints = mountPoints_.rlock();
@@ -976,19 +993,20 @@ EdenServer::MountList EdenServer::getMountPoints() const {
 }
 
 shared_ptr<EdenMount> EdenServer::getMount(StringPiece mountPath) const {
-  const auto mount = getMountOrNull(mountPath);
-  if (!mount) {
+  const auto mount = getMountUnsafe(mountPath);
+  if (!mount->isSafeForInodeAccess()) {
     throw EdenError(folly::to<string>(
-        "mount point \"", mountPath, "\" is not known to this eden instance"));
+        "mount point \"", mountPath, "\" is still initializing"));
   }
   return mount;
 }
 
-shared_ptr<EdenMount> EdenServer::getMountOrNull(StringPiece mountPath) const {
+shared_ptr<EdenMount> EdenServer::getMountUnsafe(StringPiece mountPath) const {
   const auto mountPoints = mountPoints_.rlock();
   const auto it = mountPoints->find(mountPath);
   if (it == mountPoints->end()) {
-    return nullptr;
+    throw EdenError(folly::to<string>(
+        "mount point \"", mountPath, "\" is not known to this eden instance"));
   }
   return it->second.edenMount;
 }

eden/fs/service/EdenServer.h

@@ -197,21 +197,41 @@ class EdenServer : private TakeoverHandler {
     return server_;
   }
 
+  /**
+   * Get the list of mount points.
+   *
+   * The returned list excludes mount points that are still in the process of
+   * initializing.  This is the behavior desired by most callers, as no access
+   * to inode information is allowed yet on initializing mount points.
+   *
+   * Mount points in the returned list may be in the process of shutting down.
+   * (Even if we attempted to return only running mount points, they may
+   * transition to shutting down before the caller can access them.)
+   */
   MountList getMountPoints() const;
 
+  /**
+   * Get all mount points, including mounts that are currently initializing.
+   */
+  MountList getAllMountPoints() const;
+
   /**
    * Look up an EdenMount by the path where it is mounted.
    *
-   * Throws an EdenError if no mount exists with the specified path.
+   * Throws an EdenError if no mount exists with the specified path, or if the
+   * mount is still initializing and is not ready for inode operations yet.
    */
   std::shared_ptr<EdenMount> getMount(folly::StringPiece mountPath) const;
 
   /**
    * Look up an EdenMount by the path where it is mounted.
    *
-   * Returns nullptr if no mount exists with the specified path.
+   * This is similar to getMount(), but will also return mounts that are still
+   * initializing.  It is the caller's responsibility to ensure they do not
+   * perform any inode operations on the returned mount without first verifying
+   * it is ready for access.
    */
-  std::shared_ptr<EdenMount> getMountOrNull(folly::StringPiece mountPath) const;
+  std::shared_ptr<EdenMount> getMountUnsafe(folly::StringPiece mountPath) const;
 
   std::shared_ptr<LocalStore> getLocalStore() const {
     return localStore_;

eden/fs/service/EdenServiceHandler.cpp

@@ -306,7 +306,7 @@ void EdenServiceHandler::unmount(std::unique_ptr<std::string> mountPoint) {
 void EdenServiceHandler::listMounts(std::vector<MountInfo>& results) {
 #ifndef EDEN_WIN
   auto helper = INSTRUMENT_THRIFT_CALL(DBG3);
-  for (const auto& edenMount : server_->getMountPoints()) {
+  for (const auto& edenMount : server_->getAllMountPoints()) {
     MountInfo info;
     info.mountPoint = edenMount->getPath().value();
     info.edenClientPath = edenMount->getConfig()->getClientDirectory().value();

eden/integration/mount_test.py

@@ -16,7 +16,13 @@ from typing import Optional, Set
 
 from eden.cli.util import poll_until
 from eden.thrift import EdenClient, EdenNotRunningError
-from facebook.eden.ttypes import FaultDefinition, MountState, UnblockFaultArg
+from facebook.eden.ttypes import (
+    EdenError,
+    FaultDefinition,
+    MountState,
+    UnblockFaultArg,
+    WorkingDirectoryParents,
+)
 from fb303.ttypes import fb_status
 from thrift.Thrift import TException
 
@@ -142,12 +148,33 @@ class MountTest(testcase.EdenRepoTest):
             poll_until(mount_started, timeout=30)
             self.assertEqual({self.mount: "INITIALIZING"}, self.eden.list_cmd_simple())
 
+            # Most thrift calls to access the mount should be disallowed while it is
+            # still initializing.
+            self._assert_thrift_calls_fail_during_mount_init(client)
+
             # Unblock mounting and wait for the mount to transition to running
             client.unblockFault(UnblockFaultArg(keyClass="mount", keyValueRegex=".*"))
 
             self._wait_for_mount_running(client)
             self.assertEqual({self.mount: "RUNNING"}, self.eden.list_cmd_simple())
 
+    def _assert_thrift_calls_fail_during_mount_init(self, client: EdenClient) -> None:
+        error_regex = "mount point .* is still initializing"
+        mount_path = Path(self.mount)
+        null_commit = b"\00" * 20
+
+        with self.assertRaisesRegex(EdenError, error_regex):
+            client.getFileInformation(mountPoint=bytes(mount_path), paths=[b""])
+
+        with self.assertRaisesRegex(EdenError, error_regex):
+            client.getScmStatus(
+                mountPoint=bytes(mount_path), listIgnored=False, commit=null_commit
+            )
+
+        parents = WorkingDirectoryParents(parent1=null_commit)
+        with self.assertRaisesRegex(EdenError, error_regex):
+            client.resetParentCommits(mountPoint=bytes(mount_path), parents=parents)
+
     def test_start_blocked_mount_init(self) -> None:
         self.eden.shutdown()
         self.eden.spawn_nowait(

eden/win/fs/mount/EdenMount.h

@@ -280,6 +280,10 @@ class EdenMount {
     return gid_;
   }
 
+  bool isSafeForInodeAccess() const {
+    return true;
+  }
+
   /**
    * Return a new stat structure that has been minimally initialized with
    * data for this mount point.