Change setters of dbus::Message to return false instead of aborting on errors

With this change, we can safely return error for invalid object path and service name.
It still crashes on invalid method name and interface name if we use MethodCall::MethodCall for setting those parameters.

BUG=128967
TEST=dbus_unittests

Review URL: https://chromiumcodereview.appspot.com/10409065

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@138441 0039d316-1c4b-4281-b951-d872f2087c98

7 files changed, 148 insertions, 47 deletions

diff --git a/dbus/end_to_end_async_unittest.cc b/dbus/end_to_end_async_unittest.cc
index a4438e4..d5e278f 100644
--- a/dbus/end_to_end_async_unittest.cc
+++ b/dbus/end_to_end_async_unittest.cc
@@ -453,6 +453,44 @@ TEST_F(EndToEndAsyncTest, BrokenMethodWithErrorCallback) {
   ASSERT_EQ(DBUS_ERROR_FAILED, error_names_[0]);
 }
 
+TEST_F(EndToEndAsyncTest, InvalidObjectPath) {
+  // Trailing '/' is only allowed for the root path.
+  const dbus::ObjectPath invalid_object_path("/org/chromium/TestObject/");
+
+  // Replace object proxy with new one.
+  object_proxy_ = bus_->GetObjectProxy("org.chromium.TestService",
+                                       invalid_object_path);
+
+  dbus::MethodCall method_call("org.chromium.TestInterface", "Echo");
+
+  const int timeout_ms = dbus::ObjectProxy::TIMEOUT_USE_DEFAULT;
+  CallMethodWithErrorCallback(&method_call, timeout_ms);
+  WaitForErrors(1);
+
+  // Should fail because of the invalid path.
+  ASSERT_TRUE(response_strings_.empty());
+  ASSERT_EQ("", error_names_[0]);
+}
+
+TEST_F(EndToEndAsyncTest, InvalidServiceName) {
+  // Bus name cannot contain '/'.
+  const std::string invalid_service_name = ":1/2";
+
+  // Replace object proxy with new one.
+  object_proxy_ = bus_->GetObjectProxy(
+      invalid_service_name, dbus::ObjectPath("org.chromium.TestObject"));
+
+  dbus::MethodCall method_call("org.chromium.TestInterface", "Echo");
+
+  const int timeout_ms = dbus::ObjectProxy::TIMEOUT_USE_DEFAULT;
+  CallMethodWithErrorCallback(&method_call, timeout_ms);
+  WaitForErrors(1);
+
+  // Should fail because of the invalid bus name.
+  ASSERT_TRUE(response_strings_.empty());
+  ASSERT_EQ("", error_names_[0]);
+}
+
 TEST_F(EndToEndAsyncTest, EmptyResponseCallback) {
   const char* kHello = "hello";
 
diff --git a/dbus/end_to_end_sync_unittest.cc b/dbus/end_to_end_sync_unittest.cc
index 26a9011..9cddea4 100644
--- a/dbus/end_to_end_sync_unittest.cc
+++ b/dbus/end_to_end_sync_unittest.cc
@@ -104,3 +104,35 @@ TEST_F(EndToEndSyncTest, BrokenMethod) {
       object_proxy_->CallMethodAndBlock(&method_call, timeout_ms));
   ASSERT_FALSE(response.get());
 }
