[Bluetooth][Daemon] Only handle explicitly supported data in BluetoothDaemonPDU.

RESOLVED FIXED in Firefox 38

Status

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: brsun, Assigned: brsun)

Tracking

unspecified
2.2 S6 (20feb)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:2.2+, firefox36 wontfix, firefox37 wontfix, firefox38 fixed, b2g-v2.2 fixed, b2g-master fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Assignee

Description

4 years ago
This bug is initiated from bug 1124556, which can be solved (duplicated?) by bug 1129257.

Generally speaking, PDU should handle each data type explicitly while doing packing and unpacking. It tends to introduce problems if data types are allowed to be converted with each other automatically (bug 1124556 comment 7).
Assignee

Updated

4 years ago
Depends on: 1129257
Assignee

Comment 1

4 years ago
Generally speaking this patch is the same as attachment 8559642 [details] [diff] [review] on bug 1124556. But attachment 8559642 [details] [diff] [review] is based on attachment 8557843 [details] [diff] [review]. This attachment is based on attachment 8559015 [details] [diff] [review] of bug 1129257.
Attachment #8559722 - Flags: review?(tzimmermann)
Comment on attachment 8559722 [details] [diff] [review]
bug1129846_handle_pdu_type_explicitly.patch

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

I'd generally r+ this patch, but can you check the question first? Thanks.

::: dom/bluetooth/bluedroid/BluetoothDaemonHelpers.h
@@ +931,5 @@
>  }
>  
> +template<>
> +inline nsresult
> +UnpackPDU<uint8_t>(BluetoothDaemonPDU& aPDU, UnpackArray<uint8_t>& aOut)

Can you figure out where this is required? The 'const' variant of the function should have been able to handle the case.
Assignee

Updated

4 years ago
blocking-b2g: --- → 2.2?
Assignee

Updated

4 years ago
Assignee: nobody → brsun

Comment 3

4 years ago
Triage:blocking
blocking-b2g: 2.2? → 2.2+
Assignee

Comment 4

4 years ago
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #2)
> Comment on attachment 8559722 [details] [diff] [review]
> bug1129846_handle_pdu_type_explicitly.patch
> 
> Review of attachment 8559722 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'd generally r+ this patch, but can you check the question first? Thanks.
> 
> ::: dom/bluetooth/bluedroid/BluetoothDaemonHelpers.h
> @@ +931,5 @@
> >  }
> >  
> > +template<>
> > +inline nsresult
> > +UnpackPDU<uint8_t>(BluetoothDaemonPDU& aPDU, UnpackArray<uint8_t>& aOut)
> 
> Can you figure out where this is required? The 'const' variant of the
> function should have been able to handle the case.

This function is not needed indeed. I'll remove it and update the patch again. Thanks for catching this.
Assignee

Comment 5

4 years ago
Attachment #8559722 - Attachment is obsolete: true
Attachment #8559722 - Flags: review?(tzimmermann)
Attachment #8560232 - Flags: review?(tzimmermann)
Comment on attachment 8560232 [details] [diff] [review]
bug1129846_handle_pdu_type_explicitly.v2.patch

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

r+ with a remark.

::: dom/bluetooth/bluedroid/BluetoothDaemonHelpers.h
@@ +910,5 @@
>  }
>  
> +template<typename T>
> +inline nsresult
> +UnpackPDU(BluetoothDaemonPDU& aPDU, UnpackArray<T>& aOut)

If the other function wasn't required, this one might not be needed as well. Did you really test if you need these functions?
Attachment #8560232 - Flags: review?(tzimmermann) → review+
Assignee

Comment 7

4 years ago
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #6)
> Comment on attachment 8560232 [details] [diff] [review]
> bug1129846_handle_pdu_type_explicitly.v2.patch
> 
> Review of attachment 8560232 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ with a remark.
> 
> ::: dom/bluetooth/bluedroid/BluetoothDaemonHelpers.h
> @@ +910,5 @@
> >  }
> >  
> > +template<typename T>
> > +inline nsresult
> > +UnpackPDU(BluetoothDaemonPDU& aPDU, UnpackArray<T>& aOut)
> 
> If the other function wasn't required, this one might not be needed as well.
> Did you really test if you need these functions?

Yes, this function is needed. Otherwise the following lines will suffer from link errors:
 - https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/bluedroid/BluetoothDaemonInterface.cpp#979
 - https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/bluedroid/BluetoothDaemonInterface.cpp#1033
 - https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/bluedroid/BluetoothDaemonInterface.cpp#1073
Assignee

Comment 9

4 years ago
Probably changing the usage of |UnpackArray<BluetoothProperty>| to be the same as |UnpackArray<uint8_t>| [1] could save this function. But I haven't tried this approach yet.

[1] https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/bluedroid/BluetoothDaemonInterface.cpp#1319
That's OK. I was just curious because functions with const parameters can often handle all cases.
Assignee

Comment 12

4 years ago
TPBL results seem good.
Keywords: checkin-needed
Assignee

Comment 13

4 years ago
Comment on attachment 8560352 [details] [diff] [review]
bug1129846_handle_pdu_type_explicitly.commit.patch

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: Some potential problems which are caused by implicit data-type conversion are hard to be found during programming.
Testing completed: Manual testing locally.
Risk to taking this patch (and alternatives if risky): Low. This patch doesn't modify any logics for packing / unpacking PDU data. This patch enables some error checkings during linktime instead
String or UUID changes made by this patch: N/A
Attachment #8560352 - Flags: approval-mozilla-b2g37?
waiting for m-c landing before considering branch uplift.
https://hg.mozilla.org/mozilla-central/rev/8599349898e7
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.2 S6 (20feb)
Attachment #8560352 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
You need to log in before you can comment on or make changes to this bug.