Implement ScopedFD in terms of ScopedGeneric.
Move to a new file base/files/scoped_file.h. I will also add ScopedFILE to here (currently in file_util.h) later.
I think there is a crash in the old code in content/browser/zygote_host/zygote_host_impl_linux.cc that this patch should fix. The old ScopedFD took the address of something in a vector that is being modified.
I removed SafeScopedFD from content/common/sandbox_linux/sandbox_linux.cc since base's ScopedFD not CHECKs on close failure (this is a more recent addition).
Reland of https://codereview.chromium.org/191673003/
R=agl, viettrungluu
Review URL: https://codereview.chromium.org/202113004
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@257473 0039d316-1c4b-4281-b951-d872f2087c98
CrOS-Libchrome-Original-Commit: 42f558fdef03f1ec2261f554151d8a4d168919e4
diff --git a/base/base.gypi b/base/base.gypi
index 1d2f5c1..6a0b3a6 100644
--- a/base/base.gypi
+++ b/base/base.gypi
@@ -221,6 +221,8 @@
'files/memory_mapped_file.h',
'files/memory_mapped_file_posix.cc',
'files/memory_mapped_file_win.cc',
+ 'files/scoped_file.cc',
+ 'files/scoped_file.h',
'files/scoped_platform_file_closer.cc',
'files/scoped_platform_file_closer.h',
'files/scoped_temp_dir.cc',
diff --git a/base/debug/proc_maps_linux.cc b/base/debug/proc_maps_linux.cc
index b7a5862..a956961 100644
--- a/base/debug/proc_maps_linux.cc
+++ b/base/debug/proc_maps_linux.cc
@@ -11,6 +11,7 @@
#endif
#include "base/file_util.h"
+#include "base/files/scoped_file.h"
#include "base/strings/string_split.h"
#if defined(OS_ANDROID)
@@ -45,12 +46,11 @@
// file for details.
const long kReadSize = sysconf(_SC_PAGESIZE);
- int fd = HANDLE_EINTR(open("/proc/self/maps", O_RDONLY));
- if (fd == -1) {
+ base::ScopedFD fd(HANDLE_EINTR(open("/proc/self/maps", O_RDONLY)));
+ if (!fd.is_valid()) {
DPLOG(ERROR) << "Couldn't open /proc/self/maps";
return false;
}
- file_util::ScopedFD fd_closer(&fd);
proc_maps->clear();
while (true) {
@@ -60,7 +60,7 @@
proc_maps->resize(pos + kReadSize);
void* buffer = &(*proc_maps)[pos];
- ssize_t bytes_read = HANDLE_EINTR(read(fd, buffer, kReadSize));
+ ssize_t bytes_read = HANDLE_EINTR(read(fd.get(), buffer, kReadSize));
if (bytes_read < 0) {
DPLOG(ERROR) << "Couldn't read /proc/self/maps";
proc_maps->clear();
diff --git a/base/file_util.h b/base/file_util.h
index 431569a..b86d8cb 100644
--- a/base/file_util.h
+++ b/base/file_util.h
@@ -426,32 +426,6 @@
// Automatically closes |FILE*|s.
typedef scoped_ptr<FILE, ScopedFILEClose> ScopedFILE;
-#if defined(OS_POSIX)
-// Functor for |ScopedFD| (below).
-struct ScopedFDClose {
- inline void operator()(int* x) const {
- if (x && *x >= 0) {
- // It's important to crash here.
- // There are security implications to not closing a file descriptor
- // properly. As file descriptors are "capabilities", keeping them open
- // would make the current process keep access to a resource. Much of
- // Chrome relies on being able to "drop" such access.
- // It's especially problematic on Linux with the setuid sandbox, where
- // a single open directory would bypass the entire security model.
- PCHECK(0 == IGNORE_EINTR(close(*x)));
- }
- }
-};
-
-// Automatically closes FDs (note: doesn't store the FD).
-// TODO(viettrungluu): This is a very odd API, since (unlike |FILE*|s, you'll
-// need to store the FD separately and keep its memory alive). This should
-// probably be called |ScopedFDCloser| or something like that.
-typedef scoped_ptr<int, ScopedFDClose> ScopedFD;
-// Let new users use ScopedFDCloser already, while ScopedFD is replaced.
-typedef ScopedFD ScopedFDCloser;
-#endif // OS_POSIX
-
} // namespace file_util
// Internal --------------------------------------------------------------------
diff --git a/base/file_util_posix.cc b/base/file_util_posix.cc
index 7e7dc05..fb4ebcf 100644
--- a/base/file_util_posix.cc
+++ b/base/file_util_posix.cc
@@ -33,6 +33,7 @@
#include "base/basictypes.h"
#include "base/files/file_enumerator.h"
#include "base/files/file_path.h"
+#include "base/files/scoped_file.h"
#include "base/logging.h"
#include "base/memory/scoped_ptr.h"
#include "base/memory/singleton.h"
@@ -172,15 +173,15 @@
bool DetermineDevShmExecutable() {
bool result = false;
FilePath path;
- int fd = CreateAndOpenFdForTemporaryFile(FilePath("/dev/shm"), &path);
- if (fd >= 0) {
- file_util::ScopedFD shm_fd_closer(&fd);
+
+ ScopedFD fd(CreateAndOpenFdForTemporaryFile(FilePath("/dev/shm"), &path));
+ if (fd.is_valid()) {
DeleteFile(path, false);
long sysconf_result = sysconf(_SC_PAGESIZE);
CHECK_GE(sysconf_result, 0);
size_t pagesize = static_cast<size_t>(sysconf_result);
CHECK_GE(sizeof(pagesize), sizeof(sysconf_result));
- void *mapping = mmap(NULL, pagesize, PROT_READ, MAP_SHARED, fd, 0);
+ void *mapping = mmap(NULL, pagesize, PROT_READ, MAP_SHARED, fd.get(), 0);
if (mapping != MAP_FAILED) {
if (mprotect(mapping, pagesize, PROT_READ | PROT_EXEC) == 0)
result = true;
@@ -660,11 +661,10 @@
stat_wrapper_t file_info;
#if defined(OS_ANDROID)
if (file_path.IsContentUri()) {
- int fd = OpenContentUriForRead(file_path);
- if (fd < 0)
+ ScopedFD fd(OpenContentUriForRead(file_path));
+ if (!fd.is_valid())
return false;
- file_util::ScopedFD scoped_fd(&fd);
- if (CallFstat(fd, &file_info) != 0)
+ if (CallFstat(fd.get(), &file_info) != 0)
return false;
} else {
#endif // defined(OS_ANDROID)
diff --git a/base/file_util_unittest.cc b/base/file_util_unittest.cc
index 96b5858..8ee55ff 100644
--- a/base/file_util_unittest.cc
+++ b/base/file_util_unittest.cc
@@ -26,6 +26,7 @@
#include "base/file_util.h"
#include "base/files/file_enumerator.h"
#include "base/files/file_path.h"
+#include "base/files/scoped_file.h"
#include "base/files/scoped_temp_dir.h"
#include "base/path_service.h"
#include "base/strings/utf_string_conversions.h"
@@ -2473,9 +2474,9 @@
char c = 0;
ASSERT_EQ(0, pipe(fds));
const int write_end = fds[1];
- file_util::ScopedFDCloser read_end_closer(fds);
+ base::ScopedFD read_end_closer(fds[0]);
{
- file_util::ScopedFDCloser write_end_closer(fds + 1);
+ base::ScopedFD write_end_closer(fds[1]);
}
// This is the only thread. This file descriptor should no longer be valid.
int ret = close(write_end);
@@ -2489,14 +2490,14 @@
#if defined(GTEST_HAS_DEATH_TEST)
void CloseWithScopedFD(int fd) {
- file_util::ScopedFDCloser fd_closer(&fd);
+ base::ScopedFD fd_closer(fd);
}
#endif
TEST(ScopedFD, ScopedFDCrashesOnCloseFailure) {
int fds[2];
ASSERT_EQ(0, pipe(fds));
- file_util::ScopedFDCloser read_end_closer(fds);
+ base::ScopedFD read_end_closer(fds[0]);
EXPECT_EQ(0, IGNORE_EINTR(close(fds[1])));
#if defined(GTEST_HAS_DEATH_TEST)
// This is the only thread. This file descriptor should no longer be valid.
diff --git a/base/files/scoped_file.cc b/base/files/scoped_file.cc
new file mode 100644
index 0000000..39f064d
--- /dev/null
+++ b/base/files/scoped_file.cc
@@ -0,0 +1,35 @@
+// 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/files/scoped_file.h"
+
+#include "base/logging.h"
+
+#if defined(OS_POSIX)
+#include <unistd.h>
+
+#include "base/posix/eintr_wrapper.h"
+#endif
+
+namespace base {
+namespace internal {
+
+#if defined(OS_POSIX)
+
+// static
+void ScopedFDCloseTraits::Free(int fd) {
+ // It's important to crash here.
+ // There are security implications to not closing a file descriptor
+ // properly. As file descriptors are "capabilities", keeping them open
+ // would make the current process keep access to a resource. Much of
+ // Chrome relies on being able to "drop" such access.
+ // It's especially problematic on Linux with the setuid sandbox, where
+ // a single open directory would bypass the entire security model.
+ PCHECK(0 == IGNORE_EINTR(close(fd)));
+}
+
+#endif // OS_POSIX
+
+} // namespace internal
+} // namespace base
diff --git a/base/files/scoped_file.h b/base/files/scoped_file.h
new file mode 100644
index 0000000..f5d7ecd
--- /dev/null
+++ b/base/files/scoped_file.h
@@ -0,0 +1,47 @@
+// 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_SCOPED_FILE_H_
+#define BASE_FILES_SCOPED_FILE_H_
+
+#include <stdio.h>
+
+#include "base/base_export.h"
+#include "base/logging.h"
+#include "base/scoped_generic.h"
+#include "build/build_config.h"
+
+namespace base {
+
+namespace internal {
+
+#if defined(OS_POSIX)
+struct BASE_EXPORT ScopedFDCloseTraits {
+ static int InvalidValue() {
+ return -1;
+ }
+ static void Free(int fd);
+};
+#endif
+
+} // namespace internal
+
+#if defined(OS_POSIX)
+// A low-level Posix file descriptor closer class. Use this when writing
+// platform-specific code, especially that does non-file-like things with the
+// FD (like sockets).
+//
+// If you're writing low-level Windows code, see base/win/scoped_handle.h
+// which provides some additional functionality.
+//
+// If you're writing cross-platform code that deals with actual files, you
+// should generally use base::File instead which can be constructed with a
+// handle, and in addition to handling ownership, has convenient cross-platform
+// file manipulation functions on it.
+typedef ScopedGeneric<int, internal::ScopedFDCloseTraits> ScopedFD;
+#endif
+
+} // namespace base
+
+#endif // BASE_FILES_SCOPED_FILE_H_
diff --git a/base/memory/discardable_memory_allocator_android.cc b/base/memory/discardable_memory_allocator_android.cc
index 97e9abd..1588317 100644
--- a/base/memory/discardable_memory_allocator_android.cc
+++ b/base/memory/discardable_memory_allocator_android.cc
@@ -16,6 +16,7 @@
#include "base/basictypes.h"
#include "base/containers/hash_tables.h"
#include "base/file_util.h"
+#include "base/files/scoped_file.h"
#include "base/logging.h"
#include "base/memory/discardable_memory.h"
#include "base/memory/scoped_vector.h"
@@ -65,14 +66,13 @@
size_t size,
int* out_fd,
void** out_address) {
- int fd = ashmem_create_region(name, size);
- if (fd < 0) {
+ base::ScopedFD fd(ashmem_create_region(name, size));
+ if (!fd.is_valid()) {
DLOG(ERROR) << "ashmem_create_region() failed";
return false;
}
- file_util::ScopedFD fd_closer(&fd);
- const int err = ashmem_set_prot_region(fd, PROT_READ | PROT_WRITE);
+ const int err = ashmem_set_prot_region(fd.get(), PROT_READ | PROT_WRITE);
if (err < 0) {
DLOG(ERROR) << "Error " << err << " when setting protection of ashmem";
return false;
@@ -82,14 +82,13 @@
// Lock() and Unlock(), data could get lost if they are not written to the
// underlying file when Unlock() gets called.
void* const address = mmap(
- NULL, size, PROT_READ | PROT_WRITE, MAP_SHARED, fd, 0);
+ NULL, size, PROT_READ | PROT_WRITE, MAP_SHARED, fd.get(), 0);
if (address == MAP_FAILED) {
DPLOG(ERROR) << "Failed to map memory.";
return false;
}
- ignore_result(fd_closer.release());
- *out_fd = fd;
+ *out_fd = fd.release();
*out_address = address;
return true;
}
diff --git a/base/memory/shared_memory.h b/base/memory/shared_memory.h
index 50b61c4..8e32e72 100644
--- a/base/memory/shared_memory.h
+++ b/base/memory/shared_memory.h
@@ -22,6 +22,7 @@
#if defined(OS_POSIX)
#include "base/file_descriptor_posix.h"
#include "base/file_util.h"
+#include "base/files/scoped_file.h"
#endif
namespace base {
@@ -259,7 +260,7 @@
private:
#if defined(OS_POSIX) && !defined(OS_NACL)
- bool PrepareMapFile(file_util::ScopedFILE fp, file_util::ScopedFD readonly);
+ bool PrepareMapFile(file_util::ScopedFILE fp, base::ScopedFD readonly);
bool FilePathForMemoryName(const std::string& mem_name, FilePath* path);
void LockOrUnlockCommon(int function);
#endif
diff --git a/base/memory/shared_memory_posix.cc b/base/memory/shared_memory_posix.cc
index 8780c04..1a90847 100644
--- a/base/memory/shared_memory_posix.cc
+++ b/base/memory/shared_memory_posix.cc
@@ -30,7 +30,6 @@
#include "third_party/ashmem/ashmem.h"
#endif
-using file_util::ScopedFD;
using file_util::ScopedFILE;
namespace base {
@@ -132,8 +131,7 @@
ScopedFILE fp;
bool fix_size = true;
- int readonly_fd_storage = -1;
- ScopedFD readonly_fd(&readonly_fd_storage);
+ ScopedFD readonly_fd;
FilePath path;
if (options.name_deprecated == NULL || options.name_deprecated->empty()) {
@@ -145,8 +143,8 @@
if (fp) {
// Also open as readonly so that we can ShareReadOnlyToProcess.
- *readonly_fd = HANDLE_EINTR(open(path.value().c_str(), O_RDONLY));
- if (*readonly_fd < 0) {
+ readonly_fd.reset(HANDLE_EINTR(open(path.value().c_str(), O_RDONLY)));
+ if (!readonly_fd.is_valid()) {
DPLOG(ERROR) << "open(\"" << path.value() << "\", O_RDONLY) failed";
fp.reset();
}
@@ -198,8 +196,8 @@
}
// Also open as readonly so that we can ShareReadOnlyToProcess.
- *readonly_fd = HANDLE_EINTR(open(path.value().c_str(), O_RDONLY));
- if (*readonly_fd < 0) {
+ readonly_fd.reset(HANDLE_EINTR(open(path.value().c_str(), O_RDONLY)));
+ if (!readonly_fd.is_valid()) {
DPLOG(ERROR) << "open(\"" << path.value() << "\", O_RDONLY) failed";
close(fd);
fd = -1;
@@ -265,10 +263,8 @@
const char *mode = read_only ? "r" : "r+";
ScopedFILE fp(base::OpenFile(path, mode));
- int readonly_fd_storage = -1;
- ScopedFD readonly_fd(&readonly_fd_storage);
- *readonly_fd = HANDLE_EINTR(open(path.value().c_str(), O_RDONLY));
- if (*readonly_fd < 0) {
+ ScopedFD readonly_fd(HANDLE_EINTR(open(path.value().c_str(), O_RDONLY)));
+ if (!readonly_fd.is_valid()) {
DPLOG(ERROR) << "open(\"" << path.value() << "\", O_RDONLY) failed";
}
return PrepareMapFile(fp.Pass(), readonly_fd.Pass());
@@ -353,7 +349,7 @@
bool SharedMemory::PrepareMapFile(ScopedFILE fp, ScopedFD readonly_fd) {
DCHECK_EQ(-1, mapped_file_);
DCHECK_EQ(-1, readonly_mapped_file_);
- if (fp == NULL || *readonly_fd < 0) return false;
+ if (fp == NULL || !readonly_fd.is_valid()) return false;
// This function theoretically can block on the disk, but realistically
// the temporary files we create will just go into the buffer cache
@@ -364,7 +360,7 @@
struct stat readonly_st = {};
if (fstat(fileno(fp.get()), &st))
NOTREACHED();
- if (fstat(*readonly_fd, &readonly_st))
+ if (fstat(readonly_fd.get(), &readonly_st))
NOTREACHED();
if (st.st_dev != readonly_st.st_dev || st.st_ino != readonly_st.st_ino) {
LOG(ERROR) << "writable and read-only inodes don't match; bailing";
@@ -381,7 +377,7 @@
}
}
inode_ = st.st_ino;
- readonly_mapped_file_ = *readonly_fd.release();
+ readonly_mapped_file_ = readonly_fd.release();
return true;
}
diff --git a/base/posix/unix_domain_socket_linux_unittest.cc b/base/posix/unix_domain_socket_linux_unittest.cc
index 22bb172..bb7154f 100644
--- a/base/posix/unix_domain_socket_linux_unittest.cc
+++ b/base/posix/unix_domain_socket_linux_unittest.cc
@@ -9,6 +9,7 @@
#include "base/bind.h"
#include "base/bind_helpers.h"
#include "base/file_util.h"
+#include "base/files/scoped_file.h"
#include "base/pickle.h"
#include "base/posix/unix_domain_socket_linux.h"
#include "base/synchronization/waitable_event.h"
@@ -25,8 +26,8 @@
int fds[2];
ASSERT_EQ(0, socketpair(AF_UNIX, SOCK_SEQPACKET, 0, fds));
- file_util::ScopedFD scoped_fd0(&fds[0]);
- file_util::ScopedFD scoped_fd1(&fds[1]);
+ ScopedFD scoped_fd0(fds[0]);
+ ScopedFD scoped_fd1(fds[1]);
// Have the thread send a synchronous message via the socket.
Pickle request;
@@ -62,7 +63,7 @@
ASSERT_EQ(0, sigaction(SIGPIPE, &act, &oldact));
int fds[2];
ASSERT_EQ(0, socketpair(AF_UNIX, SOCK_SEQPACKET, 0, fds));
- file_util::ScopedFD scoped_fd1(&fds[1]);
+ ScopedFD scoped_fd1(fds[1]);
ASSERT_EQ(0, IGNORE_EINTR(close(fds[0])));
// Have the thread send a synchronous message via the socket. Unless the
diff --git a/base/process/kill_posix.cc b/base/process/kill_posix.cc
index 99d70d9..8187c38 100644
--- a/base/process/kill_posix.cc
+++ b/base/process/kill_posix.cc
@@ -10,6 +10,7 @@
#include <unistd.h>
#include "base/file_util.h"
+#include "base/files/scoped_file.h"
#include "base/logging.h"
#include "base/posix/eintr_wrapper.h"
#include "base/process/process_iterator.h"
@@ -273,16 +274,15 @@
DCHECK_GT(handle, 0);
DCHECK(wait.InMilliseconds() == base::kNoTimeout || wait > base::TimeDelta());
- int kq = kqueue();
- if (kq == -1) {
+ ScopedFD kq(kqueue());
+ if (!kq.is_valid()) {
DPLOG(ERROR) << "kqueue";
return false;
}
- file_util::ScopedFD kq_closer(&kq);
struct kevent change = {0};
EV_SET(&change, handle, EVFILT_PROC, EV_ADD, NOTE_EXIT, 0, NULL);
- int result = HANDLE_EINTR(kevent(kq, &change, 1, NULL, 0, NULL));
+ int result = HANDLE_EINTR(kevent(kq.get(), &change, 1, NULL, 0, NULL));
if (result == -1) {
if (errno == ESRCH) {
// If the process wasn't found, it must be dead.
@@ -316,7 +316,7 @@
remaining_timespec_ptr = &remaining_timespec;
}
- result = kevent(kq, NULL, 0, &event, 1, remaining_timespec_ptr);
+ result = kevent(kq.get(), NULL, 0, &event, 1, remaining_timespec_ptr);
if (result == -1 && errno == EINTR) {
if (!wait_forever) {
diff --git a/base/process/launch_posix.cc b/base/process/launch_posix.cc
index 84a05d6..79e74d5 100644
--- a/base/process/launch_posix.cc
+++ b/base/process/launch_posix.cc
@@ -26,6 +26,7 @@
#include "base/debug/stack_trace.h"
#include "base/file_util.h"
#include "base/files/dir_reader_posix.h"
+#include "base/files/scoped_file.h"
#include "base/logging.h"
#include "base/memory/scoped_ptr.h"
#include "base/posix/eintr_wrapper.h"
@@ -332,14 +333,13 @@
// 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.
- int null_fd = HANDLE_EINTR(open("/dev/null", O_RDONLY));
- if (null_fd < 0) {
+ 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);
}
- file_util::ScopedFD null_fd_closer(&null_fd);
- int new_fd = HANDLE_EINTR(dup2(null_fd, STDIN_FILENO));
+ 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);
diff --git a/base/test/launcher/test_launcher.cc b/base/test/launcher/test_launcher.cc
index c25b00a..889244d 100644
--- a/base/test/launcher/test_launcher.cc
+++ b/base/test/launcher/test_launcher.cc
@@ -14,6 +14,7 @@
#include "base/environment.h"
#include "base/file_util.h"
#include "base/files/file_path.h"
+#include "base/files/scoped_file.h"
#include "base/format_macros.h"
#include "base/lazy_instance.h"
#include "base/logging.h"
@@ -244,16 +245,14 @@
options.new_process_group = true;
base::FileHandleMappingVector fds_mapping;
- file_util::ScopedFD output_file_fd_closer;
+ base::ScopedFD output_file_fd;
if (redirect_stdio) {
- int output_file_fd = open(output_file.value().c_str(), O_RDWR);
- CHECK_GE(output_file_fd, 0);
+ output_file_fd.reset(open(output_file.value().c_str(), O_RDWR));
+ CHECK(output_file_fd.is_valid());
- output_file_fd_closer.reset(&output_file_fd);
-
- fds_mapping.push_back(std::make_pair(output_file_fd, STDOUT_FILENO));
- fds_mapping.push_back(std::make_pair(output_file_fd, STDERR_FILENO));
+ fds_mapping.push_back(std::make_pair(output_file_fd.get(), STDOUT_FILENO));
+ fds_mapping.push_back(std::make_pair(output_file_fd.get(), STDERR_FILENO));
options.fds_to_remap = &fds_mapping;
}
#endif
@@ -264,10 +263,10 @@
if (redirect_stdio) {
#if defined(OS_WIN)
- FlushFileBuffers(handle.Get());
- handle.Close();
+ FlushFileBuffers(handle.Get());
+ handle.Close();
#elif defined(OS_POSIX)
- output_file_fd_closer.reset();
+ output_file_fd.reset();
#endif
}