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}

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",

base/base.gyp

@@ -376,6 +376,18 @@
         '../build/android/increase_size_for_speed.gypi',
       ],
     },
+    {
+      # 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',

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() {

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

base/files/file_posix_hooks_internal.h

+// 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_

base/files/protect_file_posix.cc

+// 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)

base/files/protect_file_posix.gypi

+# 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',
+       ],
+     }],
+   ],
+}

chrome/android/BUILD.gn

@@ -198,6 +198,13 @@ shared_library("chrome_shell") {
   deps = [
     ":chrome_shell_base",
   ]
+  # GYP: via base/files/protect_file_posix.gypi
+  # TODO(pasko): Remove this non-trivial linker wrapping as soon as
+  # crbug.com/424562 is fixed.
+  if (!is_component_build) {
+    ldflags = [ "-Wl,--wrap=close" ]
+    deps += [ "//base:protect_file_posix" ]
+  }
 }
 
 # GYP: //chrome/chrome_shell.gypi:libchromesyncshell

chrome/chrome_shell.gypi

@@ -70,6 +70,11 @@
       'dependencies': [
         'libchromeshell_base',
       ],
+      'includes': [
+        # File 'protection' is based on non-trivial linker magic. TODO(pasko):
+        # remove it when crbug.com/424562 is fixed.
+        '../base/files/protect_file_posix.gypi',
+      ],
     },
     {
       # GN: //chrome/android:chrome_sync_shell