Introduce ownership tracking for base::ScopedFD

Introduces acquire/release traits for base::ScopedFD on
Linux and Chrome OS. This should help catch double-close bugs
happening in the field.

Bug: 1208827
Change-Id: I918d79c343c0027ee1ce4353c7acbe7c0e79d1dd
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2894590
Commit-Queue: Ken Rockot <rockot@google.com>
Reviewed-by: Lei Zhang <thestig@chromium.org>
Reviewed-by: Robert Sesek <rsesek@chromium.org>
Cr-Commit-Position: refs/heads/master@{#884735}

base/BUILD.gn

@@ -1280,6 +1280,7 @@ component("base") {
       "files/file_path_watcher_linux.cc",
       "files/file_path_watcher_linux.h",
       "files/file_util_linux.cc",
+      "files/scoped_file_linux.cc",
       "process/internal_linux.cc",
       "process/internal_linux.h",
       "process/memory_linux.cc",
@@ -3326,7 +3327,10 @@ test("base_unittests") {
   }
 
   if (is_linux || is_chromeos) {
-    sources += [ "debug/proc_maps_linux_unittest.cc" ]
+    sources += [
+      "debug/proc_maps_linux_unittest.cc",
+      "files/scoped_file_linux_unittest.cc",
+    ]
   }
 
   if (is_mac) {

base/files/scoped_file.h

@@ -26,11 +26,21 @@ struct BASE_EXPORT ScopedFDCloseTraits : public ScopedGenericOwnershipTracking {
   static void Release(const ScopedGeneric<int, ScopedFDCloseTraits>&, int);
 };
 #elif defined(OS_POSIX) || defined(OS_FUCHSIA)
+#if defined(OS_CHROMEOS) || defined(OS_LINUX)
+// On ChromeOS and Linux we guard FD lifetime with a global table and hook into
+// libc close() to perform checks.
+struct BASE_EXPORT ScopedFDCloseTraits : public ScopedGenericOwnershipTracking {
+#else
 struct BASE_EXPORT ScopedFDCloseTraits {
+#endif
   static int InvalidValue() {
     return -1;
   }
   static void Free(int fd);
+#if defined(OS_CHROMEOS) || defined(OS_LINUX)
+  static void Acquire(const ScopedGeneric<int, ScopedFDCloseTraits>&, int);
+  static void Release(const ScopedGeneric<int, ScopedFDCloseTraits>&, int);
+#endif
 };
 #endif
 
@@ -44,6 +54,36 @@ struct ScopedFILECloser {
 
 }  // namespace internal
 
+#if defined(OS_CHROMEOS) || defined(OS_LINUX)
+namespace subtle {
+
+// Enables or disables enforcement of FD ownership as tracked by ScopedFD
+// objects. Enforcement is disabled by default since it proves unwieldy in some
+// test environments, but tracking is always done. It's best to enable this as
+// early as possible in a process's lifetime.
+void BASE_EXPORT EnableFDOwnershipEnforcement(bool enabled);
+
+// Resets ownership state of all FDs. The only permissible use of this API is
+// in a forked child process between the fork() and a subsequent exec() call.
+//
+// For one issue, it is common to mass-close most open FDs before calling
+// exec(), to avoid leaking FDs into the new executable's environment. For
+// processes which have enabled FD ownership enforcement, this reset operation
+// is necessary before performing such closures.
+//
+// Furthermore, fork()+exec() may be used in a multithreaded context, and
+// because fork() is not atomic, the FD ownership state in the child process may
+// be inconsistent with the actual set of opened file descriptors once fork()
+// returns in the child process.
+//
+// It is therefore especially important to call this ASAP after fork() in the
+// child process if any FD manipulation will be done prior to the subsequent
+// exec call.
+void BASE_EXPORT ResetFDOwnership();
+
+}  // namespace subtle
+#endif
+
 // -----------------------------------------------------------------------------
 
 #if defined(OS_POSIX) || defined(OS_FUCHSIA)
@@ -64,6 +104,12 @@ typedef ScopedGeneric<int, internal::ScopedFDCloseTraits> ScopedFD;
 // Automatically closes |FILE*|s.
 typedef std::unique_ptr<FILE, internal::ScopedFILECloser> ScopedFILE;
 
+#if defined(OS_CHROMEOS) || defined(OS_LINUX)
+// Queries the ownership status of an FD, i.e. whether it is currently owned by
+// a ScopedFD in the calling process.
+bool BASE_EXPORT IsFDOwned(int fd);
+#endif  // defined(OS_CHROMEOS) || defined(OS_LINUX)
+
 }  // namespace base
 
 #endif  // BASE_FILES_SCOPED_FILE_H_

base/files/scoped_file_linux.cc

@@ -0,0 +1,91 @@
+// Copyright 2021 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "base/files/scoped_file.h"
+
+#include <algorithm>
+#include <array>
+#include <atomic>
+
+#include "base/compiler_specific.h"
+#include "base/debug/stack_trace.h"
+#include "base/immediate_crash.h"
+#include "base/logging.h"
+#include "base/strings/string_piece.h"
+
+namespace {
+
+// We want to avoid any kind of allocations in our close() implementation, so we
+// use a fixed-size table. Given our common FD limits and the preference for new
+// FD allocations to use the lowest available descriptor, this should be
+// sufficient to guard most FD lifetimes. The worst case scenario if someone
+// attempts to own a higher FD is that we don't track it.
+const int kMaxTrackedFds = 4096;
+
+std::atomic_bool g_is_ownership_enforced{false};
+std::array<std::atomic_bool, kMaxTrackedFds> g_is_fd_owned;
+
+NOINLINE void CrashOnFdOwnershipViolation() {
+  RAW_LOG(ERROR, "Crashing due to FD ownership violation:\n");
+  base::debug::StackTrace().Print();
+  IMMEDIATE_CRASH();
+}
+
+bool CanTrack(int fd) {
+  return fd >= 0 && fd < kMaxTrackedFds;
+}
+
+void UpdateAndCheckFdOwnership(int fd, bool owned) {
+  if (CanTrack(fd) && g_is_fd_owned[fd].exchange(owned) == owned &&
+      g_is_ownership_enforced) {
+    CrashOnFdOwnershipViolation();
+  }
+}
+
+}  // namespace
+
+namespace base {
+namespace internal {
+
+// static
+void ScopedFDCloseTraits::Acquire(const ScopedFD& owner, int fd) {
+  UpdateAndCheckFdOwnership(fd, /*owned=*/true);
+}
+
+// static
+void ScopedFDCloseTraits::Release(const ScopedFD& owner, int fd) {
+  UpdateAndCheckFdOwnership(fd, /*owned=*/false);
+}
+
+}  // namespace internal
+
+namespace subtle {
+
+void EnableFDOwnershipEnforcement(bool enabled) {
+  g_is_ownership_enforced = enabled;
+}
+
+void ResetFDOwnership() {
+  std::fill(g_is_fd_owned.begin(), g_is_fd_owned.end(), false);
+}
+
+}  // namespace subtle
+
+bool IsFDOwned(int fd) {
+  return CanTrack(fd) && g_is_fd_owned[fd];
+}
+
+}  // namespace base
+
+extern "C" {
+
+int __close(int);
+
+__attribute__((visibility("default"), noinline)) int close(int fd) {
+  if (base::IsFDOwned(fd) && g_is_ownership_enforced)
+    CrashOnFdOwnershipViolation();
+  return __close(fd);
+}
+
+}  // extern "C"

base/files/scoped_file_linux_unittest.cc

@@ -0,0 +1,54 @@
+// Copyright 2021 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "base/files/scoped_file.h"
+
+#include "base/files/file_util.h"
+#include "base/files/scoped_temp_dir.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace base {
+namespace {
+
+class ScopedFDOwnershipTrackingTest : public testing::Test {
+ public:
+  void SetUp() override { ASSERT_TRUE(temp_dir_.CreateUniqueTempDir()); }
+  void TearDown() override { ASSERT_TRUE(temp_dir_.Delete()); }
+
+  ScopedFD OpenFD() {
+    FilePath dont_care;
+    return CreateAndOpenFdForTemporaryFileInDir(temp_dir_.GetPath(),
+                                                &dont_care);
+  }
+
+ private:
+  ScopedTempDir temp_dir_;
+};
+
+TEST_F(ScopedFDOwnershipTrackingTest, BasicTracking) {
+  ScopedFD fd = OpenFD();
+  EXPECT_TRUE(IsFDOwned(fd.get()));
+  int fd_value = fd.get();
+  fd.reset();
+  EXPECT_FALSE(IsFDOwned(fd_value));
+}
+
+#if defined(GTEST_HAS_DEATH_TEST)
+
+TEST_F(ScopedFDOwnershipTrackingTest, NoDoubleOwnership) {
+  ScopedFD fd = OpenFD();
+  subtle::EnableFDOwnershipEnforcement(true);
+  EXPECT_DEATH(ScopedFD(fd.get()), "");
+}
+
+TEST_F(ScopedFDOwnershipTrackingTest, CrashOnUnownedClose) {
+  ScopedFD fd = OpenFD();
+  subtle::EnableFDOwnershipEnforcement(true);
+  EXPECT_DEATH(close(fd.get()), "");
+}
+
+#endif  // defined(GTEST_HAS_DEATH_TEST)
+
+}  // namespace
+}  // namespace base

base/process/launch_posix.cc

@@ -358,19 +358,27 @@ Process LaunchProcess(const std::vector<std::string>& argv,
     // might do things like block waiting for threads that don't even exist
     // in the child.
 
-    // If a child process uses the readline library, the process block forever.
-    // In BSD like OSes including OS X it is safe to assign /dev/null as stdin.
-    // See http://crbug.com/56596.
-    base::ScopedFD null_fd(HANDLE_EINTR(open("/dev/null", O_RDONLY)));
-    if (!null_fd.is_valid()) {
-      RAW_LOG(ERROR, "Failed to open /dev/null");
-      _exit(127);
-    }
+#if defined(OS_LINUX) || defined(OS_CHROMEOS)
+    // See comments on the ResetFDOwnership() declaration in
+    // base/files/scoped_file.h regarding why this is called early here.
+    subtle::ResetFDOwnership();
+#endif
 
-    int new_fd = HANDLE_EINTR(dup2(null_fd.get(), STDIN_FILENO));
-    if (new_fd != STDIN_FILENO) {
-      RAW_LOG(ERROR, "Failed to dup /dev/null for stdin");
-      _exit(127);
+    {
+      // If a child process uses the readline library, the process block
+      // forever. In BSD like OSes including OS X it is safe to assign /dev/null
+      // as stdin. See http://crbug.com/56596.
+      base::ScopedFD null_fd(HANDLE_EINTR(open("/dev/null", O_RDONLY)));
+      if (!null_fd.is_valid()) {
+        RAW_LOG(ERROR, "Failed to open /dev/null");
+        _exit(127);
+      }
+
+      int new_fd = HANDLE_EINTR(dup2(null_fd.get(), STDIN_FILENO));
+      if (new_fd != STDIN_FILENO) {
+        RAW_LOG(ERROR, "Failed to dup /dev/null for stdin");
+        _exit(127);
+      }
     }
 
     if (options.new_process_group) {
@@ -550,6 +558,12 @@ static bool GetAppOutputInternal(
       // DANGER: no calls to malloc or locks are allowed from now on:
       // http://crbug.com/36678
 
+#if defined(OS_LINUX) || defined(OS_CHROMEOS)
+      // See comments on the ResetFDOwnership() declaration in
+      // base/files/scoped_file.h regarding why this is called early here.
+      subtle::ResetFDOwnership();
+#endif
+
       // Obscure fork() rule: in the child, if you don't end up doing exec*(),
       // you call _exit() instead of exit(). This is because _exit() does not
       // call any previously-registered (in the parent) exit handlers, which

content/app/content_main.cc

@@ -62,6 +62,10 @@
 #include "base/posix/global_descriptors.h"
 #endif
 
+#if defined(OS_CHROMEOS) || defined(OS_LINUX)
+#include "base/files/scoped_file.h"
+#endif
+
 #if defined(OS_MAC)
 #include "base/mac/scoped_nsautorelease_pool.h"
 #include "content/app/mac_init.h"
@@ -304,6 +308,10 @@ int RunContentProcess(const ContentMainParams& params,
     InitializeMac();
 #endif
 
+#if defined(OS_CHROMEOS) || defined(OS_LINUX)
+    base::subtle::EnableFDOwnershipEnforcement(true);
+#endif
+
     mojo::core::Configuration mojo_config;
     mojo_config.max_message_num_bytes = kMaximumMojoMessageSize;
     InitializeMojo(&mojo_config);

printing/backend/cups_helper.cc

@@ -15,7 +15,6 @@
 #include "base/files/file_util.h"
 #include "base/files/scoped_file.h"
 #include "base/logging.h"
-#include "base/macros.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
 #include "base/time/time.h"
@@ -598,18 +597,21 @@ bool ParsePpdCapabilities(cups_dest_t* dest,
     return false;
   }
 
-  ppd_file_t* ppd = ppdOpenFd(ppd_fd.get());
+  // We release ownership of `ppd_fd` here because ppdOpenFd() assumes ownership
+  // of it in all but one case (see below).
+  int unowned_ppd_fd = ppd_fd.release();
+  ppd_file_t* ppd = ppdOpenFd(unowned_ppd_fd);
   if (!ppd) {
     int line = 0;
     ppd_status_t ppd_status = ppdLastError(&line);
     LOG(ERROR) << "Failed to open PDD file: error " << ppd_status << " at line "
                << line << ", " << ppdErrorString(ppd_status);
-    if (ppd_status != PPD_FILE_OPEN_ERROR) {
-      // When the error is not from opening the file then the CUPS library
-      // internals will have already closed the file descriptor.  It is
-      // important to not close the file a second time (when ScopedFD destructor
-      // fires), so we release the descriptor prior to that.
-      ignore_result(ppd_fd.release());
+    if (ppd_status == PPD_FILE_OPEN_ERROR) {
+      // Normally ppdOpenFd assumes ownership of the file descriptor we give it,
+      // regardless of success or failure. The one exception is when it fails
+      // with PPD_FILE_OPEN_ERROR. In that case ownership is retained by the
+      // caller, so we must explicitly close it.
+      close(unowned_ppd_fd);
     }
     return false;
   }
@@ -693,10 +695,6 @@ bool ParsePpdCapabilities(cups_dest_t* dest,
   }
 
   ppdClose(ppd);
-  // The CUPS library internals close the file descriptor upon successfully
-  // reading it.  Explicitly release the `ScopedFD` to prevent a crash caused
-  // by a bad file descriptor.
-  ignore_result(ppd_fd.release());
 
   *printer_info = caps;
   return true;

third_party/crashpad/README.chromium

@@ -42,3 +42,5 @@ Local Modifications:
  - MemoryMap.SelfLargeMapFile, SelfBasic, SelfLargeFiles are disabled when
    BUILDFLAG(USE_PARTITION_ALLOC_AS_MALLOC) are defined. crbug.com/1163794
    (crashpad/util/BUILD.gn, crashpad/util/linux/memory_map_test.cc)
+ - CloseMultipleNowOrOnExec has been updated to invoke the new
+   base::subtle::ResetFDOwnership() API

third_party/crashpad/crashpad/util/posix/close_multiple.cc

@@ -109,6 +109,12 @@ bool CloseMultipleNowOrOnExecUsingFDDir(int min_fd, int preserve_fd) {
 }  // namespace
 
 void CloseMultipleNowOrOnExec(int fd, int preserve_fd) {
+#if defined(OS_LINUX) || defined(OS_CHROMEOS)
+  // See comments on the ResetFDOwnership() declaration in
+  // base/files/scoped_file.h regarding why this is called here.
+  base::subtle::ResetFDOwnership();
+#endif
+
   if (CloseMultipleNowOrOnExecUsingFDDir(fd, preserve_fd)) {
     return;
   }