Closed Bug 1129846 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox OS Graveyard :: Bluetooth, defect)

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

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

RESOLVED FIXED
2.2 S6 (20feb)
blocking-b2g 2.2+
Tracking Status
firefox36 --- wontfix
firefox37 --- wontfix
firefox38 --- fixed
b2g-v2.2 --- fixed
b2g-master --- fixed

People

(Reporter: brsun, Assigned: brsun)

References

Details

Attachments

(1 file, 2 obsolete files)

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).
Depends on: 1129257
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.
blocking-b2g: --- → 2.2?
Assignee: nobody → brsun
Triage:blocking
blocking-b2g: 2.2? → 2.2+
(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.
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+
(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
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.
TPBL results seem good.
Keywords: checkin-needed
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: 6 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.