Closed Bug 1065999 Opened 5 years ago Closed 5 years ago

[Bluetooth] CAF sends unknown property BT_PROPERTY_REMOTE_TRUST_VALUE

Categories

(Firefox OS Graveyard :: Bluetooth, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-b2g:2.1+, firefox33 wontfix, firefox34 fixed, firefox35 fixed, b2g-v1.4 wontfix, b2g-v2.0 wontfix, b2g-v2.0M wontfix, 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-v1.4 --- wontfix
b2g-v2.0 --- wontfix
b2g-v2.0M --- wontfix
b2g-v2.1 --- verified
b2g-v2.2 --- verified

People

(Reporter: tzimmermann, Assigned: tzimmermann)

References

Details

(Keywords: regression, smoketest, Whiteboard: [caf priority: p2][POVB][CR 716068])

Attachments

(2 files, 1 obsolete file)

We currently don't support CAF's extensions to BT HAL, but we still receive BT_PROPERTY_REMOTE_TRUST_VALUE from CAF's BT implementation. CAF's BT should not send this property.
Michael,

we can't handle BT_PROPERTY_REMOTE_TRUST_VALUE coming from CAF BT.
Flags: needinfo?(mvines)
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #1)
> Michael,
> 
> we can't handle BT_PROPERTY_REMOTE_TRUST_VALUE coming from CAF BT.

Let me check this further
Flags: needinfo?(mvines) → needinfo?(bhargavg1)
Why can't you ignore it?
(In reply to Michael Vines [:m1] [:evilmachines] from comment #3)
> Why can't you ignore it?

We have error checks to see if a received value is known to us. When we receive BT_PROPERTY_REMOTE_TRUST_VALUE, the respective check fails. But we can't easily add a workaround for this specific case, because the enum constant doesn't exist on non-CAF branches; and our code would fail to compile there.
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #4)
>
> We have error checks to see if a received value is known to us. When we
> receive BT_PROPERTY_REMOTE_TRUST_VALUE, the respective check fails.

When the check fails you can't simply bail out and ignore the property?
That would mean that we'd effectively remove the check.
Please provide much more detail here on the exact issue.  Code references would help too.  Also what branches are affected beyond master.  You are making it hard for us to help you.
Hi,

The STR is roughly

  - call getPairedDevices in Gaia
  - observe the error message in BluetoothNotificationHALRunnable4::Create in logcat

Shawn, can you give a better STR?

In Gecko, we convert Bluedroid constants to internal constants. The code for converting bt_property_type_t is at [1]. You'll find an array for mapping Bluedroid constants to internal constants. This function fails because the array doesn't contain BT_PROPERTY_REMOTE_TRUST_VALUE.

We cannot add BT_PROPERTY_REMOTE_TRUST_VALUE to the array, because it doesn't exist in non-CAF branches; which would consequently fail to build. Ignoring the value would mean removing our error checks.

Affected branches are v2.1 and master.

[1] http://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/bluedroid/BluetoothHALHelpers.h#184
This bug caused by CAF sending non-AOSP bt_property_t BT_PROPERTY_REMOTE_TRUST_VALUE property, when we verified the input value, we found that value doesn't exist, thus return NS_ERROR_ILLEGAL_VALUE.

see Branch v2.1 codebase: http://hg.mozilla.org/releases/mozilla-aurora/file/48c37c8442cb/dom/bluetooth/bluedroid/BluetoothInterface.cpp#l342

It seems correct to me to validate bt_property_type_t aIn value here. The basic idea of |Convert| is to hide the type of Android HAL from GeckoBluetooth.

   341   if (!aIn || aIn >= MOZ_ARRAY_LENGTH(sPropertyType)) {
   342     return NS_ERROR_ILLEGAL_VALUE;
   343   }

Comment 6 mentioned we'd effectively remove the validation here.

STR:
1. Pair with one device
2. After pairing complete, turn off bt and turn on again.
3. Settings will call getPairedDevices, and it fails because validation fails
Thanks, can you also explain why v2.0 does not have this problem?
> Comment 6 mentioned we'd effectively remove the validation here.

Sorry, also instead of returning illegal value if the validation fails why not return "unknown value" and just skip the property entirely.  Ie, "I have no idea what this is so I'm going to just ignore it".  Emit a log message too but try your best to keep going as well.  Does that cause issues later on in Gecko?
Duplicate of this bug: 1066235
Flags: needinfo?(bhargavg1)
Whiteboard: [POVB][CR 716068]
An answer to comment 10 would be nice.  We can make this change for v2.1 however there will be devices with a v2.0 Gonk that encounter this footgun once their owner shallow flashes a >=v2.1 Gecko/Gaia on them.  Seems like v2.1 Gecko needs to be made more resilient regardless of whether we remove this property on the CAF side for v2.1.
Flags: needinfo?(tzimmermann)
Whiteboard: [POVB][CR 716068] → [caf priority: p2][POVB][CR 716068]
Hi Michael,

(In reply to Michael Vines [:m1] [:evilmachines] from comment #13)
> An answer to comment 10 would be nice.  We can make this change for v2.1
> however there will be devices with a v2.0 Gonk that encounter this footgun
> once their owner shallow flashes a >=v2.1 Gecko/Gaia on them.  Seems like
> v2.1 Gecko needs to be made more resilient regardless of whether we remove
> this property on the CAF side for v2.1.

Thomas is on PTO this week. I have talked with other bluetooth team members about this issue this afternoon. Here are my thoughts.

My answer to comment 10 is: for 2.0, we ignore the unknown property, so it doesn't have the problem.

To tell the truth, I don't have a strong reason for 'not ignoring the value that we don't care'. On the other hand, however, I understand Thomas' principle in this case. Therefore, we have decided not to provide a 'removing check' patch without Thomas' opinion. I would like to wait for him coming back from PTO and we'll have a quick discussion about how to get this resolved.
Blocks: 1058031
Sorry for the delay. I've been on PTO last week.

There's a standard API for Bluedroid, which is what we're using in Gecko. Our error checking is there to ensure that we're using the API correctly.

The only incompatible changes we've found so far are the ones related to CAF extensions. But it's not a black box and there's already code to handle incompatibilities. So I don't understand the point of working around the issue in Gecko if we can actually fix it.
Flags: needinfo?(tzimmermann)
Change to be landing at, https://www.codeaurora.org/cgit/quic/la/platform/external/bluetooth/bluedroid/log/?h=LNX.LF.3.5.1_RB1.2
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Cool, thanks!
FTR I believe v2.1 Gecko should still protect itself against surprises like this.  This new code is being unnecessary myopic and v1.4/v2.0 didn't mind.

Background reading: http://en.wikipedia.org/wiki/Robustness_principle
Resolution: FIXED → INCOMPLETE
Hi

(In reply to Michael Vines [:m1] [:evilmachines] from comment #18)
> FTR I believe v2.1 Gecko should still protect itself against surprises like
> this.  This new code is being unnecessary myopic and v1.4/v2.0 didn't mind.

This fix is not used by v2.1? It's probably OK if it's in master and we have to apply a workaround to v2.1 Gecko. And once the Bluetooth daemon is ready, it might become easier to handle such incompatibilities in general.

> Background reading: http://en.wikipedia.org/wiki/Robustness_principle

I wouldn't call it 'background'. ;) And to cite from WP:

>> but code that receives input should accept non-conformant input as long as the meaning is clear

I think that's the key point: the meaning has to be clear. I would argue that for unknown properties, the meaning is not clear, so the command should be rejected.

IMHO, the Robustness Principle mostly applies to syntax and formatting, but not so much to semantics. I found

  http://aosabook.org/en/sendmail.html

about the design of Sendmail to be an interesting read. In Section 17.4.6, the author writes about his attempts to interpret malformed messages. It made sense in the early days of the Internet, but turned out to be problematic later on.
Resolution: INCOMPLETE → FIXED
Hey again,

I just want to say that comment 19 is not meant offensive in any way and I don't want to start a flame war. I just wanted to give an explanation on why I think the problem should be handled in the CAF source code, and why the Robustness Principle is IMHO not a good fit here.
This problem still exists in v2.1, right? Don't know why the status was reset in comment 19.
Resolution: FIXED → INCOMPLETE
This is the fix for v2.1.
Assignee: nobody → tzimmermann
Status: RESOLVED → REOPENED
Attachment #8496752 - Flags: review?(shuang)
Resolution: INCOMPLETE → ---
Changes since v1:

  - fixed another affected branch in |Convert(bt_property_t, BluetoothProperty)|
Attachment #8496752 - Attachment is obsolete: true
Attachment #8496801 - Flags: review?(shuang)
[Blocking Requested - why for this release]:

Gecko's Bluetooth implementation has stricter error checks in v2.1 than in older versions. This breaks support for CAF's Bluedroid implementation. CAF sends an unknown token to Gecko, which will abort the current operation.

For master the problem has already been fixed on CAF side, for v2.1 we need to land a fix in Gecko. See attachment [01] for this patch.
blocking-b2g: --- → 2.1?
Triage: blocking.
blocking-b2g: 2.1? → 2.1+
Hi Thomas,

The landing rule has been modified and now we need both [2.1+] and [approval-aurora] to get our patch land on aurora. Could you request for approval?
Flags: needinfo?(tzimmermann)
Comment on attachment 8496801 [details] [diff] [review]
[01][v2.1] Bug 1065999: Ignore unknown Bluetooth properties (v2)

Approval Request Comment
[Feature/regressing bug #]:

Bug 1065999

[User impact if declined]:

User won't always be able to use remote Bluetooth devices.

[Describe test coverage new/current, TBPL]:

Manually tested.

[Risks and why]: 

Low. The problem is that Gecko receives an unknown value from the Bluetooth driver. The patch makes it ignore that value, which is the same behavior as in pre-v2.1 releases.

[String/UUID change made/needed]:

None
Attachment #8496801 - Flags: approval-mozilla-aurora?
Flags: needinfo?(tzimmermann)
Attachment #8496801 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/2010c1973ded
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S6 (10oct)
(In reply to bhargavg1 from comment #16)
> Change to be landing at,
> https://www.codeaurora.org/cgit/quic/la/platform/external/bluetooth/
> bluedroid/log/?h=LNX.LF.3.5.1_RB1.2

This branch is LNX.LF.3.5.1_RB1.2 (somehow it looks like Android release). I don't think flame configure uses this branch. So our central still suffers this issue.

<project name="platform/external/bluetooth/bluedroid" path="external/bluetooth/bluedroid" revision="b2g_kk_3.5"/>

I checked the branch LNX.LF.3.5.1_RB1.2 it seems like there are a few patch related b2g specific is missing. Now I'm very confused here which branch we should use.

Or can we commit the same gecko patch into central?
Flipping v2.2 status to affected with results found in bug 1077173.  Also this fails our smoke tests.
Hi

(In reply to Shawn Huang [:shawnjohnjr] from comment #30)
> (In reply to bhargavg1 from comment #16)
> > Change to be landing at,
> > https://www.codeaurora.org/cgit/quic/la/platform/external/bluetooth/
> > bluedroid/log/?h=LNX.LF.3.5.1_RB1.2
> 
> This branch is LNX.LF.3.5.1_RB1.2 (somehow it looks like Android release). I
> don't think flame configure uses this branch. So our central still suffers
> this issue.
> 
> <project name="platform/external/bluetooth/bluedroid"
> path="external/bluetooth/bluedroid" revision="b2g_kk_3.5"/>
> 
> I checked the branch LNX.LF.3.5.1_RB1.2 it seems like there are a few patch
> related b2g specific is missing. Now I'm very confused here which branch we
> should use.

What's status here? According to comment 16 this bug was fixed upstream. Can you point to the patch?
Flags: needinfo?(tzimmermann) → needinfo?(bhargavg1)
(In reply to Shawn Huang [:shawnjohnjr] from comment #30)
> (In reply to bhargavg1 from comment #16)
> > Change to be landing at,
> > https://www.codeaurora.org/cgit/quic/la/platform/external/bluetooth/
> > bluedroid/log/?h=LNX.LF.3.5.1_RB1.2
> 
> This branch is LNX.LF.3.5.1_RB1.2 (somehow it looks like Android release). I
> don't think flame configure uses this branch. So our central still suffers
> this issue.
> 
> <project name="platform/external/bluetooth/bluedroid"
> path="external/bluetooth/bluedroid" revision="b2g_kk_3.5"/>
> 
> I checked the branch LNX.LF.3.5.1_RB1.2 it seems like there are a few patch
> related b2g specific is missing. Now I'm very confused here which branch we
> should use.
> 
> Or can we commit the same gecko patch into central?

Shawn, b2g_kk_3.5 is for v2.0 + Flame (hardware) configuration. 

We dont have anything for v2.2 yet. If you are looking for the change you need to maintain that internally. the changes would be, 

https://www.codeaurora.org/cgit/external/gigabyte/platform/external/bluetooth/bluedroid/commit/?h=caf/LNX.LF.3.5.1_RB1.2&id=cb3ae4fa3a76d9299686cb37a2597fb24ed0e8c4
https://www.codeaurora.org/cgit/external/gigabyte/platform/hardware/libhardware/commit/?h=caf/LNX.LF.3.5.1_RB1.2&id=fb43a5da9b31917df7bbb27ffcbba4d2174e3070
Flags: needinfo?(bhargavg1)
(In reply to bhargavg1 from comment #35)
> (In reply to Shawn Huang [:shawnjohnjr] from comment #30)
> > (In reply to bhargavg1 from comment #16)
> > > Change to be landing at,
> > > https://www.codeaurora.org/cgit/quic/la/platform/external/bluetooth/
> > > bluedroid/log/?h=LNX.LF.3.5.1_RB1.2
> > 
> > This branch is LNX.LF.3.5.1_RB1.2 (somehow it looks like Android release). I
> > don't think flame configure uses this branch. So our central still suffers
> > this issue.
> > 
> > <project name="platform/external/bluetooth/bluedroid"
> > path="external/bluetooth/bluedroid" revision="b2g_kk_3.5"/>
> > 
> > I checked the branch LNX.LF.3.5.1_RB1.2 it seems like there are a few patch
> > related b2g specific is missing. Now I'm very confused here which branch we
> > should use.
> > 
> > Or can we commit the same gecko patch into central?
> 
> Shawn, b2g_kk_3.5 is for v2.0 + Flame (hardware) configuration. 
Hi Micahel,
It seems CAF uses LNX.LF.3.5.1_RB1.2 branch for v2.1. However, on flame we always use b2g_kk_3.5 as the default bluedroid branch. There is no difference between v2.0 and v2.1 for gecko. Shall we switch branch to LNX.LF.3.5.1_RB1.2. It makes sense to me to use the latest bluedroid stack branch.
Flags: needinfo?(mwu)
LNX.LF.3.5.1_RB1.2 is not newer than b2g_kk_3.5, I recommend not switching to that branch for Flame.

Your options seem to be:
1) Fix v2.1/v2.2 Gecko to behave like v1.4/v2.0 Gecko and once again ignore this property.
2) Fork b2g_kk_3.5 and cherry-pick the commit from LNX.LF.3.5.1_RB1.2
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment on attachment 8500991 [details] [diff] [review]
[01] Bug 1065999: Ignore unknown Bluetooth properties

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

Looks good to me. Special thanks for this workaround.
Attachment #8500991 - Flags: review?(shuang) → review+
https://hg.mozilla.org/mozilla-central/rev/0c07298d0b58
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Verified fixed on Flame 2.1 and 2.2.
Using the STR found in bug 1077105, I was able to successfully pair with bluetooth devices, maintain those pairings, and transfer files to and from the Flame device via those pairings.

Environmental Variables:
Device: Flame 2.2 Master
BuildID: 20141008040203
Gaia: 0bc74ce502672cf0265b24cf3a25d117c3de5e71
Gecko: e4cfacb76830
Gonk: 52c909e821d107d414f851e267dedcd7aae2cebf
Version: 35.0a1 (2.2 Master)
Firmware: v180
User Agent: Mozilla/5.0 (Mobile; rv:35.0) Gecko/35.0 Firefox/35.0

Environmental Variables:
Device: Flame 2.1
BuildID: 20141008000201
Gaia: d71f8804d7229f4b354259d5d8543c25b4796064
Gecko: 7fa82c9acdf2
Gonk: 52c909e821d107d414f851e267dedcd7aae2cebf
Version: 34.0a2 (2.1)
Firmware: v180
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0
Status: RESOLVED → VERIFIED
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(pbylenga)
Duplicate of this bug: 1077105
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage?+]
Flags: needinfo?(pbylenga)
You need to log in before you can comment on or make changes to this bug.