dbus: add support for passing file descriptors

dbus/message.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 "dbus/message.h"
 
 #include <string>
 
 #include "base/basictypes.h"
 #include "base/format_macros.h"
 #include "base/logging.h"
+#if defined(DBUS_TYPE_UNIX_FD)
+#include "base/platform_file.h"
+#endif
 #include "base/stringprintf.h"
 #include "dbus/object_path.h"
 #include "third_party/protobuf/src/google/protobuf/message_lite.h"
 
 namespace {
 
 // Appends the header name and the value to |output|, if the value is
 // not empty.
 static void AppendStringHeader(const std::string& header_name,
                                const std::string& header_value,
@@ skipping 170 matching lines @@
         break;
       }
       case VARIANT: {
         MessageReader sub_reader(this);
         if (!reader->PopVariant(&sub_reader))
           return kBrokenMessage;
         output += indent + "variant ";
         output += ToStringInternal(indent + "  ", &sub_reader);
         break;
       }
+#if defined(DBUS_TYPE_UNIX_FD)

[satorux1, 2012/03/28 00:32:43]

Hmm, there seems to be many #ifdefs based on DBUS_TYPE_UNIX_FD added in this
patch. 

I'd rather like to minimize #ifdefs. What about doing this in message.h:

// DBUS_TYPE_UNIX_FD was added in D-Bus version x.x.x
#if defined(DBUS_TYPE_UNIX_FD)
const bool kDBusTypeUnixFdIsSupported = true;
#else
const bool kDBusTypeUnixFdIsSupported = false;
#define DBUS_TYPE_UNIX_FD      ((int) 'h')
#endif

Then, we can use kDBusTypeUnixFdIsSupported like

  CHECK(kDBusTypeUnixFdIsSupported);

[Sam Leffler, 2012/03/28 17:28:09]

You cannot do this unless you define UNIX_FD in the unsupported case.  There are
4 #ifdefs in this file.  1 is required because UNIX_FD is undefined otherwise. 2
can be handled as you request but result in a bunch of code being compiled in
that will never be used/work.  1 is around the #include of platform_file.h that
I intentionally bracketed to show it was only needed for UNIX_FD support (maybe
a style violation?).

[satorux1, 2012/03/28 17:41:54]

My proposal was to define DBUS_TYPE_UNIX_FD for the unsupported case. Please see
my original comment.



  There are
I'm fine with that. The unused code is fairly small, but #ifdefs make code
harder to maintain. IMHO, the latter is worse in this case.

[Sam Leffler, 2012/03/28 19:25:27]

Done.

+      case UNIX_FD: {
+        FileDescriptor value;
+        if (!reader->PopFileDescriptor(&value))
+          return kBrokenMessage;
+        output += indent + "fd#" +
+                  base::StringPrintf("%u", value.value()) + "\n";
+        break;
+      }
+#endif
       default:
         LOG(FATAL) << "Unknown type: " << type;
     }
   }
   return output;
 }
 
 // The returned string consists of message headers such as
 // destination if any, followed by a blank line, and the message
 // payload. For example, a MethodCall's ToString() will look like:
@@ skipping 458 matching lines @@
 }
 
 void MessageWriter::AppendVariantOfBasic(int dbus_type, const void* value) {
   const std::string signature = base::StringPrintf("%c", dbus_type);
   MessageWriter variant_writer(message_);
   OpenVariant(signature, &variant_writer);
   variant_writer.AppendBasic(dbus_type, value);
   CloseContainer(&variant_writer);
 }
 
+void MessageWriter::AppendFileDescriptor(const FileDescriptor& value) {

[satorux1, 2012/03/28 00:32:43]

If we are to go with the idea described above, you can just put

  CHECK(kDBusTypeUnixFdIsSupported);

and get rid of the #ifdef.

+#if defined(DBUS_TYPE_UNIX_FD)
+  base::PlatformFileInfo info;
+  int fd = value.value();
+  bool ok = base::GetPlatformFileInfo(fd, &info);
+  if (!ok || info.is_directory)
+    LOG(FATAL) << "Attempt to pass invalid file descriptor";
+  AppendBasic(DBUS_TYPE_UNIX_FD, &fd);
+#else
+  CHECK(false) << "File descriptor passing not supported";
+#endif
+}
+
 //
 // MessageReader implementation.
 //
 
 MessageReader::MessageReader(Message* message)
     : message_(message) {
   memset(&raw_message_iter_, 0, sizeof(raw_message_iter_));
   if (message)
     dbus_message_iter_init(message_->raw_message(), &raw_message_iter_);
 }
@@ skipping 239 matching lines @@
   return true;
 }
 
 bool MessageReader::PopVariantOfBasic(int dbus_type, void* value) {
   dbus::MessageReader variant_reader(message_);
   if (!PopVariant(&variant_reader))
     return false;
   return variant_reader.PopBasic(dbus_type, value);
 }
 
+bool MessageReader::PopFileDescriptor(FileDescriptor* value) {
+#if defined(DBUS_TYPE_UNIX_FD)
+  int fd;
+  const bool success = PopBasic(DBUS_TYPE_UNIX_FD, &fd);
+  if (!success)
+    return false;
+
+  base::PlatformFileInfo info;
+  bool ok = base::GetPlatformFileInfo(fd, &info);
+  if (!ok || info.is_directory) {
+    LOG(FATAL) << "Attempt to receive invalid file descriptor";
+    return false;  // NB: not reached
+  }

[keybuk, 2012/03/27 22:36:35]

doesn't close the file descriptor before returning

also what if base::PlatformFileInfo constructor, base::GetPlatformFileInfo or
LOG() throws an exception?

[Sam Leffler, 2012/03/27 23:20:40]

Not sure where you're referring.  fd is not closed because it's returned via
value->PutValue.  The error case will abort the process because LOG(FATAL) does
that.

[keybuk, 2012/03/28 17:53:12]

it does that now, it might not in future; likewise exception handling...
basically you can leak an fd between :968 and :978 if things change under you,
your code should defend against that

[Sam Leffler, 2012/03/28 19:25:27]

Done.

+  value->PutValue(fd);
+  return true;
+#else
+  CHECK(false) << "File descriptor passing not supported";
+  return false;
+#endif
+}
+
 }  // namespace dbus