Closed Bug 1072021 Opened 10 years ago Closed 10 years ago

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

Categories

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

ARM
Gonk (Firefox OS)
defect

Tracking

(blocking-b2g:2.1+, firefox33 wontfix, firefox34 fixed, firefox35 fixed, b2g-v2.1 verified, b2g-v2.2 verified)

VERIFIED FIXED
2.1 S6 (10oct)
blocking-b2g 2.1+
Tracking Status
firefox33 --- wontfix
firefox34 --- fixed
firefox35 --- fixed
b2g-v2.1 --- verified
b2g-v2.2 --- verified

People

(Reporter: oedipus31, Assigned: xwaynec)

References

Details

(Whiteboard: [caf priority: p2][CR 729246][LibGLA,none,QE2, A])

Attachments

(1 file)

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
Severity: normal → critical
Component: General → GonkIntegration
OS: All → Gonk (Firefox OS)
Priority: -- → P1
Hardware: All → ARM
Whiteboard: [LibGLA,none,QE2, A]
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.
}
Severity: critical → normal
Component: GonkIntegration → Bluetooth
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.
Hi Shawn , Could you please kindly take a look at this and also kindly feedback the comment1 ? 
Thank you very much!!
Flags: needinfo?(shuang)
Assignee: nobody → shuang
Flags: needinfo?(shuang)
I will investigate AudioManager.
Assignee: shuang → scheng
[Blocking Requested - why for this release]:
Dup of a 2.1+ bug 1070919.
blocking-b2g: --- → 2.1?
triage: Bug 1070919 is 2.1+
blocking-b2g: 2.1? → 2.1+
Attachment #8498754 - Flags: review?(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.
Attachment #8498754 - Flags: review?(echou) → review+
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.
Attachment #8498754 - Flags: review+ → review?(echou)
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).
Attachment #8498754 - Flags: review?(echou) → review+
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.
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.
Flags: needinfo?(chrislord.net)
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.
Flags: needinfo?(chrislord.net)
Whiteboard: [LibGLA,none,QE2, A] → [CR 729246][LibGLA,none,QE2, A]
Whiteboard: [CR 729246][LibGLA,none,QE2, A] → [caf priority: p2][CR 729246][LibGLA,none,QE2, A]
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|
  }
}
(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!
(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. :)
Assignee: scheng → waychen
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
Attachment #8498754 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/f6b6547207ab
Status: UNCONFIRMED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S6 (10oct)
(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
Flags: needinfo?(waychen)
Attachment #8498754 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
The possible way to test this gonk module is using the gtest framework, but gtest on b2g(bug 932562) isn't ready yet.
as comment 25, clear ni
Flags: needinfo?(waychen)
(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
Flags: needinfo?(ryanvm)
Flags: needinfo?(ryanvm) → needinfo?(waychen)
(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.
(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,
(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.
(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.
Alison, can you verifyme this bug is fixed and uplifted against 2.1?
Flags: needinfo?(ashiue)
Keywords: verifyme
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.
Flags: needinfo?(ashiue)
I can verify on 2.2 with an Open C.
According to comment 33 and comment 34, change the status.
Status: RESOLVED → VERIFIED
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.