Revert of Implement ScopedFD in terms of ScopedGeneric. (https://code…

…review.chromium.org/191673003/)

Reason for revert:
Doesn't correctly link

/mnt/data/b/build/slave/Chromium_Linux_Codesearch/build/src/third_party/gold/gold64: warning: hidden symbol 'base::internal::ScopedFDCloseTraits::Free(int)' in obj/base/files/nacl_helper.scoped_file.o is referenced by DSO lib/libipc.so

Original issue's description:
> 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
> 
> Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=257179

TBR=viettrungluu@chromium.org,agl@chromium.org,brettw@chromium.org
NOTREECHECKS=true
NOTRY=true
BUG=

Review URL: https://codereview.chromium.org/201203002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@257323 0039d316-1c4b-4281-b951-d872f2087c98

base/BUILD.gn

@@ -180,8 +180,6 @@ 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",

base/base.gypi

@@ -221,8 +221,6 @@
           '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',

base/debug/proc_maps_linux.cc

@@ -11,7 +11,6 @@
 #endif
 
 #include "base/file_util.h"
-#include "base/files/scoped_file.h"
 #include "base/strings/string_split.h"
 
 #if defined(OS_ANDROID)
@@ -46,11 +45,12 @@ bool ReadProcMaps(std::string* proc_maps) {
   // file for details.
   const long kReadSize = sysconf(_SC_PAGESIZE);
 
-  base::ScopedFD fd(HANDLE_EINTR(open("/proc/self/maps", O_RDONLY)));
-  if (!fd.is_valid()) {
+  int fd = HANDLE_EINTR(open("/proc/self/maps", O_RDONLY));
+  if (fd == -1) {
     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.get(), buffer, kReadSize));
+    ssize_t bytes_read = HANDLE_EINTR(read(fd, buffer, kReadSize));
     if (bytes_read < 0) {
       DPLOG(ERROR) << "Couldn't read /proc/self/maps";
       proc_maps->clear();

base/file_util.h

@@ -426,6 +426,32 @@ 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 --------------------------------------------------------------------

base/file_util_posix.cc

@@ -33,7 +33,6 @@
 #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"
@@ -173,15 +172,15 @@ int CreateAndOpenFdForTemporaryFile(FilePath directory, FilePath* path) {
 bool DetermineDevShmExecutable() {
   bool result = false;
   FilePath path;
-
-  ScopedFD fd(CreateAndOpenFdForTemporaryFile(FilePath("/dev/shm"), &path));
-  if (fd.is_valid()) {
+  int fd = CreateAndOpenFdForTemporaryFile(FilePath("/dev/shm"), &path);
+  if (fd >= 0) {
+    file_util::ScopedFD shm_fd_closer(&fd);
     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.get(), 0);
+    void *mapping = mmap(NULL, pagesize, PROT_READ, MAP_SHARED, fd, 0);
     if (mapping != MAP_FAILED) {
       if (mprotect(mapping, pagesize, PROT_READ | PROT_EXEC) == 0)
         result = true;
@@ -661,10 +660,11 @@ bool GetFileInfo(const FilePath& file_path, File::Info* results) {
   stat_wrapper_t file_info;
 #if defined(OS_ANDROID)
   if (file_path.IsContentUri()) {
-    ScopedFD fd(OpenContentUriForRead(file_path));
-    if (!fd.is_valid())
+    int fd = OpenContentUriForRead(file_path);
+    if (fd < 0)
       return false;
-    if (CallFstat(fd.get(), &file_info) != 0)
+    file_util::ScopedFD scoped_fd(&fd);
+    if (CallFstat(fd, &file_info) != 0)
       return false;
   } else {
 #endif  // defined(OS_ANDROID)

base/file_util_unittest.cc

@@ -26,7 +26,6 @@
 #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"
@@ -2474,9 +2473,9 @@ TEST(ScopedFD, ScopedFDDoesClose) {
   char c = 0;
   ASSERT_EQ(0, pipe(fds));
   const int write_end = fds[1];
-  base::ScopedFD read_end_closer(fds[0]);
+  file_util::ScopedFDCloser read_end_closer(fds);
   {
-    base::ScopedFD write_end_closer(fds[1]);
+    file_util::ScopedFDCloser write_end_closer(fds + 1);
   }
   // This is the only thread. This file descriptor should no longer be valid.
   int ret = close(write_end);
@@ -2490,14 +2489,14 @@ TEST(ScopedFD, ScopedFDDoesClose) {
 
 #if defined(GTEST_HAS_DEATH_TEST)
 void CloseWithScopedFD(int fd) {
-  base::ScopedFD fd_closer(fd);
+  file_util::ScopedFDCloser fd_closer(&fd);
 }
 #endif
 
 TEST(ScopedFD, ScopedFDCrashesOnCloseFailure) {
   int fds[2];
   ASSERT_EQ(0, pipe(fds));
-  base::ScopedFD read_end_closer(fds[0]);
+  file_util::ScopedFDCloser read_end_closer(fds);
   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.

base/memory/discardable_memory_allocator_android.cc

@@ -16,7 +16,6 @@
 #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"
@@ -66,13 +65,14 @@ bool CreateAshmemRegion(const char* name,
                         size_t size,
                         int* out_fd,
                         void** out_address) {
-  base::ScopedFD fd(ashmem_create_region(name, size));
-  if (!fd.is_valid()) {
+  int fd = ashmem_create_region(name, size);
+  if (fd < 0) {
     DLOG(ERROR) << "ashmem_create_region() failed";
     return false;
   }
+  file_util::ScopedFD fd_closer(&fd);
 
-  const int err = ashmem_set_prot_region(fd.get(), PROT_READ | PROT_WRITE);
+  const int err = ashmem_set_prot_region(fd, PROT_READ | PROT_WRITE);
   if (err < 0) {
     DLOG(ERROR) << "Error " << err << " when setting protection of ashmem";
     return false;
@@ -82,13 +82,14 @@ 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.get(), 0);
+      NULL, size, PROT_READ | PROT_WRITE, MAP_SHARED, fd, 0);
   if (address == MAP_FAILED) {
     DPLOG(ERROR) << "Failed to map memory.";
     return false;
   }
 
-  *out_fd = fd.release();
+  ignore_result(fd_closer.release());
+  *out_fd = fd;
   *out_address = address;
   return true;
 }

base/memory/shared_memory.h

@@ -22,7 +22,6 @@
 #if defined(OS_POSIX)
 #include "base/file_descriptor_posix.h"
 #include "base/file_util.h"
-#include "base/files/scoped_file.h"
 #endif
 
 namespace base {
@@ -260,7 +259,7 @@ class BASE_EXPORT SharedMemory {
 
  private:
 #if defined(OS_POSIX) && !defined(OS_NACL)
-  bool PrepareMapFile(file_util::ScopedFILE fp, base::ScopedFD readonly);
+  bool PrepareMapFile(file_util::ScopedFILE fp, file_util::ScopedFD readonly);
   bool FilePathForMemoryName(const std::string& mem_name, FilePath* path);
   void LockOrUnlockCommon(int function);
 #endif

base/memory/shared_memory_posix.cc

@@ -30,6 +30,7 @@
 #include "third_party/ashmem/ashmem.h"
 #endif
 
+using file_util::ScopedFD;
 using file_util::ScopedFILE;
 
 namespace base {
@@ -131,7 +132,8 @@ bool SharedMemory::Create(const SharedMemoryCreateOptions& options) {
 
   ScopedFILE fp;
   bool fix_size = true;
-  ScopedFD readonly_fd;
+  int readonly_fd_storage = -1;
+  ScopedFD readonly_fd(&readonly_fd_storage);
 
   FilePath path;
   if (options.name_deprecated == NULL || options.name_deprecated->empty()) {
@@ -143,8 +145,8 @@ bool SharedMemory::Create(const SharedMemoryCreateOptions& options) {
 
     if (fp) {
       // Also open as readonly so that we can ShareReadOnlyToProcess.
-      readonly_fd.reset(HANDLE_EINTR(open(path.value().c_str(), O_RDONLY)));
-      if (!readonly_fd.is_valid()) {
+      *readonly_fd = HANDLE_EINTR(open(path.value().c_str(), O_RDONLY));
+      if (*readonly_fd < 0) {
         DPLOG(ERROR) << "open(\"" << path.value() << "\", O_RDONLY) failed";
         fp.reset();
       }
@@ -196,8 +198,8 @@ bool SharedMemory::Create(const SharedMemoryCreateOptions& options) {
     }
 
     // Also open as readonly so that we can ShareReadOnlyToProcess.
-    readonly_fd.reset(HANDLE_EINTR(open(path.value().c_str(), O_RDONLY)));
-    if (!readonly_fd.is_valid()) {
+    *readonly_fd = HANDLE_EINTR(open(path.value().c_str(), O_RDONLY));
+    if (*readonly_fd < 0) {
       DPLOG(ERROR) << "open(\"" << path.value() << "\", O_RDONLY) failed";
       close(fd);
       fd = -1;
@@ -263,8 +265,10 @@ bool SharedMemory::Open(const std::string& name, bool read_only) {
 
   const char *mode = read_only ? "r" : "r+";
   ScopedFILE fp(base::OpenFile(path, mode));
-  ScopedFD readonly_fd(HANDLE_EINTR(open(path.value().c_str(), O_RDONLY)));
-  if (!readonly_fd.is_valid()) {
+  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) {
     DPLOG(ERROR) << "open(\"" << path.value() << "\", O_RDONLY) failed";
   }
   return PrepareMapFile(fp.Pass(), readonly_fd.Pass());
@@ -349,7 +353,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.is_valid()) return false;
+  if (fp == NULL || *readonly_fd < 0) return false;
 
   // This function theoretically can block on the disk, but realistically
   // the temporary files we create will just go into the buffer cache
@@ -360,7 +364,7 @@ bool SharedMemory::PrepareMapFile(ScopedFILE fp, ScopedFD readonly_fd) {
   struct stat readonly_st = {};
   if (fstat(fileno(fp.get()), &st))
     NOTREACHED();
-  if (fstat(readonly_fd.get(), &readonly_st))
+  if (fstat(*readonly_fd, &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";
@@ -377,7 +381,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;
 }