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"
+#include "base/platform_file.h"
 #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;
       }
+      case UNIX_FD: {
+        CHECK(kDBusTypeUnixFdIsSupported);
+
+        FileDescriptor value;

[satorux1, 2012/03/28 20:49:37]

nit: value -> file_descriptor?

[Sam Leffler, 2012/03/28 22:06:33]

Done.

+        if (!reader->PopFileDescriptor(&value))
+          return kBrokenMessage;
+        output += indent + "fd#" +
+                  base::StringPrintf("%u", value.value()) + "\n";

[satorux1, 2012/03/28 20:49:37]

nit: %d? fd is an int.

[Sam Leffler, 2012/03/28 22:06:33]

Done.

+        break;
+      }
       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) {
+  CHECK(kDBusTypeUnixFdIsSupported);
+
+  base::PlatformFileInfo info;
+  int fd = value.value();
+  bool ok = base::GetPlatformFileInfo(fd, &info);
+  if (!ok || info.is_directory)

[satorux1, 2012/03/28 20:49:37]

These are different errors. !ok means it just failed to get the file info, which
likely means the fd is invalid. LOG(FATAL) in this case is rather questionable.

Trying to send an invalid fd is a bad idea, but it's the caller's fault, so I'd
go with something like:

if (ok && info.is_directory)
  LOG(FATAL) << "Attempt to pass invalid file descriptor";
// |ok| can be false, which means |fd| is invalid, but it's caller's fault,
// so we don't stop.

[Jorge Lucangeli Obes, 2012/03/28 21:33:37]

And in this case, we can display an error message specifically pointing out the
fact that this is a dir fd, and we don't want to send those around.

[Sam Leffler, 2012/03/28 22:06:33]

Yes they are different errors; the message is intentionally general to cover
both cases.  I think it is a bad idea to carry on with a known bad file
descriptor even though it is the fault of the caller.  I expect the dbus library
will abort if handed an invalid descriptor but there's no guarantee and if it
continues on then I can imaging difficult-to-debug problems that can be
trivially caught here.  I can spend some more code to print out the fd or treat
the two cases separately but not catching it here seems bad.

+    LOG(FATAL) << "Attempt to pass invalid file descriptor";

[satorux1, 2012/03/28 20:49:37]

Please add some comment why we are disallowing the directory fd passing.
otherwise, it looks rather cryptic

[Sam Leffler, 2012/03/28 22:06:33]

Done.

+  AppendBasic(DBUS_TYPE_UNIX_FD, &fd);
+}
+
 //
 // 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) {
+  CHECK(kDBusTypeUnixFdIsSupported);
+
+  int fd;

[satorux1, 2012/03/28 20:49:37]

nit: initialize it to -1 to be extra defensive.

[Sam Leffler, 2012/03/28 22:06:33]

Done.

+  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) {

[satorux1, 2012/03/28 20:49:37]

ditto. these are different errors.

[Sam Leffler, 2012/03/28 22:06:33]

see above

+    base::ClosePlatformFile(fd);
+    LOG(FATAL) << "Attempt to receive invalid file descriptor";
+    return false;  // NB: not reached
+  }
+  value->PutValue(fd);
+  return true;
+}
+
 }  // namespace dbus