Closed Bug 1034660 Opened 5 years ago Closed 5 years ago

[NFC] Emulator encodes NDEF incorrectly when ID is used

Categories

(Firefox OS Graveyard :: NFC, defect)

x86
macOS
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: kamituel, Assigned: tzimmermann)

References

Details

Attachments

(2 files)

Consider this emulator telnet command:

> nfc snep put -1 -1 [0,1,SHM,EtECBGFjAQEwAA,][0,2,YXBwbGljYXRpb24vdm5kLmJsdWV0b290aC5lcC5vb2I,FgCrledEDQANCVVFIE1JTkkgQk9PTQ,MA]
> OK

It has two records (it's a handover select message).
Second record has ID assigned (Base64 "MA", decimal 48).

Once this method is executed, Gaia system app receives following message:

> {
>    "records": [
>        {
>            "id": {},
>            "payload": [18, 209, 2, 4, 97, 99, 1, 1, 48, 0],
>            "tnf": 1,
>            "type": [72, 115]
>        },
>        {
>            "id": [22],
>            "payload": [0, 171, 149, 231, 13, 0, 13, 9, 85, 69, 32, 77, 73, 78, 73, 32, 66, 79, 79, 77, 48],
>            "tnf": 2,
>            "type": [/* ok here */]
>        }
>    ],
>    "sessionToken": "{adfe9af9-1eed-4504-9a24-c89d53c41218}",
>    "techList": [
>        "P2P",
>        "NDEF"
>    ],
>    "type": "techDiscovered"
> }

First record is okay. But look at second record:
- ID is dec 22, when it should be dec 48.
- Missing ID (dec 48) is now the last byte of payload
- And current, invalid ID (dec 22) should be the first byte of payload, but it's missing.

For reference:
Payload [22, 0, 171, 149, 231, 68, 13, 0, 13, 9, 85, 69, 32, 77, 73, 78, 73, 32, 66, 79, 79, 77] in Base64 is "FgCrledEDQANCVVFIE1JTkkgQk9PTQ".

Note: if I invoke the same command, but with no "MA" ID, it is handled correctly.
Thomas, am I using this command incorrectly?
Flags: needinfo?(tzimmermann)
Hmm, IIRC 'id' is supposed to be a string. That's why it's base64 encoded. The length of a base64 string is always a multiple of 4. Unused positions at the end should be filled with '='. Encoding "48" as base64 is "NDg=". Does that work for you?
Flags: needinfo?(tzimmermann)
Sorry, indeed I used the incorrect Base64 encoded values (I was missing the padding).
This command should be better:

> nfc snep put -1 -1 [0,1,SHM=,EtECBGFjAQEwAA==,][0,2,YXBwbGljYXRpb24vdm5kLmJsdWV0b290aC5lcC5vb2I=,FgCrledEDQANCVVFIE1JTkkgQk9PTQ==,MA==]

But it still fails with the same behaviour.
I can see the incorrect value in nfcd. This is what the emulator decoded:

> (gdb) print *buf@80
> $19 = "\201\002\000\000\000\nHs\022\321\002\004ac\001\001\060\000J \000\000\000\026\001application/vnd.bluetooth.ep.oob\026\000\253\225\347D\r\000\r\tUE MINI BOOM0"

The final character is '0', which looks correct. So the bug must be somewhere in between.
In nfcd's logcat I can see the messages as shown below.

D/BrcmNciR(   47): 00005913040010020000005081020000000a487312d102046163010130004a2000000016016170706c69636174696f6e2f766e642e626c7565746f6f74682e65702e6f6f621600ab95e7440d000d095545204d494e4920424f4f4d30

This is the raw SNEP PUT as received from the emulator. Using an ASCII table, the final 5 characters are 'B', 'O', 'O', 'M', and '0'. Still looks correct.

D/BrcmNciX(   47): 000003134401
D/nfcd    (   47): SnepMessage* SnepMessenger::getMessage(): exit
D/nfcd    (   47): processNotificaton notification=2001
D/nfcd    (   47): numRecords=2
D/nfcd    (   47): tnf=1
D/nfcd    (   47): typeLength=2
D/nfcd    (   47): idLength=0
D/nfcd    (   47): payloadLength=10
D/nfcd    (   47): mPayload 0 = 18
D/nfcd    (   47): mPayload 1 = 209
D/nfcd    (   47): mPayload 2 = 2
D/nfcd    (   47): mPayload 3 = 4
D/nfcd    (   47): mPayload 4 = 97
D/nfcd    (   47): mPayload 5 = 99
D/nfcd    (   47): mPayload 6 = 1
D/nfcd    (   47): mPayload 7 = 1
D/nfcd    (   47): mPayload 8 = 48
D/nfcd    (   47): mPayload 9 = 0
D/nfcd    (   47): tnf=2
D/nfcd    (   47): typeLength=32
D/nfcd    (   47): idLength=1
D/nfcd    (   47): mId 0 = 22
D/nfcd    (   47): payloadLength=22
D/nfcd    (   47): mPayload 0 = 0
D/nfcd    (   47): mPayload 1 = 171
D/nfcd    (   47): mPayload 2 = 149
D/nfcd    (   47): mPayload 3 = 231
D/nfcd    (   47): mPayload 4 = 68
D/nfcd    (   47): mPayload 5 = 13
D/nfcd    (   47): mPayload 6 = 0
D/nfcd    (   47): mPayload 7 = 13
D/nfcd    (   47): mPayload 8 = 9
D/nfcd    (   47): mPayload 9 = 85
D/nfcd    (   47): mPayload 10 = 69
D/nfcd    (   47): mPayload 11 = 32
D/nfcd    (   47): mPayload 12 = 77
D/nfcd    (   47): mPayload 13 = 73
D/nfcd    (   47): mPayload 14 = 78
D/nfcd    (   47): mPayload 15 = 73
D/nfcd    (   47): mPayload 16 = 32
D/nfcd    (   47): mPayload 17 = 66
D/nfcd    (   47): mPayload 18 = 79
D/nfcd    (   47): mPayload 19 = 79
D/nfcd    (   47): mPayload 20 = 77
D/nfcd    (   47): mPayload 21 = 48

And this is what get's copied into the nfcd protocol's PDU. The final character is "0". This also looks correct to me. Anything missing on my side? :/
(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #4)
> I can see the incorrect value in nfcd. This is what the emulator decoded:

I mean 'I can't see it'.
And this is what i see in Gecko:

I/Gecko   (   45): Nfc Worker: Received 136 bytes.
I/Gecko   (   45): Nfc Worker: Already read 0
I/Gecko   (   45): Nfc Worker: New incoming parcel of size 132
I/Gecko   (   45): Nfc Worker: Parcel (size 132): 209,7,0,0,1,0,0,0,2,0,0,0,3,0,0,0,1,0,0,0,2,0,0,0,1,0,0,0,2,0,0,0,72,115,0,0,0,0,0,0,10,0,0,0,18,209,2,4,97,99,1,1,48,0,0,0,2,0,0,0,32,0,0,0,97,112,112,108,105,99,97,116,105,111,110,47,118,110,100,46,98,108,117,101,116,111,111,116,104,46,101,112,46,111,111,98,1,0,0,0,22,0,0,0,22,0,0,0,0,171,149,231,68,13,0,13,9,85,69,32,77,73,78,73,32,66,79,79,77,48,0,0

The PDU still looks correct here.


I/Gecko   (   45): Nfc Worker: We have at least one complete parcel.
I/Gecko   (   45): Nfc Worker: Number of bytes available in Parcel : 128
I/Gecko   (   45): Nfc Worker: Handling parcel as NFC_NOTIFICATION_TECH_DISCOVERED
I/Gecko   (   45): Nfc Worker: NFC_NOTIFICATION_TECH_DISCOVERED
D/nfcd    (   47): void SnepMessenger::sendMessage(SnepMessage&): enter
D/NfcNci  (   47): PeerToPeer::nfaServerCallback: NFA_P2P_CONGEST_EVT; nfa handle: 0x0580  congested: 1
D/nfcd    (   47): void SnepMessenger::sendMessage(SnepMessage&): exit
D/nfcd    (   47): SnepMessage* SnepMessenger::getMessage(): enter
I/Gecko   (   45): Nfc Worker: numOfRecords = 2
I/Gecko   (   45): -*- Nfc: Received message from NFC worker: {"type":"techDiscovered","sessionId":1,"techList":["P2P","NDEF"],"records":[{"tnf":1,"type":{"0":72,"1":115},"id":{},"payload":{"0":18,"1":209,"2":2,"3":4,"4":97,"5":99,"6":1,"7":1,"8":48,"9":0}},{"tnf":2,"type":{"0":97,"1":112,"2":112,"3":108,"4":105,"5":99,"6":97,"7":116,"8":105,"9":111,"10":110,"11":47,"12":118,"13":110,"14":100,"15":46,"16":98,"17":108,"18":117,"19":101,"20":116,"21":111,"22":111,"23":116,"24":104,"25":46,"26":101,"27":112,"28":46,"29":111,"30":111,"31":98},"id":{"0":22},"payload":{"0":0,"1":171,"2":149,"3":231,"4":68,"5":13,"6":0,"7":13,"8":9,"9":85,"10":69,"11":32,"12":77,"13":73,"14":78,"15":73,"16":32,"17":66,"18":79,"19":79,"20":77,"21":48}}]}

But here, the id is incorrect. The bug seems to be in the NFC worker.
Oh, I think I see the problem and it *is* in the emulator. The NDEF records's fields have the order [type, id, payload], but the emulator uses [type, payload, id].
Assignee: nobody → tzimmermann
Status: NEW → ASSIGNED
Attached file Github pull request
Attachment #8453031 - Flags: review?(dlee)
Hi Dimi,

I still have problems running test cases in the emulator. Could you apply the patch to the emulator and Gecko, and check if the tests still work? Thanks.
Attachment #8453034 - Flags: review?(dlee)
The Gecko patch depends on the test-case cleanups.
Depends on: 1035606
Attachment #8453034 - Flags: review?(dlee) → review+
Comment on attachment 8453031 [details]
Github pull request

Looks good to me, thanks!
Testcase passed after applying qemu and testcase patch on my local platform.
Attachment #8453031 - Flags: review?(dlee) → review+
https://hg.mozilla.org/mozilla-central/rev/0958d9abca75
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.