Fix a null-pointer dereference in ChromeOS Bluetooth code.

Fix a null-pointer dereference in ChromeOS Bluetooth code.

The code had undefined behavior, depending on what order the compiler chose to evaluate the arguments in.  Specifically, the call to RegisterProfile() required evaluation of two arguments: |profile->object_path()| and |base::Bind(success_callback, base::Passed(&profile))|.  If the latter was evaluated first, then |profile| would be null by the time that the prior was evaluated.

The crash stack is:

Program received signal SIGSEGV, Segmentation fault.
std::string::compare() const ()
StartsWithASCII()
dbus::IsValidObjectPath()
dbus::MessageWriter::AppendObjectPath()
chromeos::BluetoothProfileManagerClientImpl::RegisterProfile()
chromeos::BluetoothAdapterProfileChromeOS::Register()
chromeos::BluetoothAdapterChromeOS::UseProfile()
chromeos::BluetoothSocketChromeOS::RegisterProfile()

BUG=457978
TEST=(see bug, comment #14)
R=armansito@chromium.org, jamuraa@chromium.org

Committed: https://crrev.com/847466627013483020c6683c303752b6fab97b97
Cr-Commit-Position: refs/heads/master@{#320139}

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

https://codereview.chromium.org/997023002/diff/1/device/bluetooth/bluetooth_a...
File device/bluetooth/bluetooth_adapter_profile_chromeos.cc (left):

https://codereview.chromium.org/997023002/diff/1/device/bluetooth/bluetooth_a...
device/bluetooth/bluetooth_adapter_profile_chromeos.cc:32:
profile->object_path(),
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]

https://codereview.chromium.org/997023002/diff/1/device/bluetooth/bluetooth_a...
File device/bluetooth/bluetooth_adapter_profile_chromeos.cc (left):

https://codereview.chromium.org/997023002/diff/1/device/bluetooth/bluetooth_a...
device/bluetooth/bluetooth_adapter_profile_chromeos.cc:32:
profile->object_path(),
On 2015/03/11 06:48:38, armansito wrote:
> 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?

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 =)

[Marie Janssen, 2015-03-11 14:03:30]

good catch

lgtm

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

https://codereview.chromium.org/997023002/diff/1/device/bluetooth/bluetooth_a...
File device/bluetooth/bluetooth_adapter_profile_chromeos.cc (left):

https://codereview.chromium.org/997023002/diff/1/device/bluetooth/bluetooth_a...
device/bluetooth/bluetooth_adapter_profile_chromeos.cc:32:
profile->object_path(),
On 2015/03/11 09:23:54, Ilya Sherman wrote:
> On 2015/03/11 06:48:38, armansito wrote:
> > 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?
> 
> 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 =)

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:25]

https://codereview.chromium.org/997023002/diff/1/device/bluetooth/bluetooth_a...
File device/bluetooth/bluetooth_adapter_profile_chromeos.cc (left):

https://codereview.chromium.org/997023002/diff/1/device/bluetooth/bluetooth_a...
device/bluetooth/bluetooth_adapter_profile_chromeos.cc:32:
profile->object_path(),
On 2015/03/11 16:44:29, armansito wrote:
> On 2015/03/11 09:23:54, Ilya Sherman wrote:
> > On 2015/03/11 06:48:38, armansito wrote:
> > > 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?
> > 
> > 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 =)
> 
> 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.

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!

[Ilya Sherman, 2015-03-11 19:02:25]

I've expanded the CL description.  Sending to the CQ.

Michael, could you please work with Arman to figure out why this didn't get
caught by unit tests?  Thanks.

[Ilya Sherman, 2015-03-11 19:02:33]

The CQ bit was checked by isherman@chromium.org

[Ilya Sherman, 2015-03-11 19:02:46]

The CQ bit was unchecked by isherman@chromium.org

[Ilya Sherman, 2015-03-11 19:03:00]

On 2015/03/11 19:02:25, Ilya Sherman wrote:
> I've expanded the CL description.  Sending to the CQ.

Oh, wait, I still need your LG, Arman =)

[armansito, 2015-03-11 19:27:08]

lgtm, thanks! Let's also try to get this into 42 asap.

[Ilya Sherman, 2015-03-11 19:40:24]

The CQ bit was checked by isherman@chromium.org

[commit-bot: I haz the power, 2015-03-11 19:42:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/997023002/1

[commit-bot: I haz the power, 2015-03-11 20:36:16]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2015-03-11 20:37:10]

Patchset 1 (id:??) landed as
https://crrev.com/847466627013483020c6683c303752b6fab97b97
Cr-Commit-Position: refs/heads/master@{#320139}