Bluetooth pairing dialog should not display when trying to transfer file through NFC

VERIFIED FIXED

Status

Firefox OS
Bluetooth
VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: gduan, Assigned: brsun)

Tracking

unspecified
ARM
Gonk (Firefox OS)

Firefox Tracking Flags

(blocking-b2g:2.2+, b2g-v2.2 verified, b2g-master verified)

Details

Attachments

(2 attachments, 1 obsolete attachment)

Build id: 20150121160202
Gaia: 917b6c36717fddc6e71ffc1ec249633c8044c93c
gecko: f92b976370502c2a6cf91a38ede0fa7e1459e57a

STR: 
1. enable two devices' nfc 
2. launch gallery of device A and tap one image
3. put two device very close and image will tilt down
4. swipe the image up , file should start to transfer

Expect:
  file transferring done
Actual:
  bluetooth pairing dialog pop out
ni? for Shawn

Alison said this problem happends on master, but not on v2.2
Flags: needinfo?(shuang)
Component: NFC → Bluetooth

Comment 2

3 years ago
[Blocking Requested - why for this release]:
Obvious problem
blocking-b2g: --- → 3.0?
QA Whiteboard: [COM=NFC]
status-b2g-v2.2: --- → unaffected
status-b2g-master: --- → affected
Bruce,
I'm not sure it's related to daemon, but currently v2.2 uplift process has not been completed.
Can you take a look?
Assignee: nobody → brsun
Flags: needinfo?(shuang)

Comment 4

3 years ago
Hi Bruce, please help to verify the problem, thanks
Flags: needinfo?(brsun)
(Assignee)

Comment 5

3 years ago
Originally I suffered from "File could not be sent" message instead of the scenario describes on this bug. Thanks for :allstars.chh's help, it seems the patch on bug 1127726 can resolve my problem.

Now I can reproduce this issue and continue to investigate this bug now.
Flags: needinfo?(brsun)
(Assignee)

Comment 6

3 years ago
This issue is not able to be reproduced if |ro.moz.bluetooth.backend| is set to |bluedroid|.
(Assignee)

Comment 7

3 years ago
Created attachment 8557843 [details] [diff] [review]
bug1124556_unhandled_packpdu_type.patch

In this bug, |aChannel| can be converted from |int(0xffffffff)| to
|int16_t(0xffff)| without problem by using |PackConversion()| in
|BluetoothDaemonSocketModule::ConnectCmd()|. But since we lack a proper
version of |PackPDU()| to serialize |int16_t| into |BluetoothDaemonPDU|, the
overloaded version of |PackPDU()| with |int32_t| is chosen by Gecko
automatically. As a result, two unexpected bytes with value 0xffff are
inserted before |SocketFlags()|, and |read_pdu_at()| in |opcode_connect()| at
bluetoothd side treats 0xff as |flags|. Since |BTSOCK_FLAG_ENCRYPT| bit and
|BTSOCK_FLAG_AUTH| bit are set in |flags|, the authentication process will be
triggered.
Attachment #8557843 - Flags: review?(tzimmermann)
Comment on attachment 8557843 [details] [diff] [review]
bug1124556_unhandled_packpdu_type.patch

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

Thanks for finding and fixing this bug.

Everything in this patch that is not about int16_t is for future problems of this kind? I guess that makes sense. But please remove the 64-bit function. IIRC there are no places in the protocol where they are required. r+ with nit.
Attachment #8557843 - Flags: review?(tzimmermann) → review+
(Assignee)

Comment 9

3 years ago
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #8)
> Comment on attachment 8557843 [details] [diff] [review]
> bug1124556_unhandled_packpdu_type.patch
> 
> Review of attachment 8557843 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for finding and fixing this bug.
> 
> Everything in this patch that is not about int16_t is for future problems of
> this kind? I guess that makes sense. But please remove the 64-bit function.

I add all integer primitive types to avoid similar problems. All these types are common to be used in general cases, but it is also very easy to cause serious problems if some types are implicitly converted with each other.

Will you suggest to let these overloaded functions explicitly return errors while dealing with 64-bit types? I think it is not a harm to have these functions for all primitive types on PDU. PDU should do serialization / deserialization correctly no matter how the protocol changes or what the protocol is used. It would be good to have the same capabilities in gecko side as those in bluetoothd side [1][2].

What do you think?

[1] https://github.com/mozilla-b2g/platform_system_bluetoothd/blob/master/src/bt-proto.c#L73
[2] https://github.com/mozilla-b2g/platform_system_bluetoothd/blob/master/src/bt-proto.c#L228
Flags: needinfo?(tzimmermann)
(Assignee)

Comment 10

3 years ago
We should fix this issue in v2.2.
blocking-b2g: 3.0? → 2.2?
(Assignee)

Comment 11

