Update from https://crrev.com/304715 Includes patches to sky/viewer/cc/ and ui/compositor/ for the following: cc: Toggle LCD text at raster time instead of record time. https://codereview.chromium.org/684543006 https://crrev.com/304623 and Make Keyframe use TimeTicks/TimeDelta to represent time instead of double. https://codereview.chromium.org/719453007 https://crrev.com/304612 Review URL: https://codereview.chromium.org/737943002
diff --git a/BUILD.gn b/BUILD.gn index 959d462..2cce16c 100644 --- a/BUILD.gn +++ b/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.gyp b/base.gyp index ad4dd22..e31cf18 100644 --- a/base.gyp +++ b/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/files/file.cc b/files/file.cc index ea8dbf2..a997074 100644 --- a/files/file.cc +++ b/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/files/file_posix.cc b/files/file_posix.cc index 3d229e4..245ea6a 100644 --- a/files/file_posix.cc +++ b/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/files/file_posix_hooks_internal.h b/files/file_posix_hooks_internal.h new file mode 100644 index 0000000..1137b48 --- /dev/null +++ b/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/files/protect_file_posix.cc b/files/protect_file_posix.cc new file mode 100644 index 0000000..e4753c4 --- /dev/null +++ b/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/files/protect_file_posix.gypi b/files/protect_file_posix.gypi new file mode 100644 index 0000000..017fd87 --- /dev/null +++ b/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/process/process.h b/process/process.h index 7019474..50ccf8d 100644 --- a/process/process.h +++ b/process/process.h
@@ -48,6 +48,12 @@ // Returns an object for the current process. static Process Current(); + // Creates an object from a |handle| owned by someone else. + // Don't use this for new code. It is only intended to ease the migration to + // a strict ownership model. + // TODO(rvargas) crbug.com/417532: Remove this code. + static Process DeprecatedGetProcessFromHandle(ProcessHandle handle); + // Returns true if processes can be backgrounded. static bool CanBackgroundProcesses();
diff --git a/process/process_posix.cc b/process/process_posix.cc index ea8fd8c..270438e 100644 --- a/process/process_posix.cc +++ b/process/process_posix.cc
@@ -37,6 +37,12 @@ return process.Pass(); } +// static +Process Process::DeprecatedGetProcessFromHandle(ProcessHandle handle) { + DCHECK_NE(handle, GetCurrentProcessHandle()); + return Process(handle); +} + #if !defined(OS_LINUX) // static bool Process::CanBackgroundProcesses() {
diff --git a/process/process_unittest.cc b/process/process_unittest.cc index 66d6e63..a5ba834 100644 --- a/process/process_unittest.cc +++ b/process/process_unittest.cc
@@ -92,6 +92,21 @@ ASSERT_TRUE(process2.IsValid()); } +TEST_F(ProcessTest, DeprecatedGetProcessFromHandle) { + Process process1(SpawnChild("SimpleChildProcess")); + ASSERT_TRUE(process1.IsValid()); + + Process process2 = Process::DeprecatedGetProcessFromHandle(process1.Handle()); + ASSERT_TRUE(process1.IsValid()); + ASSERT_TRUE(process2.IsValid()); + EXPECT_EQ(process1.pid(), process2.pid()); + EXPECT_FALSE(process1.is_current()); + EXPECT_FALSE(process2.is_current()); + + process1.Close(); + ASSERT_TRUE(process2.IsValid()); +} + MULTIPROCESS_TEST_MAIN(SleepyChildProcess) { PlatformThread::Sleep(TestTimeouts::action_max_timeout()); return 0;
diff --git a/process/process_win.cc b/process/process_win.cc index 05041b2..3e0d79f 100644 --- a/process/process_win.cc +++ b/process/process_win.cc
@@ -39,6 +39,18 @@ } // static +Process Process::DeprecatedGetProcessFromHandle(ProcessHandle handle) { + DCHECK_NE(handle, ::GetCurrentProcess()); + ProcessHandle out_handle; + if (!::DuplicateHandle(GetCurrentProcess(), handle, + GetCurrentProcess(), &out_handle, + 0, FALSE, DUPLICATE_SAME_ACCESS)) { + return Process(); + } + return Process(out_handle); +} + +// static bool Process::CanBackgroundProcesses() { return true; }