B2G RIL: Add support for decoding FCP Template in BerTlvHelper.

RESOLVED FIXED in Firefox 28, Firefox OS v1.3

Status

Firefox OS
RIL
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: allstars, Assigned: sku)

Tracking

unspecified
1.3 C2/1.4 S2(17jan)
ARM
Gonk (Firefox OS)

Firefox Tracking Flags

(blocking-b2g:1.3+, firefox27 wontfix, firefox28 fixed, firefox29 fixed, b2g-v1.3 fixed)

Details

Attachments

(3 attachments, 11 obsolete attachments)

13.17 KB, patch
Details | Diff | Splinter Review
3.49 KB, patch
Details | Diff | Splinter Review
16.51 KB, patch
Details | Diff | Splinter Review
This is found by Shawn.
FCP Template is also one of BER TLV format. (TS 101.220, clause 7.2)

We could reuse BerTlvHelper To decode FCP template from USIM (TS 102.221, 11.1.1.3.2).
fugu? because it blocks fugu+ bug 924740.
blocking-b2g: --- → fugu?
Sam, could you help to check this issue?
Flags: needinfo?(sam.hua)

Updated

4 years ago
Flags: needinfo?(sam.hua)

Comment 3

4 years ago
add ming.li.

Hi Steven, 
Any requirements for RILC or modem about this issue?
(Assignee)

Comment 4

4 years ago
Created attachment 8346437 [details] [diff] [review]
Test Patch for Bug 947110.

This is not a formal patch. And, still need test case part.

Comment 5

4 years ago
so 924740 will also be fixed after this modification?
(Assignee)

Comment 6

