Fix a null-pointer dereference in ChromeOS Bluetooth code.

device/bluetooth/bluetooth_adapter_profile_chromeos.cc

Index: device/bluetooth/bluetooth_adapter_profile_chromeos.cc
diff --git a/device/bluetooth/bluetooth_adapter_profile_chromeos.cc b/device/bluetooth/bluetooth_adapter_profile_chromeos.cc
index f8003043d63270a0e4671977f793e243b2e93fa3..207c55c4bad2bd952aba95776ead895cfde3303a 100644
--- a/device/bluetooth/bluetooth_adapter_profile_chromeos.cc
+++ b/device/bluetooth/bluetooth_adapter_profile_chromeos.cc
@@ -28,8 +28,9 @@ void BluetoothAdapterProfileChromeOS::Register(
       new BluetoothAdapterProfileChromeOS(uuid));
 
   VLOG(1) << "Registering profile: " << profile->object_path().value();
+  const dbus::ObjectPath& object_path = profile->object_path();
   DBusThreadManager::Get()->GetBluetoothProfileManagerClient()->RegisterProfile(
-      profile->object_path(),

[armansito, 2015/03/11 06:48:38]

I really don't see how this is a NULL pointer dereference. Is it that there are
some new semantics here that invalidates the return value since RegisterProfile
accepts a const ref? It used to simply copy the value and this used to work, but
calling this a NULL-pointer deref. is misleading.

Also, it disturbs me that we didn't catch this in a unit test, even though
FakeBluetoothProfileManagerClient::RegisterValue dereferences the object path.
Do you always see this crash or only occasionally?

[Ilya Sherman, 2015/03/11 09:23:54]

I agree that it's surprising that unit tests didn't catch this.  Let me try to
describe what the problem is, by way of a minimal example. Suppose we have this
code:

  void StartHere() {
    Foo* foo = new foo();
    ConsumeFoo(foo, NullOut(&foo));
  }
  
  void ConsumeFoo(Foo* foo, bool /* ignored */) {
    foo->DoSomething();
    delete foo;
  }
  
  bool NullOut(Foo** foo) {
    *foo = nullptr;
    return true;
  }

This program has undefined behavior, because the values passed to ConsumeFoo()
depend on the order in which the compiler evaluates the arguments.  If it
evaluates them LTR, then ConsumeFoo receives a non-null foo, and the boolean
value true.  On the other hand, if the compiler evaluates the arguments in RTL
order, then ConsumeFoo receives a null foo (as well as the same boolean value
true).

What's happening in the real code is that we call RegisterProfile() with
profile->object_path() as well as base::Bind(success_callback,
base::Passed(&profile)).  If the first argument is evaluated first, then
everything works out smoothly.  But if base::Bind argument is evaluated first,
then base::Passed() nulls out |profile|, and then profile->object_path() results
in a null dereference.

Hope that makes sense =)

[armansito, 2015/03/11 16:44:29]

OK, I guess that makes sense. I would elaborate a little more in the commit
message, since it's a very elusive bug.

Btw, what platform/compiler did you see this on? I'm trying to figure out why
the same thing didn't happen with the unit test.

[armansito, 2015/03/11 17:09:24]

Also, it's not technically a NULL-pointer dereference that's causing the crash
in this case, so I would explain in the commit message that a NULL pointer is
getting passed to BluetoothAdapterProfileChromeOS::object_path() which then
leads to a dereferencing of an invalid pointer deep down the call chain. And if
you could include the stack trace in the message, that would be great too.

Otherwise, good catch!

+      object_path,
       uuid.canonical_value(),
       options,
       base::Bind(success_callback, base::Passed(&profile)),