dbus: Remove dbus::FileDescriptor

BUG=621841
TEST=dbus_unittests
TBR=steel@chromium.org for device/bluetooth/dbus

Review-Url: https://codereview.chromium.org/2337893002
Cr-Commit-Position: refs/heads/master@{#418481}

chromeos/dbus/fake_permission_broker_client.cc

@@ -15,7 +15,6 @@
 #include "base/strings/stringprintf.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "base/threading/worker_pool.h"
-#include "dbus/file_descriptor.h"
 
 namespace chromeos {
 

chromeos/dbus/mock_permission_broker_client.cc

@@ -4,8 +4,6 @@
 
 #include "chromeos/dbus/mock_permission_broker_client.h"
 
-#include "dbus/file_descriptor.h"
-
 namespace chromeos {
 
 MockPermissionBrokerClient::MockPermissionBrokerClient() {

chromeos/network/firewall_hole_unittest.cc

@@ -9,7 +9,6 @@
 #include "base/run_loop.h"
 #include "chromeos/dbus/dbus_thread_manager.h"
 #include "chromeos/dbus/mock_permission_broker_client.h"
-#include "dbus/file_descriptor.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 

dbus/BUILD.gn

@@ -17,8 +17,6 @@ component("dbus") {
     "dbus_statistics.h",
     "exported_object.cc",
     "exported_object.h",
-    "file_descriptor.cc",
-    "file_descriptor.h",
     "message.cc",
     "message.h",
     "object_manager.cc",

dbus/file_descriptor.cc

-// Copyright (c) 2012 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 <algorithm>
-
-#include "base/bind.h"
-#include "base/files/file.h"
-#include "base/location.h"
-#include "base/logging.h"
-#include "base/threading/worker_pool.h"
-#include "dbus/file_descriptor.h"
-
-using std::swap;
-
-namespace dbus {
-
-void CHROME_DBUS_EXPORT FileDescriptor::Deleter::operator()(
-    FileDescriptor* fd) {
-  base::WorkerPool::PostTask(
-      FROM_HERE, base::Bind(&base::DeletePointer<FileDescriptor>, fd), false);
-}
-
-FileDescriptor::FileDescriptor(FileDescriptor&& other) : FileDescriptor() {
-  Swap(&other);
-}
-
-FileDescriptor::~FileDescriptor() {
-  if (owner_)
-    base::File auto_closer(value_);
-}
-
-FileDescriptor& FileDescriptor::operator=(FileDescriptor&& other) {
-  Swap(&other);
-  return *this;
-}
-
-int FileDescriptor::value() const {
-  CHECK(valid_);
-  return value_;
-}
-
-int FileDescriptor::TakeValue() {
-  CHECK(valid_);  // NB: check first so owner_ is unchanged if this triggers
-  owner_ = false;
-  return value_;
-}
-
-void FileDescriptor::CheckValidity() {
-  base::File file(value_);
-  if (!file.IsValid()) {
-    valid_ = false;
-    return;
-  }
-
-  base::File::Info info;
-  bool ok = file.GetInfo(&info);
-  file.TakePlatformFile();  // Prevent |value_| from being closed by |file|.
-  valid_ = (ok && !info.is_directory);
-}
-
-void FileDescriptor::Swap(FileDescriptor* other) {
-  swap(value_, other->value_);
-  swap(owner_, other->owner_);
-  swap(valid_, other->valid_);
-}
-
-}  // namespace dbus

dbus/file_descriptor.h

-// Copyright (c) 2012 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 DBUS_FILE_DESCRIPTOR_H_
-#define DBUS_FILE_DESCRIPTOR_H_
-
-#include <memory>
-
-#include "base/macros.h"
-#include "dbus/dbus_export.h"
-
-namespace dbus {
-
-// FileDescriptor is a type used to encapsulate D-Bus file descriptors
-// and to follow the RAII idiom appropiate for use with message operations
-// where the descriptor might be easily leaked.  To guard against this the
-// descriptor is closed when an instance is destroyed if it is owned.
-// Ownership is asserted only when PutValue is used and TakeValue can be
-// used to take ownership.
-//
-// For example, in the following
-//  FileDescriptor fd;
-//  if (!reader->PopString(&name) ||
-//      !reader->PopFileDescriptor(&fd) ||
-//      !reader->PopUint32(&flags)) {
-// the descriptor in fd will be closed if the PopUint32 fails.  But
-//   writer.AppendFileDescriptor(dbus::FileDescriptor(1));
-// will not automatically close "1" because it is not owned.
-//
-// Descriptors must be validated before marshalling in a D-Bus message
-// or using them after unmarshalling.  We disallow descriptors to a
-// directory to reduce the security risks.  Splitting out validation
-// also allows the caller to do this work on the File thread to conform
-// with i/o restrictions.
-class CHROME_DBUS_EXPORT FileDescriptor {
- public:
-  // This provides a simple way to pass around file descriptors since they must
-  // be closed on a thread that is allowed to perform I/O.
-  struct Deleter {
-    void CHROME_DBUS_EXPORT operator()(FileDescriptor* fd);
-  };
-
-  // Permits initialization without a value for passing to
-  // dbus::MessageReader::PopFileDescriptor to fill in and from int values.
-  FileDescriptor() : value_(-1), owner_(false), valid_(false) {}
-  explicit FileDescriptor(int value) : value_(value), owner_(false),
-      valid_(false) {}
-
-  FileDescriptor(FileDescriptor&& other);
-
-  virtual ~FileDescriptor();
-
-  FileDescriptor& operator=(FileDescriptor&& other);
-
-  // Retrieves value as an int without affecting ownership.
-  int value() const;
-
-  // Retrieves whether or not the descriptor is ok to send/receive.
-  int is_valid() const { return valid_; }
-
-  // Sets the value and assign ownership.
-  void PutValue(int value) {
-    value_ = value;
-    owner_ = true;
-    valid_ = false;
-  }
-
-  // Takes the value and ownership.
-  int TakeValue();
-
-  // Checks (and records) validity of the file descriptor.
-  // We disallow directories to avoid potential sandbox escapes.
-  // Note this call must be made on a thread where file i/o is allowed.
-  void CheckValidity();
-
- private:
-  void Swap(FileDescriptor* other);
-
-  int value_;
-  bool owner_;
-  bool valid_;
-
-  DISALLOW_COPY_AND_ASSIGN(FileDescriptor);
-};
-
-using ScopedFileDescriptor =
-    std::unique_ptr<FileDescriptor, FileDescriptor::Deleter>;
-
-}  // namespace dbus
-
-#endif  // DBUS_FILE_DESCRIPTOR_H_

dbus/message.cc

@@ -222,11 +222,11 @@ std::string Message::ToStringInternal(const std::string& indent,
       case UNIX_FD: {
         CHECK(IsDBusTypeUnixFdSupported());
 
-        FileDescriptor file_descriptor;
+        base::ScopedFD file_descriptor;
         if (!reader->PopFileDescriptor(&file_descriptor))
           return kBrokenMessage;
         output += indent + "fd#" +
-                  base::IntToString(file_descriptor.value()) + "\n";
+                  base::IntToString(file_descriptor.get()) + "\n";
         break;
       }
       default:
@@ -719,17 +719,6 @@ void MessageWriter::AppendFileDescriptor(int value) {
   AppendBasic(DBUS_TYPE_UNIX_FD, &value);  // This duplicates the FD.
 }
 
-void MessageWriter::AppendFileDescriptor(const FileDescriptor& value) {
-  CHECK(IsDBusTypeUnixFdSupported());
-
-  if (!value.is_valid()) {
-    // NB: sending a directory potentially enables sandbox escape
-    LOG(FATAL) << "Attempt to pass invalid file descriptor";
-  }
-  int fd = value.value();
-  AppendBasic(DBUS_TYPE_UNIX_FD, &fd);
-}
-
 //
 // MessageReader implementation.
 //
@@ -1033,17 +1022,4 @@ bool MessageReader::PopFileDescriptor(base::ScopedFD* value) {
   return true;
 }
 
-bool MessageReader::PopFileDescriptor(FileDescriptor* value) {
-  CHECK(IsDBusTypeUnixFdSupported());
-
-  int fd = -1;
-  const bool success = PopBasic(DBUS_TYPE_UNIX_FD, &fd);
-  if (!success)
-    return false;
-
-  value->PutValue(fd);
-  // NB: the caller must check validity before using the value
-  return true;
-}
-
 }  // namespace dbus

dbus/message.h

@@ -16,7 +16,6 @@
 #include "base/files/scoped_file.h"
 #include "base/macros.h"
 #include "dbus/dbus_export.h"
-#include "dbus/file_descriptor.h"
 #include "dbus/object_path.h"
 
 namespace google {
@@ -291,10 +290,6 @@ class CHROME_DBUS_EXPORT MessageWriter {
   // The FD will be duplicated so you still have to close the original FD.
   void AppendFileDescriptor(int value);
 
-  // DEPRECATED: Use the method with the same name above instead.
-  // TODO(hashimoto): Remove this. crbug.com/621841
-  void AppendFileDescriptor(const FileDescriptor& value);
-
   // Opens an array. The array contents can be added to the array with
   // |sub_writer|. The client code must close the array with
   // CloseContainer(), once all contents are added.
@@ -408,10 +403,6 @@ class CHROME_DBUS_EXPORT MessageReader {
   bool PopObjectPath(ObjectPath* value);
   bool PopFileDescriptor(base::ScopedFD* value);
 
-  // DEPRECATED: Use the method with the same name above.
-  // TODO(hashimoto): Remove this. crbug.com/621841
-  bool PopFileDescriptor(FileDescriptor* value);
-
   // Sets up the given message reader to read an array at the current
   // iterator position.
   // Returns true and advances the iterator on success.

dbus/message_unittest.cc

@@ -124,35 +124,26 @@ TEST(MessageTest, AppendAndPopFileDescriptor) {
   MessageWriter writer(message.get());
 
   // Append stdout.
-  FileDescriptor temp(1);
-  // Descriptor should not be valid until checked.
-  ASSERT_FALSE(temp.is_valid());
-  // NB: thread IO requirements not relevant for unit tests.
-  temp.CheckValidity();
-  ASSERT_TRUE(temp.is_valid());
-  writer.AppendFileDescriptor(temp);
+  const int fd_in = 1;
+  writer.AppendFileDescriptor(fd_in);
 
-  FileDescriptor fd_value;
+  base::ScopedFD fd_out;
 
   MessageReader reader(message.get());
   ASSERT_TRUE(reader.HasMoreData());
   ASSERT_EQ(Message::UNIX_FD, reader.GetDataType());
   ASSERT_EQ("h", reader.GetDataSignature());
-  ASSERT_TRUE(reader.PopFileDescriptor(&fd_value));
+  ASSERT_TRUE(reader.PopFileDescriptor(&fd_out));
   ASSERT_FALSE(reader.HasMoreData());
-  // Descriptor is not valid until explicitly checked.
-  ASSERT_FALSE(fd_value.is_valid());
-  fd_value.CheckValidity();
-  ASSERT_TRUE(fd_value.is_valid());
 
   // Stdout should be returned but we cannot check the descriptor
   // value because stdout will be dup'd.  Instead check st_rdev
   // which should be identical.
   struct stat sb_stdout;
-  int status_stdout = HANDLE_EINTR(fstat(1, &sb_stdout));
+  int status_stdout = HANDLE_EINTR(fstat(fd_in, &sb_stdout));
   ASSERT_GE(status_stdout, 0);
   struct stat sb_fd;
-  int status_fd = HANDLE_EINTR(fstat(fd_value.value(), &sb_fd));
+  int status_fd = HANDLE_EINTR(fstat(fd_out.get(), &sb_fd));
   ASSERT_GE(status_fd, 0);
   EXPECT_EQ(sb_stdout.st_rdev, sb_fd.st_rdev);
 }

device/bluetooth/dbus/bluetooth_le_advertisement_service_provider.h

@@ -15,7 +15,6 @@
 #include "base/callback.h"
 #include "base/macros.h"
 #include "dbus/bus.h"
-#include "dbus/file_descriptor.h"
 #include "dbus/object_path.h"
 #include "device/bluetooth/bluetooth_export.h"
 

device/bluetooth/dbus/fake_bluetooth_le_advertisement_service_provider.h

@@ -10,7 +10,6 @@
 #include "base/bind.h"
 #include "base/callback.h"
 #include "base/macros.h"
-#include "dbus/file_descriptor.h"
 #include "dbus/object_path.h"
 #include "device/bluetooth/bluetooth_export.h"
 #include "device/bluetooth/dbus/bluetooth_le_advertisement_service_provider.h"

device/bluetooth/dbus/fake_bluetooth_media_transport_client.cc

@@ -14,7 +14,6 @@
 #include "base/bind.h"
 #include "base/macros.h"
 #include "base/stl_util.h"
-#include "dbus/file_descriptor.h"
 #include "device/bluetooth/dbus/bluetooth_media_client.h"
 #include "device/bluetooth/dbus/bluez_dbus_manager.h"
 #include "device/bluetooth/dbus/fake_bluetooth_adapter_client.h"