APER issues

[sandeep2501]

Hi @mouse07410 I am using ASN1C to decode the APER messages and while testing the ASN1C I found out the below 3 issues which I fixed. I am sharing what I fixed in the code to get the decoder working

1) Decoding of large Enumerated Values by which I mean if the no. of elements in enum type lies between 255 and 66535 This issue is divided into 2 parts a. Calculation of Padding bits: In this Padding bits will be there, So those bits need to be discarded.

`Function NativeEnumerated_decode_aper

    if(ct->flags & APC_EXTENSIBLE) {
        int inext = per_get_few_bits(pd, 1);
        if(inext < 0) ASN__DECODE_STARVED;
        if(inext) ct = 0;
    }

    /*Changes Start */
    if(ct && ct->upper_bound >= 255)
    {
        padding = (8 - (pd->moved % 8)) % 8;
        ASN_DEBUG("For NativeEnumerated %s,offset= %d Padding bits = %d", td->name, pd->moved, padding);
        ASN_DEBUG("For NativeEnumerated %s, upper bound = %lld", td->name, ct->upper_bound);
        if(padding > 0)
            per_get_few_bits(pd, padding);
    }
    /*Changes end*/

` b. No. of Bits to be used for decoding. For this I put a hack in the code. This issue has to be fixed while code is getting generated. Currently enumeration takes the range_bits from the generated code. For large Enumerated Values, No. of bits to be decoded should be 16. I didn't know the proper fix, So I put the hack in generated code and change the value to 16 there.

2) Open Source ASN.1 decoder was not expecting Padding bits after Extension Fields, Because of which sometime decoder was failing.
`

in function, SEQUENCE_decode_aper
        for(edx = specs->ext_after + 1; edx < td->elements_count; edx++) {
        asn_TYPE_member_t *elm = &td->elements[edx];
        void *memb_ptr;     /* Pointer to the member */
        void **memb_ptr2;   /* Pointer to that pointer */
        int present;

        if(!IN_EXTENSION_GROUP(specs, edx)) {
            ASN_DEBUG("%d is not extension", edx);
            continue;
        }

        /* Fetch the pointer to this member */
        if(elm->flags & ATF_POINTER) {
            memb_ptr2 = (void **)((char *)st + elm->memb_offset);
        } else {
            memb_ptr = (void *)((char *)st + elm->memb_offset);
            memb_ptr2 = &memb_ptr;
        }

        /*Changes Start*/
        /*Get Padding*/
        int padding = 0;
        padding = (8 - (pd->moved % 8)) % 8;
        if(padding > 0)
        ASN_DEBUG("For element %s,offset= %d Padding bits = %d", td->name, pd->moved, padding);
            per_get_few_bits(pd, padding);
        /*Changes End*/

        present = per_get_few_bits(&epmd, 1);
        if(present <= 0) {
            if(present < 0) break;  /* No more extensions */
            continue;
        }

        ASN_DEBUG("Decoding member %s in %s %p", elm->name, td->name, *memb_ptr2);
        rv = uper_open_type_get(opt_codec_ctx, elm->type,
            elm->per_constraints, memb_ptr2, pd);
        if(rv.code != RC_OK) {
            FREEMEM(epres);
            return rv;
        }
        }

`

3) For Integer type decoding, Decoder was calling “UPER” function, instead of “APER” function, because of which sometimes values were not coming correctly

`in function, SEQUENCE_decode_aper()

        rv = uper_open_type_get(opt_codec_ctx, elm->type,
            elm->per_constraints, memb_ptr2, pd);

asn_dec_rval_t
uper_open_type_get(asn_codec_ctx_t *ctx, asn_TYPE_descriptor_t *td,
    asn_per_constraints_t *constraints, void **sptr, asn_per_data_t *pd) {

    return uper_open_type_get_simple(ctx, td, constraints, sptr, pd);
}

uper_open_type_get_simple()
and this function calls the 
    rv = td->op->uper_decoder(ctx, td, constraints, sptr, &spd`

