1072021 - Bluetooth address is assigned wrong when A2DP is disconnected by user.

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Bluetooth, defect, P1)

Description

[Seil Park]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:32.0) Gecko/20100101 Firefox/32.0
Build ID: 20140917194002

Steps to reproduce:

1time issue


Actual results:

Bluetooth address is assigned wrong  when A2DP is disconnected by user.
By adb log, BT address is crashed in AudioPolicyManager.cpp

09-19 16:27:18.438 221 742 E AudioPolicyManager: setDeviceConnectionState() invalid address: @w!걋&<?닷칮-召?財, device_address 0x0

Comment 1

[Seil Park]

I reviewed v2.0 and finded the point of problum in AudioManager.cpp(gonk)

"aAddress"`s parameter is point object and handle aAddress.get() by NewRunnableFunction.
If "aAddress" is released, address parameter will be garbage after 1000ms in ProcessDelayedA2dpRoute()

===============================================================
AudioManager::HandleBluetoothStatusChanged(nsISupports* aSubject,
                                           const char* aTopic,
                                           const nsCString aAddress)
{
  //.....
  } else if (!strcmp(aTopic, BLUETOOTH_A2DP_STATUS_CHANGED_ID)) {
    if (audioState == AUDIO_POLICY_DEVICE_STATE_UNAVAILABLE && sA2dpSwitchDone) {
      MessageLoop::current()->PostDelayedTask(
        FROM_HERE, NewRunnableFunction(&ProcessDelayedA2dpRoute, audioState, aAddress.get()), 1000);
      sA2dpSwitchDone = false;
}

static void ProcessDelayedA2dpRoute(audio_policy_dev_state_t aState, const char *aAddress)
{
 // aAdress will be garbage.
}

Comment 2

[Seil Park]

AudioPolicyService can`t read BT address and can`t route device. So, This case always keeps A2DP device after user is disconnecting Blutooth. 
It needs to restore bt address string.

Comment 3

[Rachelle Yang [:ryang][[e-mail]]]

Hi Shawn , Could you please kindly take a look at this and also kindly feedback the comment1 ? 
Thank you very much!!

Comment 4

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Sure. I will take a look.

Comment 5

[Star Cheng [:scheng]]

I will investigate AudioManager.

Comment 7

[Eric Chou [:ericchou] [:echou]]

[Blocking Requested - why for this release]:
Dup of a 2.1+ bug 1070919.

Comment 8

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

triage: Bug 1070919 is 2.1+

Comment 9

[Wayne Chen [:xwaynec]]

Attachment: bug-1072021.patch

Comment 10

[Eric Chou [:ericchou] [:echou]]

Comment on attachment 8498754 [details] [diff] [review]
bug-1072021.patch

Review of attachment 8498754 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the nit addressed.

::: dom/system/gonk/AudioManager.cpp
@@ +198,5 @@
>    InternalSetAudioRoutes(aState);
>    sSwitchDone = true;
>  }
>  
> +static void ProcessDelayedA2dpRoute(audio_policy_dev_state_t aState, const nsCString aAddress)

nit: we usually use |const nsACString&| for arguments.

Comment 11

[Chris Lord [:cwiiis]]

Comment on attachment 8498754 [details] [diff] [review]
bug-1072021.patch

Review of attachment 8498754 [details] [diff] [review]:
-----------------------------------------------------------------

Fly-by feedback, if I may be so bold - I think there are issues that need addressing here.

::: dom/system/gonk/AudioManager.cpp
@@ +279,5 @@
>      }
>    } else if (!strcmp(aTopic, BLUETOOTH_A2DP_STATUS_CHANGED_ID)) {
>      if (audioState == AUDIO_POLICY_DEVICE_STATE_UNAVAILABLE && sA2dpSwitchDone) {
>        MessageLoop::current()->PostDelayedTask(
> +        FROM_HERE, NewRunnableFunction(&ProcessDelayedA2dpRoute, audioState, aAddress), 1000);

Seeing what the problem is, is this actually a reasonable fix? What owns aAddress, is it ok to assume that it will stay alive for a second? In fact, reading around a bit, I can see that it's just the return value of NS_ConvertUTF16toUTF8, so as soon as anything else calls this function, the address will be corrupt again won't it? It'd be safer to copy the string and free it in the callback.

Also, what happens if the device connection state changes before this runnable is processed, there doesn't seem to be any cancelling of this function in that situation? Theoretically, you could disconnect a device and reconnect another one quickly and the callback would then disconnect it, I suppose (or leave it in an inconsistent state?)

It's probably worth keeping a static variable around with the callback id so it can be cancelled in the situation that the connection state changes before it gets processed.

Comment 12

[Chris Lord [:cwiiis]]

Comment on attachment 8498754 [details] [diff] [review]
bug-1072021.patch

Sorry, bugzilla conflict, didn't mean to remove review (though I'm concerned about this code being checked in as is).

Comment 13

[Eric Chou [:ericchou] [:echou]]

Hi Chris,

Thanks for your comment. Replied inline.

> 
> ::: dom/system/gonk/AudioManager.cpp
> @@ +279,5 @@
> >      }
> >    } else if (!strcmp(aTopic, BLUETOOTH_A2DP_STATUS_CHANGED_ID)) {
> >      if (audioState == AUDIO_POLICY_DEVICE_STATE_UNAVAILABLE && sA2dpSwitchDone) {
> >        MessageLoop::current()->PostDelayedTask(
> > +        FROM_HERE, NewRunnableFunction(&ProcessDelayedA2dpRoute, audioState, aAddress), 1000);
> 
> Seeing what the problem is, is this actually a reasonable fix? What owns
> aAddress, is it ok to assume that it will stay alive for a second? 
> In fact, reading around a bit, I can see that it's just the return value of
> NS_ConvertUTF16toUTF8, so as soon as anything else calls this function, the
> address will be corrupt again won't it? It'd be safer to copy the string and
> free it in the callback.
>

The value of |aAddress| should be held by nsCString, which is ref-counted by default, so the only thing that we need to care about is what if the original string would be deleted or modified. The original content is held by BluetoothA2dpManager::mAddress.

Please correct me if my understanding was wrong. :)

> Also, what happens if the device connection state changes before this
> runnable is processed, there doesn't seem to be any cancelling of this
> function in that situation? Theoretically, you could disconnect a device and
> reconnect another one quickly and the callback would then disconnect it, I
> suppose (or leave it in an inconsistent state?)
>
> It's probably worth keeping a static variable around with the callback id so
> it can be cancelled in the situation that the connection state changes
> before it gets processed.

We do have a static variable "sA2dpSwitchDone" to do protection here. I know that it might not be able to give full protection, but at least we haven't heard of any problems which are caused by not canceling the queued task. Chris, do you think we need to do more for these cases? I think a follow-up bug would be a good idea. 

Having said that, I would like to propose that we still land the patch on 2.1 since the original code really has a serious problem (use-after-free) and the patch has fixed that.

Comment 14

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Hi

> ::: dom/system/gonk/AudioManager.cpp
> @@ +279,5 @@
> >      }
> >    } else if (!strcmp(aTopic, BLUETOOTH_A2DP_STATUS_CHANGED_ID)) {
> >      if (audioState == AUDIO_POLICY_DEVICE_STATE_UNAVAILABLE && sA2dpSwitchDone) {
> >        MessageLoop::current()->PostDelayedTask(
> > +        FROM_HERE, NewRunnableFunction(&ProcessDelayedA2dpRoute, audioState, aAddress), 1000);
> 
> Seeing what the problem is, is this actually a reasonable fix? What owns
> aAddress, is it ok to assume that it will stay alive for a second? In fact,
> reading around a bit, I can see that it's just the return value of
> NS_ConvertUTF16toUTF8, so as soon as anything else calls this function, the
> address will be corrupt again won't it? It'd be safer to copy the string and
> free it in the callback.

|NS_ConvertUTF16toUTF8| is a class constructor, not a function. The class inherits from |nsACString|, so this code should be safe.

Comment 15

[Chris Lord [:cwiiis]]

So, bear in mind that I've never been particularly knowledgeable about our string classes, and if you're pretty sure about any of this, please feel free to discard my comments/correct me :) That said;

(In reply to Eric Chou [:ericchou] [:echou] from comment #13)
> > ::: dom/system/gonk/AudioManager.cpp
> > @@ +279,5 @@
> > >      }
> > >    } else if (!strcmp(aTopic, BLUETOOTH_A2DP_STATUS_CHANGED_ID)) {
> > >      if (audioState == AUDIO_POLICY_DEVICE_STATE_UNAVAILABLE && sA2dpSwitchDone) {
> > >        MessageLoop::current()->PostDelayedTask(
> > > +        FROM_HERE, NewRunnableFunction(&ProcessDelayedA2dpRoute, audioState, aAddress), 1000);
> > 
> > Seeing what the problem is, is this actually a reasonable fix? What owns
> > aAddress, is it ok to assume that it will stay alive for a second? 
> > In fact, reading around a bit, I can see that it's just the return value of
> > NS_ConvertUTF16toUTF8, so as soon as anything else calls this function, the
> > address will be corrupt again won't it? It'd be safer to copy the string and
> > free it in the callback.
> >
> 
> The value of |aAddress| should be held by nsCString, which is ref-counted by
> default, so the only thing that we need to care about is what if the
> original string would be deleted or modified. The original content is held
> by BluetoothA2dpManager::mAddress.
> 
> Please correct me if my understanding was wrong. :)

So, just spent longer than I expected reading through the string code and I'm still not sure this is safe. nsCString isn't ref-counted and may or may not share its buffer with the assigning string, depending on various flags set. A literal string is used and NS_ConvertUTF16toUTF8 is in-place, so the string won't be copied at any point, I don't think (the part about NS_ConvertUTF16toUTF8 I was wrong about) - So this code now relies on aData in Observe staying alive, which you're saying comes from mAddress? (mAddress on what?) This is safe if this mAddress is guaranteed to stay alive for the duration, but it seems very delicate to me... It feels like a good idea for this to be documented if that's the case, but it also seems like for the cost of a string copy, this code would be a lot more fool-proof.

> We do have a static variable "sA2dpSwitchDone" to do protection here. I know
> that it might not be able to give full protection, but at least we haven't
> heard of any problems which are caused by not canceling the queued task.
> Chris, do you think we need to do more for these cases? I think a follow-up
> bug would be a good idea. 

I'm not sure how, but I totally missed this - my worry here is completely unwarranted :)

> Having said that, I would like to propose that we still land the patch on
> 2.1 since the original code really has a serious problem (use-after-free)
> and the patch has fixed that.

What doesn't convince me is that all this does is move the get from one place to another, there are no refcounts so I don't see how this is actually functionally different from what was there before. Has this been tested to actually fix the issue? If it does, are we certain there are no leaks? If it fixes the issue, I expect it's because at some point a copy is occurring, and if that's happening, it might as well be made explicit so the code becomes less opaque.

Comment 16

[Wayne Chen [:xwaynec]]

https://tbpl.mozilla.org/?tree=Try&rev=57304d1c2378

Comment 17

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

nsCString is not ref-counted, however nsStringBuffer is ref-counted
void
nsStringBuffer::Release()
{
  int32_t count = --mRefCount;
  NS_LOG_RELEASE(this, count, "nsStringBuffer");
  if (count == 0) {
    STRING_STAT_INCREMENT(Free);
    free(this); // we were allocated with |malloc|
  }
}

Comment 18

[Chris Lord [:cwiiis]]

(In reply to Shawn Huang [:shawnjohnjr] from comment #17)
> nsCString is not ref-counted, however nsStringBuffer is ref-counted
> void
> nsStringBuffer::Release()
> {
>   int32_t count = --mRefCount;
>   NS_LOG_RELEASE(this, count, "nsStringBuffer");
>   if (count == 0) {
>     STRING_STAT_INCREMENT(Free);
>     free(this); // we were allocated with |malloc|
>   }
> }

Aaaah... Ok, I'm convinced, sorry for wasting peoples' time!

Comment 19

[Eric Chou [:ericchou] [:echou]]

(In reply to Chris Lord [:cwiiis] from comment #18)
> (In reply to Shawn Huang [:shawnjohnjr] from comment #17)
> > nsCString is not ref-counted, however nsStringBuffer is ref-counted
> > void
> > nsStringBuffer::Release()
> > {
> >   int32_t count = --mRefCount;
> >   NS_LOG_RELEASE(this, count, "nsStringBuffer");
> >   if (count == 0) {
> >     STRING_STAT_INCREMENT(Free);
> >     free(this); // we were allocated with |malloc|
> >   }
> > }
> 
> Aaaah... Ok, I'm convinced, sorry for wasting peoples' time!

No problem at all. Thanks for asking questions. :)

Comment 20

[Eric Chou [:ericchou] [:echou]]

b2g-inbound: https://hg.mozilla.org/integration/b2g-inbound/rev/f6b6547207ab

Comment 21

[Eric Chou [:ericchou] [:echou]]

Comment on attachment 8498754 [details] [diff] [review]
bug-1072021.patch

Approval Request Comment
[Feature/regressing bug #]: Not a regression.
[User impact if declined]: A2DP state could be wrong so users may be confused.
[Describe test coverage new/current, TBPL]: Manually / Try
[Risks and why]: Low. No logic change.
[String/UUID change made/needed]: No

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f6b6547207ab

Comment 23

[bhavana bajaj [:bajaj]]

(In reply to Eric Chou [:ericchou] [:echou] from comment #21)
> Comment on attachment 8498754 [details] [diff] [review]
> bug-1072021.patch
> 
> Approval Request Comment
> [Feature/regressing bug #]: Not a regression.
> [User impact if declined]: A2DP state could be wrong so users may be
> confused.
> [Describe test coverage new/current, TBPL]: Manually / Try
Can we add more automated tests here, maybe as a follow bug?
> [Risks and why]: Low. No logic change.
> [String/UUID change made/needed]: No

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2ea176a71d17

Comment 25

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

The possible way to test this gonk module is using the gtest framework, but gtest on b2g(bug 932562) isn't ready yet.

Comment 26

[Wayne Chen [:xwaynec]]

as comment 25, clear ni

Comment 27

[Seil Park]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #24)
> https://hg.mozilla.org/releases/mozilla-aurora/rev/2ea176a71d17

This issues occured from b2g-v2.0. It need the patch in v2.0

Comment 28

[Star Cheng [:scheng]]

(In reply to Seil Park from comment #27)
> (In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #24)
> > https://hg.mozilla.org/releases/mozilla-aurora/rev/2ea176a71d17
> 
> This issues occured from b2g-v2.0. It need the patch in v2.0


Hi, Seil

This issue is regression of bug 1037359 but the patch of bug 1037359 did *not* be landed in v2.0. So there is no symptom of this bug in V2.0.

Comment 29

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(In reply to Seil Park from comment #27)
> (In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #24)
> > https://hg.mozilla.org/releases/mozilla-aurora/rev/2ea176a71d17
> 
> This issues occured from b2g-v2.0. It need the patch in v2.0
> ===============================================================
> AudioManager::HandleBluetoothStatusChanged(nsISupports* aSubject,
>                                            const char* aTopic,
>                                            const nsCString aAddress)
> {
>   //.....
>   } else if (!strcmp(aTopic, BLUETOOTH_A2DP_STATUS_CHANGED_ID)) {
>     if (audioState == AUDIO_POLICY_DEVICE_STATE_UNAVAILABLE &&
> sA2dpSwitchDone) {
>       MessageLoop::current()->PostDelayedTask(
>         FROM_HERE, NewRunnableFunction(&ProcessDelayedA2dpRoute, audioState,
> aAddress.get()), 1000);
>       sA2dpSwitchDone = false;
> }
> 
> static void ProcessDelayedA2dpRoute(audio_policy_dev_state_t aState, const
> char *aAddress)
> {
>  // aAdress will be garbage.
> }
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/file/69ca61f7edf3/dom/system/gonk/AudioManager.cpp
I don't see ProcessDelayedA2dpRoute in v2.0,

Comment 30

[Seil Park]

(In reply to Shawn Huang [:shawnjohnjr] from comment #29)
> (In reply to Seil Park from comment #27)
> > (In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #24)
> > > https://hg.mozilla.org/releases/mozilla-aurora/rev/2ea176a71d17
> > 
> > This issues occured from b2g-v2.0. It need the patch in v2.0
> > ===============================================================
> > AudioManager::HandleBluetoothStatusChanged(nsISupports* aSubject,
> >                                            const char* aTopic,
> >                                            const nsCString aAddress)
> > {
> >   //.....
> >   } else if (!strcmp(aTopic, BLUETOOTH_A2DP_STATUS_CHANGED_ID)) {
> >     if (audioState == AUDIO_POLICY_DEVICE_STATE_UNAVAILABLE &&
> > sA2dpSwitchDone) {
> >       MessageLoop::current()->PostDelayedTask(
> >         FROM_HERE, NewRunnableFunction(&ProcessDelayedA2dpRoute, audioState,
> > aAddress.get()), 1000);
> >       sA2dpSwitchDone = false;
> > }
> > 
> > static void ProcessDelayedA2dpRoute(audio_policy_dev_state_t aState, const
> > char *aAddress)
> > {
> >  // aAdress will be garbage.
> > }
> https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/file/69ca61f7edf3/dom/
> system/gonk/AudioManager.cpp
> I don't see ProcessDelayedA2dpRoute in v2.0,

Thanks your comment. It seems someone landed in OEM source.

Comment 31

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(In reply to Seil Park from comment #30)
> (In reply to Shawn Huang [:shawnjohnjr] from comment #29)
> > (In reply to Seil Park from comment #27)
> > https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/file/69ca61f7edf3/dom/
> > system/gonk/AudioManager.cpp
> > I don't see ProcessDelayedA2dpRoute in v2.0,
> 
> Thanks your comment. It seems someone landed in OEM source.
Thanks for clarification.

Comment 32

[Tony Chung [:tchung]]

Alison, can you verifyme this bug is fixed and uplifted against 2.1?

Comment 33

[Alison Shiue]

Verified on:
[2.1]
Gaia-Rev        778ebac47554e1c4b7e9a952d73e850f58123914
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-aurora/rev/c4a4b04c617c
Build-ID        20141006000205
Version         34.0a2

I could not verify on 2.2 version since stuck at bug 1079127.

Comment 34

[Chris Lord [:cwiiis]]

I can verify on 2.2 with an Open C.

Comment 35

[Alison Shiue]

According to comment 33 and comment 34, change the status.