Intercept base::File Open/Close

When a file descriptor is opened by the base::File, all calls to close(3) from the same dynamic library will hit a CHECK unless they are made from a whitelist of callsites belonging to base::File.

There is a handy protect_file_posix.gypi introduced to make it easy to enable on Chrome-for-Android.

This 'linker magic' is somewhat crazy, so:
1. it will be *removed *when crbug.com/424562 is fixed
2. it should only be used by a whitelist of binaries/libraries (in the opensource part: libchromeshell only)

BUG=424562

Review URL: https://codereview.chromium.org/676873004

Cr-Commit-Position: refs/heads/master@{#304592}

7 files changed, 213 insertions, 1 deletions

diff --git a/base/BUILD.gn b/base/BUILD.gn
index 959d462..2cce16c 100644
--- a/base/BUILD.gn
+++ b/base/BUILD.gn
@@ -1108,6 +1108,13 @@ source_set("message_loop_tests") {
   ]
 }
 
+# TODO(pasko): Remove this target when crbug.com/424562 is fixed.
+source_set("protect_file_posix") {
+  sources = [
+    "files/protect_file_posix.cc",
+  ]
+}
+
 test("base_unittests") {
   sources = [
     "android/application_status_listener_unittest.cc",
diff --git a/base/base.gyp b/base/base.gyp
index ad4dd22..e31cf18 100644
--- a/base/base.gyp
+++ b/base/base.gyp
@@ -377,6 +377,18 @@
       ],
     },
     {
+      # TODO(pasko): Remove this target when crbug.com/424562 is fixed.
+      # GN: //base:protect_file_posix
+      'target_name': 'protect_file_posix',
+      'type': 'static_library',
+      'dependencies': [
+        'base',
+      ],
+      'sources': [
+        'files/protect_file_posix.cc',
+      ],
+    },
+    {
       'target_name': 'base_prefs_test_support',
       'type': 'static_library',
       'dependencies': [
diff --git a/base/files/file.cc b/base/files/file.cc
index ea8dbf2..a997074 100644
--- a/base/files/file.cc
+++ b/base/files/file.cc
@@ -5,6 +5,10 @@
 #include "base/files/file.h"
 #include "base/files/file_path.h"
 
+#if defined(OS_POSIX)
+#include "base/files/file_posix_hooks_internal.h"
+#endif
+
 namespace base {
 
 File::Info::Info()
@@ -38,6 +42,8 @@ File::File(PlatformFile platform_file)
       async_(false) {
 #if defined(OS_POSIX)
   DCHECK_GE(platform_file, -1);
+  if (IsValid())
+    ProtectFileDescriptor(platform_file);
 #endif
 }
 
@@ -52,6 +58,10 @@ File::File(RValue other)
       error_details_(other.object->error_details()),
       created_(other.object->created()),
       async_(other.object->async_) {
+#if defined(OS_POSIX)
+   if (IsValid())
+     ProtectFileDescriptor(GetPlatformFile());
+#endif
 }
 
 File::~File() {
diff --git a/base/files/file_posix.cc b/base/files/file_posix.cc
index 3d229e4..245ea6a 100644
--- a/base/files/file_posix.cc
+++ b/base/files/file_posix.cc
@@ -10,6 +10,7 @@
 #include <unistd.h>
 
 #include "base/files/file_path.h"
+#include "base/files/file_posix_hooks_internal.h"
 #include "base/logging.h"
 #include "base/metrics/sparse_histogram.h"
 #include "base/posix/eintr_wrapper.h"
@@ -166,6 +167,14 @@ void File::Info::FromStat(const stat_wrapper_t& stat_info) {
                                   Time::kNanosecondsPerMicrosecond);
 }
 
+// Default implementations of Protect/Unprotect hooks defined as weak symbols
+// where possible.
+void ProtectFileDescriptor(int fd) {
+}
+
+void UnprotectFileDescriptor(int fd) {
+}
+
 // NaCl doesn't implement system calls to open files directly.
 #if !defined(OS_NACL)
 // TODO(erikkay): does it make sense to support FLAG_EXCLUSIVE_* here?
@@ -252,6 +261,7 @@ void File::InitializeUnsafe(const FilePath& name, uint32 flags) {
   async_ = ((flags & FLAG_ASYNC) == FLAG_ASYNC);
   error_details_ = FILE_OK;
   file_.reset(descriptor);
+  ProtectFileDescriptor(descriptor);
 }
 #endif  // !defined(OS_NACL)
 
@@ -264,6 +274,8 @@ PlatformFile File::GetPlatformFile() const {
 }
 
 PlatformFile File::TakePlatformFile() {
+  if (IsValid())
+    UnprotectFileDescriptor(GetPlatformFile());
   return file_.release();
 }
 
@@ -272,6 +284,7 @@ void File::Close() {
     return;
 
   base::ThreadRestrictions::AssertIOAllowed();
+  UnprotectFileDescriptor(GetPlatformFile());
   file_.reset();
 }
 
@@ -527,8 +540,10 @@ void File::MemoryCheckingScopedFD::UpdateChecksum() {
 }
 
 void File::SetPlatformFile(PlatformFile file) {
-  DCHECK(!file_.is_valid());
+  CHECK(!file_.is_valid());
   file_.reset(file);
+  if (file_.is_valid())
+    ProtectFileDescriptor(GetPlatformFile());
 }
 
 }  // namespace base
diff --git a/base/files/file_posix_hooks_internal.h b/base/files/file_posix_hooks_internal.h
new file mode 100644
index 0000000..1137b48
--- /dev/null
+++ b/base/files/file_posix_hooks_internal.h
@@ -0,0 +1,31 @@
+// Copyright 2014 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.
+
+#ifndef BASE_FILES_FILE_POSIX_HOOKS_INTERNAL_H_
+#define BASE_FILES_FILE_POSIX_HOOKS_INTERNAL_H_
+
+#include "base/base_export.h"
+
+namespace base {
+
+// Define empty hooks for blacklisting file descriptors used in base::File.
+// These functions should be declared 'weak', i.e. the functions declared in
+// a default way would have precedence over the weak ones at link time. This
+// works for both static and dynamic linking.
+// TODO(pasko): Remove these hooks when crbug.com/424562 is fixed.
+//
+// With compilers other than GCC/Clang define strong no-op symbols for
+// simplicity.
+#if defined(COMPILER_GCC)
+#define ATTRIBUTE_WEAK __attribute__ ((weak))
+#else
+#define ATTRIBUTE_WEAK
+#endif
+BASE_EXPORT void ProtectFileDescriptor(int fd) ATTRIBUTE_WEAK;
+BASE_EXPORT void UnprotectFileDescriptor(int fd) ATTRIBUTE_WEAK;
+#undef ATTRIBUTE_WEAK
+
+}  // namespace base
+
+#endif  // BASE_FILES_FILE_POSIX_HOOKS_INTERNAL_H_
diff --git a/base/files/protect_file_posix.cc b/base/files/protect_file_posix.cc
new file mode 100644
index 0000000..e4753c4
--- /dev/null
+++ b/base/files/protect_file_posix.cc
@@ -0,0 +1,106 @@
+// Copyright 2014 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/containers/hash_tables.h"
+#include "base/files/file.h"
+#include "base/lazy_instance.h"
+#include "base/logging.h"
+#include "base/posix/eintr_wrapper.h"
+#include "base/synchronization/lock.h"
+
+// These hooks provided for base::File perform additional sanity checks when
+// files are closed. These extra checks are hard to understand and maintain,
+// hence they are temporary. TODO(pasko): Remove these extra checks as soon as
+// crbug.com/424562 is fixed.
+//
+// Background:
+//   1. The browser process crashes if a call to close() provided by the C
+//      library (i.e. close(3)) fails. This is done for security purposes. See
+//      base/files/scoped_file.cc. When a crash like this happens, there is not
+//      enough context in the minidump to triage the problem.
+//   2. base::File provides a good abstraction to prevent closing incorrect
+//      file descriptors or double-closing. Closing non-owned file descriptors
+//      would more likely happen from outside base::File and base::ScopedFD.
+//
+// These hooks intercept base::File operations to 'protect' file handles /
+// descriptors from accidental close(3) by other portions of the code being
+// linked into the browser. Also, this file provides an interceptor for the
+// close(3) itself, and requires to be linked with cooperation of
+// --Wl,--wrap=close (i.e. linker wrapping).
+//
+// Wrapping close(3) on all libraries can lead to confusion, particularly for
+// the libraries that do not use ::base (I am also looking at you,
+// crazy_linker). Instead two hooks are added to base::File, which are
+// implemented as no-op by default. This file should be linked into the Chrome
+// native binary(-ies) for a whitelist of targets where "file descriptor
+// protection" is useful.
+
+// With compilers other than GCC/Clang the wrapper is trivial. This is to avoid
+// overexercising mechanisms for overriding weak symbols.
+#if !defined(COMPILER_GCC)
+extern "C" {
+
+int __real_close(int fd);
+
+BASE_EXPORT int __wrap_close(int fd) {
+  return __real_close(fd);
+}
+
+}  // extern "C"
+
+#else  // defined(COMPILER_GCC)
+
+namespace {
+
+// Protects the |g_protected_fd_set|.
+base::LazyInstance<base::Lock>::Leaky g_lock = LAZY_INSTANCE_INITIALIZER;
+
+// Holds the set of all 'protected' file descriptors.
+base::LazyInstance<base::hash_set<int> >::Leaky g_protected_fd_set =
+    LAZY_INSTANCE_INITIALIZER;
+
+bool IsFileDescriptorProtected(int fd) {
+  base::AutoLock lock(*g_lock.Pointer());
+  return g_protected_fd_set.Get().count(fd) != 0;
+}
+
+}  // namespace
+
+namespace base {
+
+BASE_EXPORT void ProtectFileDescriptor(int fd) {
+  base::AutoLock lock(g_lock.Get());
+  CHECK(!g_protected_fd_set.Get().count(fd)) << "fd: " << fd;
+  g_protected_fd_set.Get().insert(fd);
+}
+
+BASE_EXPORT void UnprotectFileDescriptor(int fd) {
+  base::AutoLock lock(*g_lock.Pointer());
+  CHECK(g_protected_fd_set.Get().erase(fd)) << "fd: " << fd;
+}
+
+}  // namespace base
+
+extern "C" {
+
+int __real_close(int fd);
+
+BASE_EXPORT int __wrap_close(int fd) {
+  // The crash happens here if a protected file descriptor was attempted to be
+  // closed without first being unprotected. Unprotection happens only in
+  // base::File. In other words this is an "early crash" as compared to the one
+  // happening in scoped_file.cc.
+  //
+  // Getting an earlier crash provides a more useful stack trace (minidump)
+  // allowing to debug deeper into the thread that freed the wrong resource.
+  CHECK(!IsFileDescriptorProtected(fd)) << "fd: " << fd;
+
+  // Crash by the same reason as in scoped_file.cc.
+  PCHECK(0 == IGNORE_EINTR(__real_close(fd)));
+  return 0;
+}
+
+}  // extern "C"
+
+#endif  // defined(COMPILER_GCC)
diff --git a/base/files/protect_file_posix.gypi b/base/files/protect_file_posix.gypi
new file mode 100644
index 0000000..017fd87
--- /dev/null
+++ b/base/files/protect_file_posix.gypi
@@ -0,0 +1,31 @@
+# Copyright 2014 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.
+
+# Provides sanity-checks and early crashes on some improper use of posix file
+# descriptors. See protect_file_posix.cc for details.
+#
+# Usage:
+#   {
+#     'target_name': 'libsomething',
+#     'type': 'shared_library',  // Do *not* use it for static libraries.
+#     'includes': [
+#       'base/files/protect_file_posix.gypi',
+#     ],
+#     ...
+#   }
+{
+   'conditions': [
+     # In the component build the interceptors have to be declared with
+     # non-hidden visibility, which is not desirable for the release build.
+     # Disable the extra checks for the component build for simplicity.
+     ['component != "shared_library"', {
+       'ldflags': [
+         '-Wl,--wrap=close',
+       ],
+       'dependencies': [
+         '<(DEPTH)/base/base.gyp:protect_file_posix',
+       ],
+     }],
+   ],
+}