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}
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 @@
]
}
+# 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 ad4dd226..e31cf185 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 @@
async_(false) {
#if defined(OS_POSIX)
DCHECK_GE(platform_file, -1);
+ if (IsValid())
+ ProtectFileDescriptor(platform_file);
#endif
}
@@ -52,6 +58,10 @@
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 3d229e41..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 @@
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 @@
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::TakePlatformFile() {
+ if (IsValid())
+ UnprotectFileDescriptor(GetPlatformFile());
return file_.release();
}
@@ -272,6 +284,7 @@
return;
base::ThreadRestrictions::AssertIOAllowed();
+ UnprotectFileDescriptor(GetPlatformFile());
file_.reset();
}
@@ -527,8 +540,10 @@
}
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..1137b487
--- /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..017fd875
--- /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',
+ ],
+ }],
+ ],
+}
diff --git a/chrome/android/BUILD.gn b/chrome/android/BUILD.gn
index b9ee059..b4660db 100644
--- a/chrome/android/BUILD.gn
+++ b/chrome/android/BUILD.gn
@@ -198,6 +198,13 @@
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
diff --git a/chrome/chrome_shell.gypi b/chrome/chrome_shell.gypi
index 1a65dc8..3f546893 100644
--- a/chrome/chrome_shell.gypi
+++ b/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
