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). BUG= R=agl@chromium.org, viettrungluu@chromium.org Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=257001 Review URL: https://codereview.chromium.org/191673003 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@257179 0039d316-1c4b-4281-b951-d872f2087c98
FairyWorld · Mar 14, 2014 · c0e895e · c0e895e
1 parent 5fcc99e
commit c0e895e
Show file tree

Hide file tree

Showing 42 changed files with 332 additions and 286 deletions.
diff --git a/base/BUILD.gn b/base/BUILD.gn
@@ -180,6 +180,8 @@ component("base") {
     "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_temp_dir.cc",
     "files/scoped_temp_dir.h",
     "float_util.h",

diff --git a/base/base.gypi b/base/base.gypi
@@ -219,6 +219,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
@@ -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 @@ bool ReadProcMaps(std::string* proc_maps) {
   // 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 @@ bool ReadProcMaps(std::string* proc_maps) {
     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
@@ -426,32 +426,6 @@ struct ScopedFILEClose {
 // 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
@@ -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 @@ int CreateAndOpenFdForTemporaryFile(FilePath directory, FilePath* path) {
 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 @@ bool GetFileInfo(const FilePath& file_path, File::Info* results) {
   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
@@ -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 @@ TEST(ScopedFD, ScopedFDDoesClose) {
   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 @@ TEST(ScopedFD, ScopedFDDoesClose) {
 
 #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
@@ -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
@@ -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
@@ -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 @@ bool CreateAshmemRegion(const char* name,
                         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 @@ bool CreateAshmemRegion(const char* name,
   // 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
@@ -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 @@ class BASE_EXPORT SharedMemory {
 
  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
@@ -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 @@ bool SharedMemory::Create(const SharedMemoryCreateOptions& options) {
 
   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 @@ bool SharedMemory::Create(const SharedMemoryCreateOptions& options) {
 
     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 @@ bool SharedMemory::Create(const SharedMemoryCreateOptions& options) {
     }
 
     // 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 @@ bool SharedMemory::Open(const std::string& name, bool read_only) {
 
   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 @@ void SharedMemory::UnlockDeprecated() {
 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 @@ bool SharedMemory::PrepareMapFile(ScopedFILE fp, ScopedFD readonly_fd) {
   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 @@ bool SharedMemory::PrepareMapFile(ScopedFILE fp, ScopedFD readonly_fd) {
     }
   }
   inode_ = st.st_ino;
-  readonly_mapped_file_ = *readonly_fd.release();
+  readonly_mapped_file_ = readonly_fd.release();
 
   return true;
 }