libbrillo: dup and scope file descriptor

brillo::dbus_utils::FileDescriptor may be used to pass FDs
back out of adaptor code into D-Bus bindings, in which case the
adaptor code does not pass ownership of the FDs to the D-Bus
bindings but the D-Bus FD will probably outlive the lifetime of
the FD in the adaptor code.

To deal with this, we use brillo::dbus_utils::FileDescriptor as a
wrapper around a ScopedFD which dups file descriptors that are
put in. This means adaptor code can pass FDs without having to
give up ownership or manually try to mitigate leaks later, and
does not have to deal with e.g. duping into a ScopedFD at all
callsites.

BUG=b:37434548
TEST=unit tests, use in permission_broker/session_manager/etc

Change-Id: Ic3f139d4af817bb0bbe975cec3855e44a09bcda9
Reviewed-on: https://chromium-review.googlesource.com/987197
Commit-Ready: Eric Caruso <ejcaruso@chromium.org>
Tested-by: Eric Caruso <ejcaruso@chromium.org>
Reviewed-by: Dan Erat <derat@chromium.org>

3 files changed, 27 insertions, 5 deletions

diff --git a/brillo/dbus/data_serialization.cc b/brillo/dbus/data_serialization.cc
index 0803b61..35ab00a 100644
--- a/brillo/dbus/data_serialization.cc
+++ b/brillo/dbus/data_serialization.cc
@@ -68,7 +68,7 @@ void AppendValueToWriter(dbus::MessageWriter* writer,
 
 void AppendValueToWriter(dbus::MessageWriter* writer,
                          const FileDescriptor& value) {
-  writer->AppendFileDescriptor(value.fd);
+  writer->AppendFileDescriptor(value.get());
 }
 
 void AppendValueToWriter(dbus::MessageWriter* writer,
diff --git a/brillo/dbus/dbus_method_invoker.h b/brillo/dbus/dbus_method_invoker.h
index 09d48f0..ff6b79e 100644
--- a/brillo/dbus/dbus_method_invoker.h
+++ b/brillo/dbus/dbus_method_invoker.h
@@ -165,6 +165,9 @@ inline dbus::FileDescriptor HackMove(const dbus::FileDescriptor& val) {
 inline base::ScopedFD HackMove(const base::ScopedFD& val) {
   return std::move(const_cast<base::ScopedFD&>(val));
 }
+inline FileDescriptor HackMove(const FileDescriptor& val) {
+  return std::move(const_cast<FileDescriptor&>(val));
+}
 }  // namespace internal
 
 // Extracts the parameters of |ResultTypes...| types from the message reader
diff --git a/brillo/dbus/file_descriptor.h b/brillo/dbus/file_descriptor.h
index bcee136..2c4a01b 100644
--- a/brillo/dbus/file_descriptor.h
+++ b/brillo/dbus/file_descriptor.h
@@ -5,6 +5,9 @@
 #ifndef LIBBRILLO_BRILLO_DBUS_FILE_DESCRIPTOR_H_
 #define LIBBRILLO_BRILLO_DBUS_FILE_DESCRIPTOR_H_
 
+#include <base/files/scoped_file.h>
+#include <base/macros.h>
+
 namespace brillo {
 namespace dbus_utils {
 
@@ -12,16 +15,32 @@ namespace dbus_utils {
 // Implicit conversions are provided because this should be as transparent
 // a wrapper as possible to match the libchrome bindings below when this
 // class is used by chromeos-dbus-bindings.
+//
+// Because we might pass these around and the calling code neither passes
+// ownership nor knows when this will be destroyed, it actually dups the FD
+// so that the calling code and binding code both have a clear handle on the
+// lifetimes of their respective copies of the FD.
 struct FileDescriptor {
-  FileDescriptor() : fd(-1) {}
-  FileDescriptor(int fd) : fd(fd) {}
+  FileDescriptor() = default;
+  FileDescriptor(int fd) : fd(dup(fd)) {}
+  FileDescriptor(FileDescriptor&& other) : fd(std::move(other.fd)) {}
 
   inline FileDescriptor& operator=(int new_fd) {
-    fd = new_fd;
+    fd.reset(dup(new_fd));
+    return *this;
+  }
+
+  FileDescriptor& operator=(FileDescriptor&& other) {
+    fd = std::move(other.fd);
     return *this;
   }
 
-  int fd;
+  int get() const { return fd.get(); }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(FileDescriptor);
+
+  base::ScopedFD fd;
 };
 
 }  // namespace dbus_utils