It's possible that there's something wrong while unmarshalling parcels from nfcd, so it might overflow when convert the ints to String.

[Blocking Requested - why for this release]: regression from Bug 933588

triage: per comment1, this is a potential regression

Yoshi, Do you mean [1] and [2]? It seems converting |mEvent.mOriginType| to string is already safe. Only [1] may cause out-of-bounds array access. I can pick this as my first nfc bug even though it's not quite relevant to nfc :p Thanks! [1] http://hg.mozilla.org/mozilla-central/file/c360f3d1c00d/dom/nfc/gonk/NfcService.cpp#l132 [2] http://hg.mozilla.org/mozilla-central/file/c360f3d1c00d/dom/nfc/gonk/NfcService.cpp#l176

yeah, but I already had patch for this. :P

Comment on attachment 8482658 [details] [diff] [review] Patch Review of attachment 8482658 [details] [diff] [review]: ----------------------------------------------------------------- Hi smaug Bug 1053732 is still under review in Gaia part, so this patch is base on m-c. Could you review this for me? Thanks

Comment on attachment 8482658 [details] [diff] [review] Patch > notifyHCIEventTransaction: function notifyHCIEventTransaction(message) { >+ message.origin = message.originType + message.originIndex; This really needs some explanation. What does the origin mean here? Why can't all the users of the API use originType + originIndex? Or if they can't, why do we need originType + originIndex and not just origin >-static const nsLiteralString SEOriginString[] = { >- NS_LITERAL_STRING("SIM"), >- NS_LITERAL_STRING("ESE"), So here you have "ESE", but the new webidl enum has "eSE". Why that difference? > > // HCI Event Transaction parameters. >- int size = sizeof(SEOriginString) / sizeof(nsLiteralString); >- // TODO: We need a map or something to more rigorously validate against >- // Gonk Message header values from inside NfcService. >- if ((mEvent.mOriginType != -1) && (mEvent.mOriginType < size)) { >- mEvent.mOrigin.Assign(SEOriginString[mEvent.mOriginType]); >- mEvent.mOrigin.AppendInt(mEvent.mOriginIndex, 16 /* radix */); >- event.mOrigin.Construct(); >- event.mOrigin.Value() = mEvent.mOrigin; I have no idea what this old code does, but getting rid of it looks good. > enum NFCTechType { >+ "NDEF", >+ "NDEF_WRITABLE", >+ "NDEF_FORMATABLE", >+ "P2P", > "NFC_A", > "NFC_B", >- "NFC_ISO_DEP", > "NFC_F", > "NFC_V", >- "NDEF", >- "NDEF_FORMATABLE", >+ "NFC_ISO_DEP", > "MIFARE_CLASSIC", > "MIFARE_ULTRALIGHT", >- "NFC_BARCODE", >- "P2P", >- "UNKNOWN_TECH" > }; Why the reordering and why dropping NFC_BARCODE? r- just because I want some explanations (I'm not too familiar with some of this code).

(In reply to Olli Pettay [:smaug] from comment #7) Hi Smaug Sorry for not being clear. > > notifyHCIEventTransaction: function notifyHCIEventTransaction(message) { > >+ message.origin = message.originType + message.originIndex; > This really needs some explanation. What does the origin mean here? It's from GSMA spec, [1] Origin means the source of event, it could be from SIM, embedded SE, or ASSD (SD card with Secure element). And in case that more than one of Secure Elements are of the same kind , from the specification it uses a digit to indentify them, like SIM1, SIM2. > Why can't all the users of the API use originType + originIndex? Also from the same specification, it uses a String to represent the origin, so users of this API will expect they get a String like 'SIM1', so I concat them. > Or if they can't, why do we need originType + originIndex and not just origin > That also works(just origin), that's pretty what the original code is doing. > >-static const nsLiteralString SEOriginString[] = { > >- NS_LITERAL_STRING("SIM"), > >- NS_LITERAL_STRING("ESE"), > So here you have "ESE", but the new webidl enum has "eSE". Why that > difference? > From the specification it uses the term 'eSE' so that's why I do the renaming here. But I'll file another bug to fix the typo since it's not quite related to this bug. > > enum NFCTechType { > >+ "NDEF", > >+ "NDEF_WRITABLE", > >+ "NDEF_FORMATABLE", > >+ "P2P", > > "NFC_A", > > "NFC_B", > >- "NFC_ISO_DEP", > > "NFC_F", > > "NFC_V", > >- "NDEF", > >- "NDEF_FORMATABLE", > >+ "NFC_ISO_DEP", > > "MIFARE_CLASSIC", > > "MIFARE_ULTRALIGHT", > >- "NFC_BARCODE", > >- "P2P", > >- "UNKNOWN_TECH" > > }; > Why the reordering and why dropping NFC_BARCODE? Basically the same idea from your comment https://bugzilla.mozilla.org/show_bug.cgi?id=1053732#c9 The original code from NfcOptions.webidl uses sequence<DOMString> techList, I thought I might reuse the 'enum NFCTechType', and the reordering is to make sure it could also work in lower level deserialization in NfcService.cpp, to make it have the order of the 'nsLiteralString NfcTechString[]' defined NfcService.cpp, so I could remove NfcTechString since WebIDL binding code has already defined that. And dropping NFC_BARCODE is the same reason, NfcTechString[] doesn't define this, but to prevent confusion I'll create another part of patch to remove these non-used enums defined in MozNFCTag.webidl. Thanks for the review and sorry for the confusion here. I'll try to use a DOMString origin in NfcOptions.webidl to see if it's easier. [1] : http://www.gsma.com/newsroom/wp-content/uploads/2014/03/TS26v5-0.pdf

(In reply to Yoshi Huang[:allstars.chh] from comment #8) > And dropping NFC_BARCODE is the same reason, NfcTechString[] doesn't define > this, but to prevent confusion I'll create another part of patch to remove > these non-used enums defined in MozNFCTag.webidl. > Actualy NFC_BARCODE is defined in NfcService but called 'BARCODE', I'll add it back.

Use DOMString origin

Comment on attachment 8483380 [details] [diff] [review] Patch v2. > enum SecureElementOrigin { > SIM = 0, > ESE = 1, > ASSD = 2, >+ OriginEndGuard maybe set to = 3 explicitly

assign OriginEndGuard = 3

try :https://tbpl.mozilla.org/?tree=Try&rev=d3cf55db08ad https://hg.mozilla.org/integration/b2g-inbound/rev/07442dfa17e3

Note that due to recent policy changes, all patches must have approval for uplift regardless of blocking status. Please request aurora approval on this patch when you get a chance. Sorry for the inconvenience :(

Comment on attachment 8483907 [details] [diff] [review] Patch v3. Approval Request Comment [Feature/regressing bug #]: 933588, 979767 [User impact if declined]: Prevent crash by buffer overflow [Describe test coverage new/current, TBPL]: Demo Wallet app [Risks and why]: No [String/UUID change made/needed]: No

(In reply to Yoshi Huang[:allstars.chh] from comment #16) > Comment on attachment 8483907 [details] [diff] [review] > Patch v3. > > Approval Request Comment > [Feature/regressing bug #]: > 933588, 979767 > > [User impact if declined]: > Prevent crash by buffer overflow > > [Describe test coverage new/current, TBPL]: > Demo Wallet app > > [Risks and why]: > No > > [String/UUID change made/needed]: > No Do we have any automation tests for this ?

(In reply to bhavana bajaj [:bajaj] from comment #17) > > Approval Request Comment > > [Feature/regressing bug #]: > > 933588, 979767 > > [Describe test coverage new/current, TBPL]: > > Demo Wallet app Sorry, should be Marionette-webapi + Demo Wallet app > > > Do we have any automation tests for this ? For the 'techList' part check (from Bug 933588), marionette-webapi also covers it. But from the 'origin' (from Bug 979767), there's no automation test yet, so using a demo app to verify this.

(In reply to Yoshi Huang[:allstars.chh] from comment #18) > (In reply to bhavana bajaj [:bajaj] from comment #17) > > > Approval Request Comment > > > [Feature/regressing bug #]: > > > 933588, 979767 > > > [Describe test coverage new/current, TBPL]: > > > Demo Wallet app > Sorry, should be Marionette-webapi + Demo Wallet app > > > > > > Do we have any automation tests for this ? > > For the 'techList' part check (from Bug 933588), marionette-webapi also > covers it. > But from the 'origin' (from Bug 979767), there's no automation test yet, so > using a demo app to verify this. Can you please file a separate bug to add missing automation tests here and check that in ?