4 years ago
(In reply to sam.hua from comment #5)
> so 924740 will also be fixed after this modification?

Yes, (924740 and 947110 are the same issue.)
(Assignee)

Comment 7

4 years ago
Comment on attachment 8346437 [details] [diff] [review]
Test Patch for Bug 947110.

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

::: dom/system/gonk/ril_worker.js
@@ +4872,5 @@
>     * Process STK Proactive Command.
>     */
>    processStkProactiveCommand: function processStkProactiveCommand() {
>      let length = Buf.readInt32();
> +    let berTlv; 

remove space.
(In reply to shawn ku [:sku] from comment #6)
> (In reply to sam.hua from comment #5)
> > so 924740 will also be fixed after this modification?
> 
> Yes, (924740 and 947110 are the same issue.)

No, this bug is to add more functionalities in BerTlvHelper.

Bug 924740 is to handle the result the decoded by BerTLVHelper.

That's why we have seperate bugs.
Comment on attachment 8346437 [details] [diff] [review]
Test Patch for Bug 947110.

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

I guess you want to take this bug so have some comments here.

::: dom/system/gonk/ril_consts.js
@@ +705,5 @@
> +this.BER_FCP_SA_COMPACT_FORMAT_TAG = 0x8c; // Security Attribute - Compact Format.
> +this.BER_FCP_SAT_EXPANDED_FORMAT_TAG = 0xab; // Security Attribute Template - Expanded Format.
> +this.BER_FCP_PROPRIETARY_TEMPLATE_TAG = 0xa5;
> +this.BER_FCP_PIN_STATUS_DATA_OBJECTS_TAG = 0xc6;
> +

These consts should be put below with those existing values.

Also put those definitions in asscending order.

::: dom/system/gonk/ril_worker.js
@@ +4875,5 @@
>      let length = Buf.readInt32();
> +    let berTlv; 
> +    try {
> +      berTlv = BerTlvHelper.decode({tag: BER_PROACTIVE_COMMAND_TAG,
> +                                    dataLen: length / 2});

simply BerTlvHelper.decode(dataLen);

@@ +10852,5 @@
>    /**
>     * Decode Ber TLV.
>     *
> +   * @param tag
> +   *        The expected tag to be parsing.

no need.

@@ +10865,5 @@
>  
> +    if (options.tag !== tag) {
> +      throw new Error("Unexpected Ber tag 0x" + tag.toString(16));
> +      return null;
> +    }

no need.

@@ +10866,5 @@
> +    if (options.tag !== tag) {
> +      throw new Error("Unexpected Ber tag 0x" + tag.toString(16));
> +      return null;
> +    }
> +

should decode the length here.

this.decodeLength(dataLen);

Or call it processLength if you like.

@@ +10872,5 @@
> +    if (typeof method != "function") {
> +      throw new Error("Unknown Ber tag 0x" + tag.toString(16));
> +      return;
> +    }
> +

Since the length is handled here.

So you need to call those processXXX functions to get the value part.

let value = method.call(length);

return {
  tag: tag,
  length: length,
  value: value
};

@@ +11252,5 @@
>    /**
>     * Process a ICC_COMMAND_GET_RESPONSE type command for REQUEST_SIM_IO.
>     */
>    processICCIOGetResponse: function processICCIOGetResponse(options) {
>      let strLen = Buf.readInt32();

Move the code below to Bug 924740

@@ +13710,5 @@
>  
>  /**
> + * Helper for ICC FCP Template.
> + */
> +let ICCFcpTemplateHelper = {

It seems to me the code here is overlapped with BerTlvHelper.

And seems to me we won't need this helper and we can do it in BerTlvHelper.
(Assignee)

Comment 10

4 years ago
Comment on attachment 8346437 [details] [diff] [review]
Test Patch for Bug 947110.

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

::: dom/system/gonk/ril_worker.js
@@ +10889,5 @@
> +    // The length is either 1 or 2 for FCP template.
> +    let length = GsmPDUHelper.readHexOctet();
> +    hlen++;
> +    if (dataLen !== (hlen + length)) {
> +      length = (length << 8) | GsmPDUHelper.readHexOctet();

This part is incorrect, need correction. see TS 101 220 7.1.2 Length encoding
How does it relates to https://bugzilla.mozilla.org/show_bug.cgi?id=904038 ?
(Assignee)

Comment 12

4 years ago
Created attachment 8348535 [details] [diff] [review]
Bug 947110 - Part1: RIL patch. B2G RIL: Add support for decoding FCP Template in BerTlvHelper.
Attachment #8346437 - Attachment is obsolete: true
(Assignee)

Comment 13

4 years ago
Created attachment 8348537 [details] [diff] [review]
Bug 947110 - Part2: Add test cases. B2G RIL: Add support for decoding FCP Template in BerTlvHelper.
(Assignee)

Updated

4 years ago
Attachment #8348535 - Flags: review?(allstars.chh)
(Assignee)

Updated

4 years ago
Attachment #8348537 - Flags: review?(allstars.chh)
(Assignee)

Updated

4 years ago
Assignee: nobody → sku
Comment on attachment 8348535 [details] [diff] [review]
Bug 947110 - Part1: RIL patch. B2G RIL: Add support for decoding FCP Template in BerTlvHelper.

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

Mostly okay,
cancelling r? for I'd like to revise the handling of TLV object.

::: dom/system/gonk/ril_worker.js
@@ +10864,5 @@
>      let hlen = 0;
>      let tag = GsmPDUHelper.readHexOctet();
>      hlen++;
>  
> +    // The length is either 1 or 2 for Ber TLV format.

The length is coded onto 1 or 2 bytes.

@@ +10892,5 @@
> +    if (typeof method != "function") {
> +      throw new Error("Unknown Ber tag 0x" + tag.toString(16));
> +    }
> +
> +    return method.call(this, length);

So you return TLV in the method, which I think is not a good way to do this.

You decode tag octet and length octet(s) here, so it's better to construct the TLV in this function, which means the handler for BerTlvHelper is only to process the 'value' part.

Otherwise you can see each of your handle has to copy the 'tag' and 'length' from this function.

@@ +10939,5 @@
> +    return {
> +      tag: tag,
> +      length: length,
> +      value: this.retrieve(tag, length),
> +      hlen: hlen

we don't have to pass hlen.
Since the tag and the length will take 2 bytes.

@@ +10962,5 @@
> +    let method = this[tag];
> +    if (typeof method != "function") {
> +      if (DEBUG) {
> +        debug("Unknown Ber tag : 0x" + tag.toString(16));
> +      }

throw new Error(...);
Attachment #8348535 - Flags: review?(allstars.chh)
(Assignee)

Comment 15

4 years ago
(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #14)
> Comment on attachment 8348535 [details] [diff] [review]
> Bug 947110 - Part1: RIL patch. B2G RIL: Add support for decoding FCP
> Template in BerTlvHelper.
> 
> Review of attachment 8348535 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Mostly okay,
> cancelling r? for I'd like to revise the handling of TLV object.
> 
> ::: dom/system/gonk/ril_worker.js
> @@ +10864,5 @@
> >      let hlen = 0;
> >      let tag = GsmPDUHelper.readHexOctet();
> >      hlen++;
> >  
> > +    // The length is either 1 or 2 for Ber TLV format.
> 
> The length is coded onto 1 or 2 bytes.

Will address this in next patch.

> 
> @@ +10892,5 @@
> > +    if (typeof method != "function") {
> > +      throw new Error("Unknown Ber tag 0x" + tag.toString(16));
> > +    }
> > +
> > +    return method.call(this, length);
> 
> So you return TLV in the method, which I think is not a good way to do this.
> 
> You decode tag octet and length octet(s) here, so it's better to construct
> the TLV in this function, which means the handler for BerTlvHelper is only
> to process the 'value' part.
> 
> Otherwise you can see each of your handle has to copy the 'tag' and 'length'
> from this function.
> 

Will address this in next patch.

> @@ +10939,5 @@
> > +    return {
> > +      tag: tag,
> > +      length: length,
> > +      value: this.retrieve(tag, length),
> > +      hlen: hlen
> 
> we don't have to pass hlen.
> Since the tag and the length will take 2 bytes.

Will do.

> 
> @@ +10962,5 @@
> > +    let method = this[tag];
> > +    if (typeof method != "function") {
> > +      if (DEBUG) {
> > +        debug("Unknown Ber tag : 0x" + tag.toString(16));
> > +      }
> 
> throw new Error(...);

"throw new Error(...);" might not be a proper action we should take here.
We got 8 tags from getResponse, and we only care about 0x82/0x83/0x80 currently.

If throw a new excpetion, we need to catch that within the same function block.
otherwise, there is no way for us to trigger "Buf.seekIncoming(length * Buf.PDU_HEX_OCTET_SIZE);" to ignore the chunk we don't need. That's why I use log for debugging's convenient.

What is you opinion?

Description                   Tag
File Descriptor               0x82
File Identifier               0x83
Proprietary information       0xA5
Life Cycle Status Integer     0x8A
Security attributes           0x8B || 0x8C || 0xAB
File size                     0x80
Total file size               0x81
Short File Identifier (SFI)   0x88
(Assignee)

Comment 16

4 years ago
Hi Yoshi:
 Could you please check my bottom reply on comment 15?
Flags: needinfo?(allstars.chh)

Updated

4 years ago
blocking-b2g: fugu? → fugu+
(In reply to shawn ku [:sku] from comment #16)
> Hi Yoshi:
>  Could you please check my bottom reply on comment 15?

seekIncoming is better, please ignore my previous comment for 'throw new Error'.
Flags: needinfo?(allstars.chh)
(Assignee)

Comment 18

4 years ago
Created attachment 8349288 [details] [diff] [review]
Bug 947110 - Part1: RIL patch. B2G RIL: Add support for decoding FCP Template in BerTlvHelper. v2.
Attachment #8348535 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #8349288 - Flags: review?(allstars.chh)
Comment on attachment 8349288 [details] [diff] [review]
Bug 947110 - Part1: RIL patch. B2G RIL: Add support for decoding FCP Template in BerTlvHelper. v2.

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

::: dom/system/gonk/ril_worker.js
@@ +10892,5 @@
> +    if (typeof method != "function") {
> +      throw new Error("Unknown Ber tag 0x" + tag.toString(16));
> +    }
> +
> +    return method.call(this, length);

? you only return 'value' object, instead of TLV ?

let value = method.call(...);

return {
  tag: tag,
  length: length,
  value: value
};

@@ +10903,5 @@
> +   *        The length of data in bytes.
> +   */
> +  processFcpTemplate: function processFcpTemplate(length) {
> +    let tlvs = this.decodeChunks(length);
> +    return {value: tlvs};

return tlvs;

@@ +10917,2 @@
>      let ctlvs = ComprehensionTlvHelper.decodeChunks(length);
> +    return {value: ctlvs};

ditto

@@ +10992,5 @@
> +    let dataCodingByte = GsmPDUHelper.readHexOctet();
> +    // See TS 102 221 Table 11.5, we only care the first 3 bytes for EF type.
> +    let fileType = fileDescriptorByte & 0x07;
> +
> +    let fileDescriptor;

let fileDescriptor = {
  fileType: fileType
};

@@ +11002,5 @@
> +        fileType: fileType,
> +        recordLength: ((GsmPDUHelper.readHexOctet() << 8) +
> +                       GsmPDUHelper.readHexOctet()),
> +        numOfRecords: GsmPDUHelper.readHexOctet()
> +      };

fileDescriptor.recordLength = ..;
fileDescriptor.numOfRecords = ...;

@@ +11003,5 @@
> +        recordLength: ((GsmPDUHelper.readHexOctet() << 8) +
> +                       GsmPDUHelper.readHexOctet()),
> +        numOfRecords: GsmPDUHelper.readHexOctet()
> +      };
> +    } else {

remove else.

@@ +11023,5 @@
> +    return {fileId : (GsmPDUHelper.readHexOctet() << 8) +
> +                      GsmPDUHelper.readHexOctet()};
> +  },
> +
> +  searchForTag: function searchForTag(tag, tlvs) {

who uses this function?

@@ +11028,5 @@
> +    let iter = Iterator(tlvs);
> +    return this.searchForNextTag(tag, iter);
> +  },
> +
> +  searchForNextTag: function searchForNextTag(tag, iter) {

here too.
Attachment #8349288 - Flags: review?(allstars.chh) → review-
(Assignee)

Comment 20

4 years ago
(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #19)
> Comment on attachment 8349288 [details] [diff] [review]
> Bug 947110 - Part1: RIL patch. B2G RIL: Add support for decoding FCP
> Template in BerTlvHelper. v2.
> 
> Review of attachment 8349288 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/ril_worker.js
> @@ +10892,5 @@
> > +    if (typeof method != "function") {
> > +      throw new Error("Unknown Ber tag 0x" + tag.toString(16));
> > +    }
> > +
> > +    return method.call(this, length);
> 
> ? you only return 'value' object, instead of TLV ?

value contains berTLV (from FCP template) or COMPREHENSION-TLV (from Card application toolkit data objects)
the origin tag/lenght are not used outside, that's why I only return value.

I will add tag/length back in next patch. 

> 
> let value = method.call(...);
> 
> return {
>   tag: tag,
>   length: length,
>   value: value
> };
> 
> @@ +10903,5 @@
> > +   *        The length of data in bytes.
> > +   */
> > +  processFcpTemplate: function processFcpTemplate(length) {
> > +    let tlvs = this.decodeChunks(length);
> > +    return {value: tlvs};
> 
> return tlvs;
> 
> @@ +10917,2 @@
> >      let ctlvs = ComprehensionTlvHelper.decodeChunks(length);
> > +    return {value: ctlvs};
> 
> ditto
> 
> @@ +10992,5 @@
> > +    let dataCodingByte = GsmPDUHelper.readHexOctet();
> > +    // See TS 102 221 Table 11.5, we only care the first 3 bytes for EF type.
> > +    let fileType = fileDescriptorByte & 0x07;
> > +
> > +    let fileDescriptor;
> 
> let fileDescriptor = {
>   fileType: fileType
> };
> 
> @@ +11002,5 @@
> > +        fileType: fileType,
> > +        recordLength: ((GsmPDUHelper.readHexOctet() << 8) +
> > +                       GsmPDUHelper.readHexOctet()),
> > +        numOfRecords: GsmPDUHelper.readHexOctet()
> > +      };
> 
> fileDescriptor.recordLength = ..;
> fileDescriptor.numOfRecords = ...;
> 

Thanks! Will do.

> @@ +11003,5 @@
> > +        recordLength: ((GsmPDUHelper.readHexOctet() << 8) +
> > +                       GsmPDUHelper.readHexOctet()),
> > +        numOfRecords: GsmPDUHelper.readHexOctet()
> > +      };
> > +    } else {
> 
> remove else.
> 
> @@ +11023,5 @@
> > +    return {fileId : (GsmPDUHelper.readHexOctet() << 8) +
> > +                      GsmPDUHelper.readHexOctet()};
> > +  },
> > +
> > +  searchForTag: function searchForTag(tag, tlvs) {
> 
> who uses this function?

Shince this is a helper function, I prefer to let searchForTag/searchForNextTag APIs rady first.
It looks like to me you think there is no caller, these APIs does not need to be existed.

however, w/o these APIs, it's hard for us to write meaningful test case down.
> 
> @@ +11028,5 @@
> > +    let iter = Iterator(tlvs);
> > +    return this.searchForNextTag(tag, iter);
> > +  },
> > +
> > +  searchForNextTag: function searchForNextTag(tag, iter) {
> 
> here too.
(In reply to shawn ku [:sku] from comment #20)
> 
> Shince this is a helper function, I prefer to let
> searchForTag/searchForNextTag APIs rady first.
> It looks like to me you think there is no caller, these APIs does not need
> to be existed.
> 
> however, w/o these APIs, it's hard for us to write meaningful test case down.

You could write your util in the test case.
Also when reviewing this I think I should file another bug for creating some util function for ComprehensionTlvHelper, BerTlvHelper for handling TLV.
(Assignee)

Comment 22

4 years ago
(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #21)
> (In reply to shawn ku [:sku] from comment #20)
> > 
> > Shince this is a helper function, I prefer to let
> > searchForTag/searchForNextTag APIs rady first.
> > It looks like to me you think there is no caller, these APIs does not need
> > to be existed.
> > 
> > however, w/o these APIs, it's hard for us to write meaningful test case down.
> 
> You could write your util in the test case.
> Also when reviewing this I think I should file another bug for creating some
> util function for ComprehensionTlvHelper, BerTlvHelper for handling TLV.

I believe the ComprehensionTlvHelper was covered by test_stk_proactive_command_search_next_tag@test_ril_worker_stk.js unless you think it is necessary to add more tests on it.
(Assignee)

Comment 23

4 years ago
Created attachment 8349891 [details] [diff] [review]
Bug 947110 - Part1: RIL patch. B2G RIL: Add support for decoding FCP Template in BerTlvHelper. v3.
Attachment #8349288 - Attachment is obsolete: true
(Assignee)

Comment 24

4 years ago
Comment on attachment 8349891 [details] [diff] [review]
Bug 947110 - Part1: RIL patch. B2G RIL: Add support for decoding FCP Template in BerTlvHelper. v3.

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

Update address parts for part1 patch.
Attachment #8349891 - Flags: review?(allstars.chh)
(Assignee)

Comment 25

4 years ago
Comment on attachment 8348537 [details] [diff] [review]
Bug 947110 - Part2: Add test cases. B2G RIL: Add support for decoding FCP Template in BerTlvHelper.

Obsolete part2 since searchForTag/searchForNextTag are removed.
Attachment #8348537 - Attachment is obsolete: true
Attachment #8348537 - Flags: review?(allstars.chh)
(Assignee)

Comment 26

4 years ago
Created attachment 8349904 [details] [diff] [review]
Bug 947110 - Part2: Add test cases. B2G RIL: Add support for decoding FCP Template in BerTlvHelper.
(Assignee)

Updated

4 years ago
Attachment #8349904 - Flags: review?(allstars.chh)
Comment on attachment 8349891 [details] [diff] [review]
Bug 947110 - Part1: RIL patch. B2G RIL: Add support for decoding FCP Template in BerTlvHelper. v3.

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

::: dom/system/gonk/ril_consts.js
@@ +536,4 @@
>  this.RESPONSE_DATA_STRUCTURE = 13;
>  this.RESPONSE_DATA_RECORD_LENGTH = 14;
>  
>  // Types of files  TS 11.11 9.3

Actually it should be struture of file.

Structure could be LinearFixed, Cyclic, Transparent or BerTLV.
Type could be MF, DF or EF.

I'll file another bug to fix this.

@@ +541,5 @@
>  this.EF_TYPE_LINEAR_FIXED = 1;
>  this.EF_TYPE_CYCLIC = 3;
>  
> +// TS 102.221 11.1.1.4.3 Table 11.5: File descriptor byte.
> +this.UICC_EF_TYPE = {};

rename it to structure, not type.

::: dom/system/gonk/ril_worker.js
@@ +10895,5 @@
> +
> +    let value = method.call(this, length);
> +
> +    return {
> +     tag: tag,

indent. 
Use two spaces.

@@ +10902,5 @@
> +    };
> +  },
> +
> +  /**
> +   * Construct a param for FCP template tlv.

process the value part for FCP template TLV.

@@ +10913,5 @@
> +    return tlvs;
> +  },
> +
> +  /**
> +   * Construct a param for proactive command tlv.

ditto.

@@ +10926,5 @@
> +
> +  /**
> +   * Decode raw data to a Ber-TLV.
> +   */
> +  decodeBerTlv: function decodeBerTlv() {

decodeInnerTlv, since you're decoding the inner BER TLVs, and try to prevent confusion with original decode().

@@ +10945,5 @@
> +      if (tlv.value) {
> +        chunks.push(tlv);
> +      }
> +      index += tlv.length;
> +      // tag + length fields consume 2 bytes for FCP template.

remove 'for FCP template'.

Since there are many kinds of BER tags.

@@ +10975,5 @@
> +   */
> +  retrieveFileSizeData: function retrieveFileSizeData(length) {
> +    let fileSizeData = 0;
> +    for (let i = 0; i < length; i++) {
> +      fileSizeData += (GsmPDUHelper.readHexOctet() << (8*(length-i-1)));

fileSizeData = fileSizeData << 8;
fileSizeData += GsmPDUHelper.readHexOctet();

@@ +10995,5 @@
> +   */
> +  retrieveFileDescriptor: function retrieveFileDescriptor(length) {
> +    let fileDescriptorByte = GsmPDUHelper.readHexOctet();
> +    let dataCodingByte = GsmPDUHelper.readHexOctet();
> +    // See TS 102 221 Table 11.5, we only care the first 3 bytes for EF type.

the least 3 bits for the structure of file.

@@ +10996,5 @@
> +  retrieveFileDescriptor: function retrieveFileDescriptor(length) {
> +    let fileDescriptorByte = GsmPDUHelper.readHexOctet();
> +    let dataCodingByte = GsmPDUHelper.readHexOctet();
> +    // See TS 102 221 Table 11.5, we only care the first 3 bytes for EF type.
> +    let fileType = fileDescriptorByte & 0x07;

rename to 'structure'.
Attachment #8349891 - Flags: review?(allstars.chh)
(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #27)
 > ::: dom/system/gonk/ril_consts.js
> @@ +536,4 @@
> >  this.RESPONSE_DATA_STRUCTURE = 13;
> >  this.RESPONSE_DATA_RECORD_LENGTH = 14;
> >  
> >  // Types of files  TS 11.11 9.3
> 
> Actually it should be struture of file.
> 
> Structure could be LinearFixed, Cyclic, Transparent or BerTLV.
> Type could be MF, DF or EF.
> 
> I'll file another bug to fix this.
> 

Bug 952025.
(Assignee)

Comment 29

4 years ago
Created attachment 8349954 [details] [diff] [review]
Bug 947110 - Part1: RIL patch. B2G RIL: Add support for decoding FCP Template in BerTlvHelper. v4.
Attachment #8349891 - Attachment is obsolete: true
(Assignee)

Comment 30

4 years ago
Created attachment 8349957 [details] [diff] [review]
Bug 947110 - Part2: Add test cases. B2G RIL: Add support for decoding FCP Template in BerTlvHelper. v2.
Attachment #8349904 - Attachment is obsolete: true
Attachment #8349904 - Flags: review?(allstars.chh)
(Assignee)

Updated

4 years ago
Attachment #8349954 - Flags: review?(allstars.chh)
(Assignee)

Updated

4 years ago
Attachment #8349957 - Flags: review?(allstars.chh)
Comment on attachment 8349954 [details] [diff] [review]
Bug 947110 - Part1: RIL patch. B2G RIL: Add support for decoding FCP Template in BerTlvHelper. v4.

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

Thanks.
Add r=me.
Attachment #8349954 - Flags: review?(allstars.chh) → review+
(Reporter)

Updated

4 years ago
Attachment #8349957 - Flags: review?(allstars.chh) → review+
(Assignee)

Comment 33

4 years ago
Created attachment 8350408 [details] [diff] [review]
Bug 947110 - Part1: RIL patch. B2G RIL: Add support for decoding FCP Template in BerTlvHelper. v4. r=yoshi.
Attachment #8349954 - Attachment is obsolete: true
(Assignee)

Comment 34

4 years ago
Created attachment 8350409 [details] [diff] [review]
Bug 947110 - Part2: Add test cases. B2G RIL: Add support for decoding FCP Template in BerTlvHelper. v2. r=yoshi.
Attachment #8349957 - Attachment is obsolete: true
(Assignee)

Comment 35

4 years ago
try server - https://tbpl.mozilla.org/?tree=Try&rev=8a7787facb6a
Keywords: checkin-needed
From the try I think Marionette-webapi tests are fail.
Keywords: checkin-needed
(Assignee)

Comment 37

4 years ago
Thanks Yoshi, I will check what cause this failures on Monday morning.
(Assignee)

Comment 38

4 years ago
Thanks for Youshi again....

(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #36)
> From the try I think Marionette-webapi tests are fail.

The root cause is:
"if (dataLen - hlen < length) {" was changed to "if (dataLen - hlen !== length) {" in patch

and for two-bytes length BerTLV, hlen++ is missed (should be a old bug, but not found yet).
That cause an unexpected expcetion thrown.

I will provide a new patch to review on Monday.



// Original one
    } else if (temp === 0x81) {
      length = GsmPDUHelper.readHexOctet();

// Fixed one
    } else if (temp === 0x81) {
      length = GsmPDUHelper.readHexOctet();
+      hlen++;



  decode: function decode(dataLen) {
    let hlen = 0;
    let tag = GsmPDUHelper.readHexOctet();
    hlen++;

    // The length is coded onto 1 or 2 bytes.
    // Length    | Byte 1                | Byte 2
    // 0 - 127   | length ('00' to '7f') | N/A
    // 128 - 255 | '81'                  | length ('80' to 'ff')
    let length;
    let temp = GsmPDUHelper.readHexOctet();
    hlen++;
    if (temp < 0x80) {
      length = temp;
    } else if (temp === 0x81) {
      length = GsmPDUHelper.readHexOctet();
      if (length < 0x80) {
        throw new Error("Invalid length " + length);
      }
    ...
(Assignee)

Comment 39

4 years ago
Created attachment 8350973 [details] [diff] [review]
Bug 947110 - Part1: RIL patch. B2G RIL: Add support for decoding FCP Template in BerTlvHelper. v5.
Attachment #8350408 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #8350973 - Flags: review?(allstars.chh)
(Assignee)

Comment 40

4 years ago
try result is green -
https://tbpl.mozilla.org/?tree=Try&rev=0241d1ecaf9f
(Reporter)

Updated

4 years ago
Attachment #8350973 - Flags: review?(allstars.chh) → review+
(Assignee)

Comment 41

4 years ago
Created attachment 8351113 [details] [diff] [review]
Bug 947110 - Part1: RIL patch. B2G RIL: Add support for decoding FCP Template in BerTlvHelper. v5. r=yoshi.
Attachment #8350973 - Attachment is obsolete: true
(Assignee)

Comment 42

4 years ago
Created attachment 8351115 [details] [diff] [review]
Bug 947110 - Part2: Add test cases. B2G RIL: Add support for decoding FCP Template in BerTlvHelper. v2. r=yoshi.
Attachment #8350409 - Attachment is obsolete: true
(Assignee)

Comment 43

4 years ago
https://tbpl.mozilla.org/?tree=Try&rev=43bbc512ab3e
Keywords: checkin-needed
(Reporter)

Updated

4 years ago
Keywords: checkin-needed
(Assignee)

Comment 45

4 years ago
Created attachment 8351276 [details] [diff] [review]
[Patch for 1.2f] Bug 947110 - B2G RIL: Add support for decoding FCP Template in BerTlvHelper. r=yoshi.

https://tbpl.mozilla.org/?tree=Try&rev=c4150d341b64
https://hg.mozilla.org/mozilla-central/rev/1a9b9e00a24b
https://hg.mozilla.org/mozilla-central/rev/5f575f480383
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
(Assignee)

Comment 47

4 years ago
fugu DSDS use v1.3 branch. nominate 1.3? here.
blocking-b2g: fugu+ → 1.3?

Comment 48

4 years ago
Yes, it is a 1.3+ bug. Without this, the DSDS SIM functions doesn't work.
blocking-b2g: 1.3? → 1.3+

Comment 49

4 years ago
Shawn, you may need to provide a patch for 1.3 branch.
Flags: needinfo?(sku)
https://hg.mozilla.org/releases/mozilla-aurora/rev/3fac51aeed0e
https://hg.mozilla.org/releases/mozilla-aurora/rev/2f877b02b2f9
status-b2g-v1.3: --- → fixed
status-firefox27: --- → wontfix
status-firefox28: --- → fixed
status-firefox29: --- → fixed
Flags: needinfo?(sku)
Target Milestone: --- → 1.3 C2/1.4 S2(17jan)
You need to log in before you can comment on or make changes to this bug.