3 years ago
(In reply to Bruce Sun [:brsun] from comment #10)
> We should fix this issue in v2.2.

and master as well.
OS: Mac OS X → Gonk (Firefox OS)
Hardware: x86 → ARM
(Assignee)

Comment 12

3 years ago
Created attachment 8558299 [details] [review]
https://github.com/mozilla-b2g/platform_system_bluetoothd/pull/17

There is one nit needed to be fixed while unpacking |channel| in bluetoothd as well.
Attachment #8558299 - Flags: review?(tzimmermann)

Comment 13

3 years ago
blocking for 2.2+
blocking-b2g: 2.2? → 2.2+
Hi

> Will you suggest to let these overloaded functions explicitly return errors
> while dealing with 64-bit types? 

I meant to remove them completely, because they are not required by the HAL protocol. If you think they make sense, then just leave them in as in the original patch. Won't hurt at least.
Flags: needinfo?(tzimmermann)
Comment on attachment 8557843 [details] [diff] [review]
bug1124556_unhandled_packpdu_type.patch

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

Hi Bruce,

It looks like the BlueZ docs are incorrect. I just checked what BlueZ does and they are packing the channel in an 'int32_t'. See [1] for the structure. We should do the same, so I'm r-'ing the patch.

[1] https://git.kernel.org/cgit/bluetooth/bluez.git/tree/android/hal-msg.h#n290
Attachment #8557843 - Flags: review+ → review-
Attachment #8558299 - Flags: review?(tzimmermann)
Please see comment 15.
Flags: needinfo?(brsun)
I sent a patch [1] to BlueZ upstream. Let's see what they say.

[1] https://marc.info/?l=linux-bluetooth&m=142295339503242&w=2
The documentation patch has been applied upstream. [1] We should fix our code accordingly.

[1] https://marc.info/?l=linux-bluetooth&m=142296333606260&w=2

Updated

3 years ago
status-b2g-v2.2: unaffected → affected
(Assignee)

Comment 19

3 years ago
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #16)
> Please see comment 15.

Hi Thomas,

Thanks for catching this issue. It seems the newly found issue is another bug to me if correcting the data type of |channel| is not just a nit anymore. I've created bug 1129257 for this. For me this bug is more related to the completeness of serialization / deserialization, and that newly created bug is more related to the correctness of protocol implementation.

How about completing the serialization / deserialization function on this bug first?

By the way, |opcode_listen| in |bt-sock-io.c| of bluetoothd might also suffers from the same issue[1] as well.

[1] https://git.kernel.org/cgit/bluetooth/bluez.git/tree/android/hal-msg.h#n281
Flags: needinfo?(brsun) → needinfo?(tzimmermann)
(Assignee)

Updated

3 years ago
Attachment #8558299 - Attachment is obsolete: true
> By the way, |opcode_listen| in |bt-sock-io.c| of bluetoothd might also
> suffers from the same issue[1] as well.

They are both fixed in upstream documentation.
Flags: needinfo?(tzimmermann)
(Assignee)

Comment 22

3 years ago
Hi Thomas, those concerns on comment 15 have been addressed on bug 1129257. Would you mind reconsidering the review flag of attachment 8557843 [details] [diff] [review] again?
Flags: needinfo?(tzimmermann)
Hi,

I though the other bug would already fix the issue?
Flags: needinfo?(tzimmermann)
(Assignee)

Comment 24

3 years ago
Bug 1129257 do fix this problematic scenario, but it fixes the problem just by coincident.

We still have to complete serialization / deserialization functions to make sure PDU works well under common primitive types. Letting our codes have possibilities to do implicit type-conversion might not a good idea while dealing with PDU.
We'd only add lot's of unused functions to the code base to catch up with the implicit type conversion. I think we need a better approach to prevent this problem in the first place.

Let's add wrapper structures around primitive types instead. For unpacking this would look like

  template <typename T>
  struct UnpackType<T> MOZ_FINAL {
    UnpackType(T& aValue)
      : mValue(&aValue)

  private:
    T* mValue;
  };

Unpacking and 'int32_t' would look like

  int32_t value;

  Unpack<UnpackType<int32_t> >(pdu, UnpackType<int32_t>(value));

That is a bit longer but prevents the compiler from making implicit type conversions because the basic type is wrapped in a structure. Using this pattern throughout the code should reduce the problem significantly. Similar code can be added for packing PDUs. Conversion functions might have to be adapted as well.
(Assignee)

Comment 26

3 years ago
Created attachment 8559642 [details] [diff] [review]
bug1124556_handle_pdu_used_type_explicitly.patch

By declaring but not implementing |template <typename T> nsresult PackPDU(T aIn, BluetoothDaemonPDU& aPDU)| and |template <typename T> nsresult UnpackPDU(BluetoothDaemonPDU& aPDU, T& aOut)|, we can notice non-handled data type during link stage.
Attachment #8559642 - Flags: review?(tzimmermann)
(Assignee)

Comment 27

3 years ago
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #25)
> We'd only add lot's of unused functions to the code base to catch up with
> the implicit type conversion.

Hi Thomas,

Unused functions for primitive integer types should not be a problem for |BluetoothDaemonPDU| by design. Just like there are many unused cases in |read_pdu_at_va()| and |write_pdu_at_va()| in bluetoothd. It might not a good idea to remove those switch cases from bluetoothd just because we don't have real use cases currently. It would be a part of completeness of PDU, not a redundancy of PDU. Will you reconsider the benefit of attachment 8557843 [details] [diff] [review]?
Flags: needinfo?(tzimmermann)
Hi

> Unused functions for primitive integer types should not be a problem for
> |BluetoothDaemonPDU| by design. Just like there are many unused cases in
> |read_pdu_at_va()| and |write_pdu_at_va()| in bluetoothd. It might not a
> good idea to remove those switch cases from bluetoothd just because we don't
> have real use cases currently. It would be a part of completeness of PDU,
> not a redundancy of PDU. Will you reconsider the benefit of attachment
> 8557843 [details] [diff] [review]?

My critic of this approach is that it doesn't solve the problem. It just adds code _in the hope_ to catch some of the errors. That it results in lot's of unused code is rather an unpleasant side effect.
Flags: needinfo?(tzimmermann)
Comment on attachment 8559642 [details] [diff] [review]
bug1124556_handle_pdu_used_type_explicitly.patch

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

This is a very good idea. Thanks for bringing this up. A template for UnpackPDU should be added as well. I'd say this should rather go into a separate bug report. It still seems that the other patch fixed the NFC problem already, and this patch is completely unrelated.
Some other idea: It just occurred to me that we can probably also improve by passing basic types as constant reference instead of values. The compiler shouldn't be able to do the type conversion then.
(Assignee)

Comment 31

3 years ago
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #28)
> Hi
> 
> > Unused functions for primitive integer types should not be a problem for
> > |BluetoothDaemonPDU| by design. Just like there are many unused cases in
> > |read_pdu_at_va()| and |write_pdu_at_va()| in bluetoothd. It might not a
> > good idea to remove those switch cases from bluetoothd just because we don't
> > have real use cases currently. It would be a part of completeness of PDU,
> > not a redundancy of PDU. Will you reconsider the benefit of attachment
> > 8557843 [details] [diff] [review]?
> 
> My critic of this approach is that it doesn't solve the problem. It just
> adds code _in the hope_ to catch some of the errors. That it results in
> lot's of unused code is rather an unpleasant side effect.

Got your point. Bug 1129846 is created to handle data types of PDU in a better way.
(Assignee)

Comment 32

3 years ago
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #29)
> Comment on attachment 8559642 [details] [diff] [review]
> bug1124556_handle_pdu_used_type_explicitly.patch
> 
> Review of attachment 8559642 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is a very good idea. Thanks for bringing this up. A template for
> UnpackPDU should be added as well. I'd say this should rather go into a
> separate bug report. It still seems that the other patch fixed the NFC
> problem already, and this patch is completely unrelated.

|UnpackPDU()| is added in this patch already. I'll attach this patch onto another for review again.
Oh, I missed that. Sorry.
(Assignee)

Comment 34

3 years ago
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #30)
> Some other idea: It just occurred to me that we can probably also improve by
> passing basic types as constant reference instead of values. The compiler
> shouldn't be able to do the type conversion then.

This is a solution as well from coding conventions' point of view. Ideally it can work perfectly, but it lacks enforcement. Rules would be violated if programmers don't follow this guideline carefully.
(In reply to Bruce Sun [:brsun] from comment #34)
> (In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #30)
> > Some other idea: It just occurred to me that we can probably also improve by
> > passing basic types as constant reference instead of values. The compiler
> > shouldn't be able to do the type conversion then.
> 
> This is a solution as well from coding conventions' point of view. Ideally
> it can work perfectly, but it lacks enforcement. Rules would be violated if
> programmers don't follow this guideline carefully.

Well, good point. I'd say that it would still be easier to enforce in a review than watching out for invalid conversions.

Let's land the template-based patch, however.
(Assignee)

Comment 36

3 years ago
This bug can be fixed after bug 1129257 has been resolved.
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1129257
Attachment #8559642 - Flags: review?(tzimmermann)

Comment 37

3 years ago
Verified on
[2.2]
Gaia-Rev        21cce750c095a3f815275fe5439fa9dbfe3dfc6b
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/41ccc5328740
Build-ID        20150209162506
Version         37.0a2
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20150209.194557
FW-Date         Mon Feb  9 19:46:08 EST 2015
Bootloader      L1TC000118D0


[3.0]
Gaia-Rev        0cf517083f7eb5fc269e1236edba50534f65e3cd
Gecko-Rev       https://hg.mozilla.org/mozilla-central/rev/2cb22c058add
Build-ID        20150209160204
Version         38.0a1
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20150209.193059
FW-Date         Mon Feb  9 19:31:10 EST 2015
Bootloader      L1TC000118D0
Status: RESOLVED → VERIFIED
status-b2g-v2.2: affected → fixed
status-b2g-master: affected → fixed
Resolution: DUPLICATE → FIXED

Updated

3 years ago
status-b2g-v2.2: fixed → verified
status-b2g-master: fixed → verified
You need to log in before you can comment on or make changes to this bug.