As it is calling the uper_decoder function to decode the APER message, it is causing the issues and values are getting mismatch So the code needs to be changed and for APER decoder it is better to have 2 different functions

`aper_open_type_get 
aper_open_type_get_simple() and this function calls the
    rv = td->op->aper_decoder(ctx, td, constraints, sptr, &spd);

and 
in function, SEQUENCE_decode_aper()     
        rv = aper_open_type_get(opt_codec_ctx, elm->type,
            elm->per_constraints, memb_ptr2, pd);`

I have gone through the Python ASN.1 APER decoder https://github.com/mitshell/libmich and it also suggested the same fix. All the changes are marked in /Changes start / /Changes End/

[mouse07410]

@sandeep2501 after looking through the code, I'm sorry to say that I can't make use of your report - too much is unspecified. Like I said, ideally I'd like to see a PR against this repository. At the very worst I need the exact patches (file name, line numbers, code that is being replaced and the code that is replacing it). Without that I just can't track it.

Also, I'm not sure you've been using this repo, as the patches to not seem to match this code exactly.

Oh, and probably the encoders need a similar fix too?

@velichkov, can you make sense of the above report?

Update I think I've taken care of the three issues listed (again, thanks to @sandeep2501 for the proposed fixes). Please confirm that (a) it fixed the reported problems, and (b) it did not break anything else.

[mouse07410]

@sandeep2501 I'd really appreciate if you could test the current master and let me know whether the problems you reported in this issue have been fixed.

If I don't hear from you before this Friday, I'll assume that everything's fine (and you'd complain if it weren't), and close this issue.

[sandeep2501]

Hi @mouse07410

Thanks for taking my changes and My apologies for the late reply. I was on extended holidays.

I have downloaded the latest code and found 2 issues in the file constr_SEQUENCE.c related to these 2 issues

• Accounting for padding bits (APER vs UPER) in SEQUENCE decoding;
• Calling APER decoder rather than UPER decoder when dealing with APER.

I have fixed those issues and attached the same file as constr_SEQUENCE.txt

constr_SEQUENCE.txt 35);

For issue - Decoding of large Enumerated values (number of enum types between 255 and 66535) This issue is divided into 2 parts a. Calculation of Padding bits: In this Padding bits will be there, So those bits need to be discarded. --- Fixed and Working b. No. of Bits to be used for decoding. --- Not Fixed For this I still have to put a hack in the code. This issue has to be fixed when source code is generated by asn1c compiler from the ASN Grammar. Currently enumeration takes the range_bits from the generated code. For large Enumerated Values, No. of bits to be decoded should be 16. I didn't know the proper fix, So I put the hack in generated code and change the value to 16 there.

[mouse07410]

@sandeep2501 thank you for checking.

For large Enumerated Values, No. of bits to be decoded should be 16.

Should it be always 16, or just larger than 8? I understand you need it to be 16, but what's the generic case?

@velichkov, any comment here?

[vlm]

I am rebasing your master into vlm/asn1c. Would you please hold on making changes for a few more hours, trying to avoid a big mess with merges/rebases.

If you see my email in my profile, I'd appreciate a shout out, to coordinate.

[mouse07410]

Sure, no problem. 48-hour freeze on my changes. Longer if needed.

[vlm]

Thank you!

[mouse07410]

@vlm could you clarify please. Am I correct to assume that your master now incorporates all the additions and fixes that my master has? I.e., you merged everything from mine back into yours? Thanks!

[vlm]

Not yet, but I am progressing in this direction. Still ongoing, need a few more hours.

[mouse07410]

I see. No problem. I need to figure how to address the two issues currently outstanding in my fork.

BTW, you might want to give them a thought... I'd love to hear your opinion (or maybe even proposed solutions).

[vlm]

@mouse07410 , sorry to bring bad news, at least for now. I have tried to rebase on top of your code and found numerous problem with APER support, 64-bit support and others.

