libbrillo: add new FD D-Bus bindings

Add serialization and deserialization for the FD types used by
the new libchrome D-Bus bindings, and plumb those through the
method invoker as well.

CQ-DEPEND=CL:956622
BUG=b:37434548
TEST=unit tests

Change-Id: Ice58ecd307da253cd073be83c7805e68fb9c6edf
Reviewed-on: https://chromium-review.googlesource.com/956623
Commit-Ready: Eric Caruso <ejcaruso@chromium.org>
Tested-by: Eric Caruso <ejcaruso@chromium.org>
Reviewed-by: Eric Caruso <ejcaruso@chromium.org>

6 files changed, 161 insertions, 10 deletions

diff --git a/brillo/dbus/data_serialization.cc b/brillo/dbus/data_serialization.cc
index 86d1c63..0803b61 100644
--- a/brillo/dbus/data_serialization.cc
+++ b/brillo/dbus/data_serialization.cc
@@ -67,6 +67,11 @@ void AppendValueToWriter(dbus::MessageWriter* writer,
 }
 
 void AppendValueToWriter(dbus::MessageWriter* writer,
+                         const FileDescriptor& value) {
+  writer->AppendFileDescriptor(value.fd);
+}
+
+void AppendValueToWriter(dbus::MessageWriter* writer,
                          const brillo::Any& value) {
   value.AppendToDBusMessageWriter(writer);
 }
@@ -149,6 +154,13 @@ bool PopValueFromReader(dbus::MessageReader* reader,
   return ok;
 }
 
+bool PopValueFromReader(dbus::MessageReader* reader,
+                        base::ScopedFD* value) {
+  dbus::MessageReader variant_reader(nullptr);
+  return details::DescendIntoVariantIfPresent(&reader, &variant_reader) &&
+         reader->PopFileDescriptor(value);
+}
+
 namespace {
 
 // Helper methods for PopValueFromReader(dbus::MessageReader*, Any*)
diff --git a/brillo/dbus/data_serialization.h b/brillo/dbus/data_serialization.h
index 46da165..e9265af 100644
--- a/brillo/dbus/data_serialization.h
+++ b/brillo/dbus/data_serialization.h
@@ -36,7 +36,9 @@
 //               |     (UVW...)    |  std::tuple<U,V,W,...>
 //   DICT        |       a{KV}     |  std::map<K,V>
 //   VARIANT     |        v        |  brillo::Any
-//   UNIX_FD     |        h        |  dbus::FileDescriptor
+//   UNIX_FD     |        h        |  dbus::FileDescriptor (deprecated)
+//               |                 |  brillo::dbus_utils::FileDescriptor (write)
+//               |                 |  base::ScopedFD (read)
 //   SIGNATURE   |        g        |  (unsupported)
 //
 // Additional overloads/specialization can be provided for custom types.
@@ -57,8 +59,10 @@
 #include <utility>
 #include <vector>
 
+#include <base/files/scoped_file.h>
 #include <base/logging.h>
 #include <brillo/brillo_export.h>
+#include <brillo/dbus/file_descriptor.h>
 #include <brillo/type_name_undecorate.h>
 #include <dbus/message.h>
 
@@ -432,6 +436,34 @@ struct DBusType<dbus::FileDescriptor> {
   }
 };
 
+// brillo::dbus_utils::FileDescriptor/base::ScopedFD --------------------------
+BRILLO_EXPORT void AppendValueToWriter(dbus::MessageWriter* writer,
+                                         const FileDescriptor& value);
+BRILLO_EXPORT bool PopValueFromReader(dbus::MessageReader* reader,
+                                        base::ScopedFD* value);
+
+template<>
+struct DBusType<FileDescriptor> {
+  inline static std::string GetSignature() {
+    return DBUS_TYPE_UNIX_FD_AS_STRING;
+  }
+  inline static void Write(dbus::MessageWriter* writer,
+                           const FileDescriptor& value) {
+    AppendValueToWriter(writer, value);
+  }
+};
+
+template<>
+struct DBusType<base::ScopedFD> {
+  inline static std::string GetSignature() {
+    return DBUS_TYPE_UNIX_FD_AS_STRING;
+  }
+  inline static bool Read(dbus::MessageReader* reader,
+                          base::ScopedFD* value) {
+    return PopValueFromReader(reader, value);
+  }
+};
+
 // brillo::Any --------------------------------------------------------------
 BRILLO_EXPORT void AppendValueToWriter(dbus::MessageWriter* writer,
                                          const brillo::Any& value);