+
+TEST_F(EndToEndSyncTest, InvalidObjectPath) {
+  // Trailing '/' is only allowed for the root path.
+  const dbus::ObjectPath invalid_object_path("/org/chromium/TestObject/");
+
+  // Replace object proxy with new one.
+  object_proxy_ = client_bus_->GetObjectProxy("org.chromium.TestService",
+                                              invalid_object_path);
+
+  dbus::MethodCall method_call("org.chromium.TestInterface", "Echo");
+
+  const int timeout_ms = dbus::ObjectProxy::TIMEOUT_USE_DEFAULT;
+  scoped_ptr<dbus::Response> response(
+      object_proxy_->CallMethodAndBlock(&method_call, timeout_ms));
+  ASSERT_FALSE(response.get());
+}
+
+TEST_F(EndToEndSyncTest, InvalidServiceName) {
+  // Bus name cannot contain '/'.
+  const std::string invalid_service_name = ":1/2";
+
+  // Replace object proxy with new one.
+  object_proxy_ = client_bus_->GetObjectProxy(
+      invalid_service_name, dbus::ObjectPath("org.chromium.TestObject"));
+
+  dbus::MethodCall method_call("org.chromium.TestInterface", "Echo");
+
+  const int timeout_ms = dbus::ObjectProxy::TIMEOUT_USE_DEFAULT;
+  scoped_ptr<dbus::Response> response(
+      object_proxy_->CallMethodAndBlock(&method_call, timeout_ms));
+  ASSERT_FALSE(response.get());
+}
diff --git a/dbus/exported_object.cc b/dbus/exported_object.cc
index 7c4bada..f9ddb62 100644
--- a/dbus/exported_object.cc
+++ b/dbus/exported_object.cc
@@ -90,7 +90,7 @@ void ExportedObject::ExportMethod(const std::string& interface_name,
 void ExportedObject::SendSignal(Signal* signal) {
   // For signals, the object path should be set to the path to the sender
   // object, which is this exported object here.
-  signal->SetPath(object_path_);
+  CHECK(signal->SetPath(object_path_));
 
   // Increment the reference count so we can safely reference the
   // underlying signal message until the signal sending is complete. This
diff --git a/dbus/message.cc b/dbus/message.cc
index 400c1bc..5b45d42 100644
--- a/dbus/message.cc
+++ b/dbus/message.cc
@@ -249,40 +249,28 @@ std::string Message::ToString() {
   return headers + "\n" + ToStringInternal("", &reader);
 }
 
-void Message::SetDestination(const std::string& destination) {
-  const bool success = dbus_message_set_destination(raw_message_,
-                                                    destination.c_str());
-  CHECK(success) << "Unable to allocate memory";
+bool Message::SetDestination(const std::string& destination) {
+  return dbus_message_set_destination(raw_message_, destination.c_str());
 }
 
-void Message::SetPath(const ObjectPath& path) {
-  const bool success = dbus_message_set_path(raw_message_,
-                                             path.value().c_str());
-  CHECK(success) << "Unable to allocate memory";
+bool Message::SetPath(const ObjectPath& path) {
+  return dbus_message_set_path(raw_message_, path.value().c_str());
 }
 
-void Message::SetInterface(const std::string& interface) {
-  const bool success = dbus_message_set_interface(raw_message_,
-                                                  interface.c_str());
-  CHECK(success) << "Unable to allocate memory";
+bool Message::SetInterface(const std::string& interface) {
+  return dbus_message_set_interface(raw_message_, interface.c_str());
 }
 
-void Message::SetMember(const std::string& member) {
-  const bool success = dbus_message_set_member(raw_message_,
-                                               member.c_str());
-  CHECK(success) << "Unable to allocate memory";
+bool Message::SetMember(const std::string& member) {
+  return dbus_message_set_member(raw_message_, member.c_str());
 }
 
-void Message::SetErrorName(const std::string& error_name) {
-  const bool success = dbus_message_set_error_name(raw_message_,
-                                                   error_name.c_str());
-  CHECK(success) << "Unable to allocate memory";
+bool Message::SetErrorName(const std::string& error_name) {
+  return dbus_message_set_error_name(raw_message_, error_name.c_str());
 }
 
-void Message::SetSender(const std::string& sender) {
-  const bool success = dbus_message_set_sender(raw_message_,
-                                               sender.c_str());
-  CHECK(success) << "Unable to allocate memory";
+bool Message::SetSender(const std::string& sender) {
+  return dbus_message_set_sender(raw_message_, sender.c_str());
 }
 
 void Message::SetSerial(uint32 serial) {
@@ -345,8 +333,8 @@ MethodCall::MethodCall(const std::string& interface_name,
     : Message() {
   Init(dbus_message_new(DBUS_MESSAGE_TYPE_METHOD_CALL));
 
-  SetInterface(interface_name);
-  SetMember(method_name);
+  CHECK(SetInterface(interface_name));
+  CHECK(SetMember(method_name));
 }
 
 MethodCall::MethodCall() : Message() {
@@ -368,8 +356,8 @@ Signal::Signal(const std::string& interface_name,
     : Message() {
   Init(dbus_message_new(DBUS_MESSAGE_TYPE_SIGNAL));
 
-  SetInterface(interface_name);
-  SetMember(method_name);
+  CHECK(SetInterface(interface_name));
+  CHECK(SetMember(method_name));
 }
 
 Signal::Signal() : Message() {
diff --git a/dbus/message.h b/dbus/message.h
index 75e4045..0b9834d 100644
--- a/dbus/message.h
+++ b/dbus/message.h
@@ -90,12 +90,12 @@ class Message {
   DBusMessage* raw_message() { return raw_message_; }
 
   // Sets the destination, the path, the interface, the member, etc.
-  void SetDestination(const std::string& destination);
-  void SetPath(const ObjectPath& path);
-  void SetInterface(const std::string& interface);
-  void SetMember(const std::string& member);
-  void SetErrorName(const std::string& error_name);
-  void SetSender(const std::string& sender);
+  bool SetDestination(const std::string& destination);
+  bool SetPath(const ObjectPath& path);
+  bool SetInterface(const std::string& interface);
+  bool SetMember(const std::string& member);
+  bool SetErrorName(const std::string& error_name);
+  bool SetSender(const std::string& sender);
   void SetSerial(uint32 serial);
   void SetReplySerial(uint32 reply_serial);
   // SetSignature() does not exist as we cannot do it.
diff --git a/dbus/message_unittest.cc b/dbus/message_unittest.cc
index 9d87c2a..d78d90f2 100644
--- a/dbus/message_unittest.cc
+++ b/dbus/message_unittest.cc
@@ -565,12 +565,12 @@ TEST(MessageTest, GetAndSetHeaders) {
   EXPECT_EQ(0U, message->GetSerial());
   EXPECT_EQ(0U, message->GetReplySerial());
 
-  message->SetDestination("org.chromium.destination");
-  message->SetPath(dbus::ObjectPath("/org/chromium/path"));
-  message->SetInterface("org.chromium.interface");
-  message->SetMember("member");
-  message->SetErrorName("org.chromium.error");
-  message->SetSender(":1.2");
+  EXPECT_TRUE(message->SetDestination("org.chromium.destination"));
+  EXPECT_TRUE(message->SetPath(dbus::ObjectPath("/org/chromium/path")));
+  EXPECT_TRUE(message->SetInterface("org.chromium.interface"));
+  EXPECT_TRUE(message->SetMember("member"));
+  EXPECT_TRUE(message->SetErrorName("org.chromium.error"));
+  EXPECT_TRUE(message->SetSender(":1.2"));
   message->SetSerial(123);
   message->SetReplySerial(456);
 
@@ -583,3 +583,33 @@ TEST(MessageTest, GetAndSetHeaders) {
   EXPECT_EQ(123U, message->GetSerial());
   EXPECT_EQ(456U, message->GetReplySerial());
 }
+
+TEST(MessageTest, SetInvalidHeaders) {
+  scoped_ptr<dbus::Response> message(dbus::Response::CreateEmpty());
+  EXPECT_EQ("", message->GetDestination());
+  EXPECT_EQ(dbus::ObjectPath(""), message->GetPath());
+  EXPECT_EQ("", message->GetInterface());
+  EXPECT_EQ("", message->GetMember());
+  EXPECT_EQ("", message->GetErrorName());
+  EXPECT_EQ("", message->GetSender());
+
+  // Empty element between periods.
+  EXPECT_FALSE(message->SetDestination("org..chromium"));
+  // Trailing '/' is only allowed for the root path.
+  EXPECT_FALSE(message->SetPath(dbus::ObjectPath("/org/chromium/")));
+  // Interface name cannot contain '/'.
+  EXPECT_FALSE(message->SetInterface("org/chromium/interface"));
+  // Member name cannot begin with a digit.
+  EXPECT_FALSE(message->SetMember("1member"));
+  // Error name cannot begin with a period.
+  EXPECT_FALSE(message->SetErrorName(".org.chromium.error"));
+  // Disallowed characters.
+  EXPECT_FALSE(message->SetSender("?!#*"));
+
+  EXPECT_EQ("", message->GetDestination());
+  EXPECT_EQ(dbus::ObjectPath(""), message->GetPath());
+  EXPECT_EQ("", message->GetInterface());
+  EXPECT_EQ("", message->GetMember());
+  EXPECT_EQ("", message->GetErrorName());
+  EXPECT_EQ("", message->GetSender());
+}
diff --git a/dbus/object_proxy.cc b/dbus/object_proxy.cc
index 051d8f1..4d7aa25 100644
--- a/dbus/object_proxy.cc
+++ b/dbus/object_proxy.cc
@@ -63,11 +63,11 @@ Response* ObjectProxy::CallMethodAndBlock(MethodCall* method_call,
                                           int timeout_ms) {
   bus_->AssertOnDBusThread();
 
-  if (!bus_->Connect())
+  if (!bus_->Connect() ||
+      !method_call->SetDestination(service_name_) ||
+      !method_call->SetPath(object_path_))
     return NULL;
 
-  method_call->SetDestination(service_name_);
-  method_call->SetPath(object_path_);
   DBusMessage* request_message = method_call->raw_message();
 
   ScopedDBusError error;
@@ -108,15 +108,28 @@ void ObjectProxy::CallMethodWithErrorCallback(MethodCall* method_call,
                                               ErrorCallback error_callback) {
   bus_->AssertOnOriginThread();
 
-  method_call->SetDestination(service_name_);
-  method_call->SetPath(object_path_);
+  const base::TimeTicks start_time = base::TimeTicks::Now();
+
+  if (!method_call->SetDestination(service_name_) ||
+      !method_call->SetPath(object_path_)) {
+    // In case of a failure, run the error callback with NULL.
+    DBusMessage* response_message = NULL;
+    base::Closure task = base::Bind(&ObjectProxy::RunResponseCallback,
+                                    this,
+                                    callback,
+                                    error_callback,
+                                    start_time,
+                                    response_message);
+    bus_->PostTaskToOriginThread(FROM_HERE, task);
+    return;
+  }
+
   // Increment the reference count so we can safely reference the
   // underlying request message until the method call is complete. This
   // will be unref'ed in StartAsyncMethodCall().
   DBusMessage* request_message = method_call->raw_message();
   dbus_message_ref(request_message);
 
-  const base::TimeTicks start_time = base::TimeTicks::Now();
   base::Closure task = base::Bind(&ObjectProxy::StartAsyncMethodCall,
                                   this,
                                   timeout_ms,