To the point that in some cases I don't see how the code could possible work properly (for example, in APER alignment cases in _nsnnwn handling).

We'll have to approach it in some other way.

Here are my thoughts, for starters:

1. It would help you could rebase your master on top of my master, that will significantly reduce the surface area of further attempts to recombine the chances.

2. The 64-bit support should be based on int64_t/uint64_t types in generated structures (conditionally generated!) and intmax_t/uintmax_t in constraints handling code. The use of "long long" in both places is confusing and sometimes wrong. For example, if the code generator generates the long types for native integers, then subsequently we can't parse them in NativeInteger_decode_{u,a}per as long longs. How this code works is beyond me, it shouldn't. Most certainly signifies a lack of test coverage.

3. The 64-bit support in generated structures should be constraints-based and therefore optional. In the future, the ASN.1 type Foo { small INTEGER, bigger INTEGER(0..5000000000), biggest INTEGER(MIN..MAX) } should generate long,int64_t, INTEGER_t, for the integer fields, respectively.

4. A configure.ac's support for C89 for skeletons is intentional. asn1c is used in embedded code, where C99 is not always available. Changing to C99 is premature.

5. It is polite in FOSS to let the primary author/maintainer to change the software version numbering (in configure.ac), or more properly fork the project by changing the project name.

If you're up to it, it might make sense to have a call to discuss the way to resolve the merge. Please send me an email to exchange contacts and schedule.

[mouse07410]

It would help you could rebase your master on top of my master, that will significantly reduce the surface area of further attempts to recombine the chances.

I'll consider that.

The 64-bit support should be based on int64_t/uint64_t types in generated structures (conditionally generated!) and intmax_t/uintmax_t in constraints handling code. The use of long long in both places is confusing and sometimes wrong. For example, if the code generator generates the long types for native integers, then subsequently we can't parse them in NativeInteger_decode_{u,a} per as long longs. How this code works is beyond me, it shouldn't. Most certainly signifies a lack of test coverage.

A point. I'll consider reverting this. It was caused by the attempt to make constraints work with larger limits - especially in non-INTEGER items (like OCTET STRING SIZE(5..500000000000)), which did not work with the original code. So in order to move to the correct types we need to make sure the correct constraints are being generated - including the cases when the values of the constraints do not fit int64_t/uint64_t. Also, see below.

The 64-bit support in generated structures should be constraints-based and therefore optional. In the future, the ASN.1 type Foo { small INTEGER, bigger INTEGER(0..5000000000), biggest INTEGER(MIN..MAX) } should generate long,int64_t, INTEGER_t for the integer fields, respectively.

Yeah... Unfortunately, it might have worked for INTEGER types - but it did not work for size constraints, as some of the reported issues indicated. The implemented modifications addressed those issues - though I agree that there could be a better way of doing that. For example, I'd love the constraints for Foo { len INTEGER(0..999000000000), OCTET STRING SIZE(0..999000000000) } work. Currently for APER they don't.

A configure.ac's support for C89 for skeletons is intentional. asn1c is used in embedded code, where C99 is not always available. Changing to C99 is premature.

This should be a matter of the compiler, right? And in 2017 probably any compiler you could possibly find would support C99 at the very least. Having said that, we can probably revert back to C89 if there's a compelling reason (like knowledge of a compiler used today that is supports ANSI C only up to C89, but nothing newer).

It is polite in FOSS to let the primary author/maintainer to change the software version numbering (in configure.ac), or more properly fork the project by changing the project name.

A point. We figured that the amount of extensions/enhancements added to this fork justified bumping the version number. Not sure what to do about that...

If you're up to it, it might make sense to have a call to discuss the way to resolve the merge. Please send me an email to exchange contacts and schedule.

Sure. Could you please send me email once again, and mention your time zone please.

@velichkov, you contributed much - would you like to participate? Probably @brchiu too?