diff --git a/brillo/dbus/data_serialization_unittest.cc b/brillo/dbus/data_serialization_unittest.cc
index 585f20b..915195b 100644
--- a/brillo/dbus/data_serialization_unittest.cc
+++ b/brillo/dbus/data_serialization_unittest.cc
@@ -11,7 +11,6 @@
 
 #include "brillo/dbus/test.pb.h"
 
-using dbus::FileDescriptor;
 using dbus::Message;
 using dbus::MessageReader;
 using dbus::MessageWriter;
@@ -33,7 +32,9 @@ TEST(DBusUtils, Supported_BasicTypes) {
   EXPECT_TRUE(IsTypeSupported<double>::value);
   EXPECT_TRUE(IsTypeSupported<std::string>::value);
   EXPECT_TRUE(IsTypeSupported<ObjectPath>::value);
+  EXPECT_TRUE(IsTypeSupported<dbus::FileDescriptor>::value);
   EXPECT_TRUE(IsTypeSupported<FileDescriptor>::value);
+  EXPECT_TRUE(IsTypeSupported<base::ScopedFD>::value);
   EXPECT_TRUE(IsTypeSupported<Any>::value);
   EXPECT_TRUE(IsTypeSupported<google::protobuf::MessageLite>::value);
   EXPECT_TRUE(IsTypeSupported<dbus_utils_test::TestMessage>::value);
@@ -89,7 +90,9 @@ TEST(DBusUtils, Signatures_BasicTypes) {
   EXPECT_EQ("d", GetDBusSignature<double>());
   EXPECT_EQ("s", GetDBusSignature<std::string>());
   EXPECT_EQ("o", GetDBusSignature<ObjectPath>());
+  EXPECT_EQ("h", GetDBusSignature<dbus::FileDescriptor>());
   EXPECT_EQ("h", GetDBusSignature<FileDescriptor>());
+  EXPECT_EQ("h", GetDBusSignature<base::ScopedFD>());
   EXPECT_EQ("v", GetDBusSignature<Any>());
 }
 
@@ -105,7 +108,9 @@ TEST(DBusUtils, Signatures_Arrays) {
   EXPECT_EQ("ad", GetDBusSignature<std::vector<double>>());
   EXPECT_EQ("as", GetDBusSignature<std::vector<std::string>>());
   EXPECT_EQ("ao", GetDBusSignature<std::vector<ObjectPath>>());
+  EXPECT_EQ("ah", GetDBusSignature<std::vector<dbus::FileDescriptor>>());
   EXPECT_EQ("ah", GetDBusSignature<std::vector<FileDescriptor>>());
+  EXPECT_EQ("ah", GetDBusSignature<std::vector<base::ScopedFD>>());
   EXPECT_EQ("av", GetDBusSignature<std::vector<Any>>());
   EXPECT_EQ("a(is)",
             (GetDBusSignature<std::vector<std::pair<int, std::string>>>()));
@@ -228,7 +233,7 @@ TEST(DBusUtils, AppendAndPopBasicDataTypes) {
 }
 
 // Check all basic types can be properly written and read.
-TEST(DBusUtils, AppendAndPopFileDescriptor) {
+TEST(DBusUtils, AppendAndPopFileDescriptor_Deprecated) {
   if (!dbus::IsDBusTypeUnixFdSupported()) {
     LOG(WARNING) << "FD passing is not supported";
     return;
@@ -238,7 +243,7 @@ TEST(DBusUtils, AppendAndPopFileDescriptor) {
   MessageWriter writer(message.get());
 
   // Append stdout.
-  FileDescriptor temp(1);
+  dbus::FileDescriptor temp(1);
   // Descriptor should not be valid until checked.
   EXPECT_FALSE(temp.is_valid());
   // NB: thread IO requirements not relevant for unit tests.
@@ -248,7 +253,7 @@ TEST(DBusUtils, AppendAndPopFileDescriptor) {
 
   EXPECT_EQ("h", message->GetSignature());
 
-  FileDescriptor fd_value;
+  dbus::FileDescriptor fd_value;
 
   MessageReader reader(message.get());
   EXPECT_TRUE(reader.HasMoreData());
@@ -259,6 +264,31 @@ TEST(DBusUtils, AppendAndPopFileDescriptor) {
   EXPECT_TRUE(fd_value.is_valid());
 }
 
+// Check all basic types can be properly written and read.
+TEST(DBusUtils, AppendAndPopFileDescriptor) {
+  if (!dbus::IsDBusTypeUnixFdSupported()) {
+    LOG(WARNING) << "FD passing is not supported";
+    return;
+  }
+
+  std::unique_ptr<Response> message = Response::CreateEmpty();
+  MessageWriter writer(message.get());
+
+  // Append stdout.
+  FileDescriptor temp = 1;
+  AppendValueToWriter(&writer, temp);
+
+  EXPECT_EQ("h", message->GetSignature());
+
+  base::ScopedFD fd_value;
+
+  MessageReader reader(message.get());
+  EXPECT_TRUE(reader.HasMoreData());
+  EXPECT_TRUE(PopValueFromReader(&reader, &fd_value));
+  EXPECT_FALSE(reader.HasMoreData());
+  EXPECT_TRUE(fd_value.is_valid());
+}
+
 // Check all variant types can be properly written and read.
 TEST(DBusUtils, AppendAndPopVariantDataTypes) {
   std::unique_ptr<Response> message = Response::CreateEmpty();
diff --git a/brillo/dbus/dbus_method_invoker.h b/brillo/dbus/dbus_method_invoker.h
index f3d0e6b..09d48f0 100644
--- a/brillo/dbus/dbus_method_invoker.h
+++ b/brillo/dbus/dbus_method_invoker.h
@@ -67,8 +67,10 @@
 #include <tuple>
 
 #include <base/bind.h>
+#include <base/files/scoped_file.h>
 #include <brillo/dbus/dbus_param_reader.h>
 #include <brillo/dbus/dbus_param_writer.h>
+#include <brillo/dbus/file_descriptor.h>
 #include <brillo/dbus/utils.h>
 #include <brillo/errors/error.h>
 #include <brillo/errors/error_codes.h>
@@ -160,6 +162,9 @@ inline const T& HackMove(const T& val) {
 inline dbus::FileDescriptor HackMove(const dbus::FileDescriptor& val) {
   return std::move(const_cast<dbus::FileDescriptor&>(val));
 }
+inline base::ScopedFD HackMove(const base::ScopedFD& val) {
+  return std::move(const_cast<base::ScopedFD&>(val));
+}
 }  // namespace internal
 
 // Extracts the parameters of |ResultTypes...| types from the message reader
diff --git a/brillo/dbus/dbus_method_invoker_unittest.cc b/brillo/dbus/dbus_method_invoker_unittest.cc
index cbd7065..ef20cc7 100644
--- a/brillo/dbus/dbus_method_invoker_unittest.cc
+++ b/brillo/dbus/dbus_method_invoker_unittest.cc
@@ -35,6 +35,7 @@ const char kTestMethod1[] = "TestMethod1";
 const char kTestMethod2[] = "TestMethod2";
 const char kTestMethod3[] = "TestMethod3";
 const char kTestMethod4[] = "TestMethod4";
+const char kTestMethod5[] = "TestMethod5";
 
 class DBusMethodInvokerTest : public testing::Test {
  public:
@@ -88,6 +89,7 @@ class DBusMethodInvokerTest : public testing::Test {
           return response.release();
         }
       } else if (method_call->GetMember() == kTestMethod4) {
+        // DEPRECATED. will be removed after libchrome uprev b/37434548
         method_call->SetSerial(123);
         MessageReader reader(method_call);
         dbus::FileDescriptor fd;
@@ -98,6 +100,16 @@ class DBusMethodInvokerTest : public testing::Test {
           writer.AppendFileDescriptor(fd);
           return response.release();
         }
+      } else if (method_call->GetMember() == kTestMethod5) {
+        method_call->SetSerial(123);
+        MessageReader reader(method_call);
+        base::ScopedFD fd;
+        if (reader.PopFileDescriptor(&fd)) {
+          auto response = Response::CreateEmpty();
+          MessageWriter writer(response.get());
+          writer.AppendFileDescriptor(fd.get());
+          return response.release();
+        }
       }
     }
 
@@ -131,7 +143,7 @@ class DBusMethodInvokerTest : public testing::Test {
   }
 
   // Sends a file descriptor received over D-Bus back to the caller.
-  dbus::FileDescriptor EchoFD(const dbus::FileDescriptor& fd_in) {
+  dbus::FileDescriptor DeprecatedEchoFD(const dbus::FileDescriptor& fd_in) {
     std::unique_ptr<dbus::Response> response =
         brillo::dbus_utils::CallMethodAndBlock(mock_object_proxy_.get(),
                                                kTestInterface, kTestMethod4,
@@ -143,6 +155,19 @@ class DBusMethodInvokerTest : public testing::Test {
     return fd_out;
   }
 
+  // Sends a file descriptor received over D-Bus back to the caller using the
+  // new types.
+  base::ScopedFD EchoFD(int fd_in) {
+    std::unique_ptr<dbus::Response> response =
+        brillo::dbus_utils::CallMethodAndBlock(
+            mock_object_proxy_.get(), kTestInterface, kTestMethod5,
+            nullptr, brillo::dbus_utils::FileDescriptor{fd_in});
+    EXPECT_NE(nullptr, response.get());
+    base::ScopedFD fd_out;
+    using brillo::dbus_utils::ExtractMethodCallResults;
+    EXPECT_TRUE(ExtractMethodCallResults(response.get(), nullptr, &fd_out));
+    return fd_out;
+  }
   scoped_refptr<dbus::MockBus> bus_;
   scoped_refptr<dbus::MockObjectProxy> mock_object_proxy_;
 };
@@ -175,19 +200,37 @@ TEST_F(DBusMethodInvokerTest, TestProtobuf) {
   EXPECT_EQ("bar", resp.bar());
 }
 
-TEST_F(DBusMethodInvokerTest, TestFileDescriptors) {
+TEST_F(DBusMethodInvokerTest, TestFileDescriptors_Deprecated) {
   // Passing a file descriptor over D-Bus would effectively duplicate the fd.
   // So the resulting file descriptor value would be different but it still
   // should be valid.
   dbus::FileDescriptor fd_stdin(0);
   fd_stdin.CheckValidity();
-  EXPECT_NE(fd_stdin.value(), EchoFD(fd_stdin).value());
+  EXPECT_NE(fd_stdin.value(), DeprecatedEchoFD(fd_stdin).value());
   dbus::FileDescriptor fd_stdout(1);
   fd_stdout.CheckValidity();
-  EXPECT_NE(fd_stdout.value(), EchoFD(fd_stdout).value());
+  EXPECT_NE(fd_stdout.value(), DeprecatedEchoFD(fd_stdout).value());
   dbus::FileDescriptor fd_stderr(2);
   fd_stderr.CheckValidity();
-  EXPECT_NE(fd_stderr.value(), EchoFD(fd_stderr).value());
+  EXPECT_NE(fd_stderr.value(), DeprecatedEchoFD(fd_stderr).value());
+}
+
+TEST_F(DBusMethodInvokerTest, TestFileDescriptors) {
+  // Passing a file descriptor over D-Bus would effectively duplicate the fd.
+  // So the resulting file descriptor value would be different but it still
+  // should be valid.
+  int fd_stdin = 0;
+  base::ScopedFD out_fd = EchoFD(fd_stdin);
+  EXPECT_NE(fd_stdin, out_fd.get());
+  EXPECT_TRUE(out_fd.is_valid());
+  int fd_stdout = 1;
+  out_fd = EchoFD(fd_stdout);
+  EXPECT_NE(fd_stdout, out_fd.get());
+  EXPECT_TRUE(out_fd.is_valid());
+  int fd_stderr = 2;
+  out_fd = EchoFD(fd_stderr);
+  EXPECT_NE(fd_stderr, out_fd.get());
+  EXPECT_TRUE(out_fd.is_valid());
 }
 
 //////////////////////////////////////////////////////////////////////////////
diff --git a/brillo/dbus/file_descriptor.h b/brillo/dbus/file_descriptor.h
new file mode 100644
index 0000000..f959d46
--- /dev/null
+++ b/brillo/dbus/file_descriptor.h
@@ -0,0 +1,29 @@
+// Copyright 2018 The Chromium OS 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 LIBBRILLO_BRILLO_DBUS_FILE_DESCRIPTOR_H_
+#define LIBBRILLO_BRILLO_DBUS_FILE_DESCRIPTOR_H_
+
+namespace brillo {
+namespace dbus_utils {
+
+// This struct wraps file descriptors to give them a type other than int.
+// 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.
+struct FileDescriptor {
+  FileDescriptor(int fd) : fd(fd) {}
+
+  inline FileDescriptor& operator=(int new_fd) {
+    fd = new_fd;
+    return *this;
+  }
+
+  int fd;
+};
+
+}  // namespace dbus_utils
+}  // namespace brillo
+
+#endif  // LIBBRILLO_BRILLO_DBUS_FILE_DESCRIPTOR_H_