Fix a null-pointer dereference in ChromeOS Bluetooth code.

device/bluetooth/bluetooth_adapter_profile_chromeos.cc

 // Copyright 2015 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 "device/bluetooth/bluetooth_adapter_profile_chromeos.h"
 
 #include <string>
 
 #include "base/bind.h"
 #include "base/logging.h"
@@ skipping 10 matching lines @@
 // static
 void BluetoothAdapterProfileChromeOS::Register(
     const device::BluetoothUUID& uuid,
     const BluetoothProfileManagerClient::Options& options,
     const ProfileRegisteredCallback& success_callback,
     const BluetoothProfileManagerClient::ErrorCallback& error_callback) {
   scoped_ptr<BluetoothAdapterProfileChromeOS> profile(
       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)),
       error_callback);
 }
 
 BluetoothAdapterProfileChromeOS::BluetoothAdapterProfileChromeOS(
     const device::BluetoothUUID& uuid)
     : uuid_(uuid), weak_ptr_factory_(this) {
   std::string uuid_path;
@@ skipping 109 matching lines @@
   // Cancel() should only go to a delegate accepting connections.
   if (delegates_.find("") == delegates_.end()) {
     VLOG(1) << object_path_.value() << ": Cancel with no delegate!";
     return;
   }
 
   delegates_[""]->Cancel();
 }
 
 }  // namespace chromeos