librasn / rasn

A Safe #[no_std] ASN.1 Codec Framework
Other
183 stars 43 forks source link

OER/UPER/APER roundtrip fails with untagged sequence fields and optional types #271

Open Nicceboy opened 2 weeks ago

Nicceboy commented 2 weeks ago

OER/UPER/APER fails to encode Sequence with untagged and optional fields

use rasn::{prelude::*, uper};

#[derive(AsnType, Decode, Encode, Clone, Debug, PartialEq, Eq)]
pub struct SequenceOptionals {
    // #[rasn(tag(explicit(0)))]
    pub it: Integer,
    // #[rasn(tag(explicit(1)))]
    pub is: Option<OctetString>,
    // #[rasn(tag(explicit(2)))]
    pub late: Option<Integer>,
}
fn main() {
    let test_seq = SequenceOptionals {
        it: 42.into(),
        is: None,
        late: None,
    };
    let encoded = uper::encode(&test_seq).unwrap();
    dbg!(&encoded);
    let decoded: SequenceOptionals = uper::decode(&encoded).unwrap();
    dbg!(&decoded);
    assert_eq!(test_seq, decoded);
}

Output:

[testing/src/main.rs:19:5] &encoded = [
    0,
]
thread 'main' panicked at testing/src/main.rs:20:61:
called `Result::unwrap()` on an `Err` value: DecodeError { kind: FieldError { name: "SequenceOptionals.it", nested: DecodeError { kind: Incomplete { needed: Size(2) }, codec: Uper } }, codec: Uper }
stack backtrace:
   0: rust_begin_unwind
             at /rustc/ada5e2c7b5427a591e30baeeee2698a5eb6db0bd/library/std/src/panicking.rs:652:5
   1: core::panicking::panic_fmt
             at /rustc/ada5e2c7b5427a591e30baeeee2698a5eb6db0bd/library/core/src/panicking.rs:72:14
   2: core::result::unwrap_failed
             at /rustc/ada5e2c7b5427a591e30baeeee2698a5eb6db0bd/library/core/src/result.rs:1679:5
   3: core::result::Result<T,E>::unwrap
             at /rustc/ada5e2c7b5427a591e30baeeee2698a5eb6db0bd/library/core/src/result.rs:1102:23
   4: testing::main
             at ./src/main.rs:20:38
   5: core::ops::function::FnOnce::call_once
             at /rustc/ada5e2c7b5427a591e30baeeee2698a5eb6db0bd/library/core/src/ops/function.rs:250:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace
Nicceboy commented 1 week ago

The issue here is duplicate tags which get overwritten when creating field_bitfield mapping and this results to wrong preamble bits.

Nicceboy commented 1 week ago

@XAMPPRocky what could be the best approach here? I guess the above is valid ASN.1 syntax, so we should be able to encode and decode it.

I was thinking of adding field_index field for Tag type, so in untagged sequence/set we can distinct duplicate tags in sequence in encoder and decoder logic.

XAMPPRocky commented 1 week ago

My initial question is, why not fix the field_bitfield to allow this to work instead of adding a new field?

Nicceboy commented 1 week ago

My initial question is, why not fix the field_bitfield to allow this to work instead of adding a new field?

I tried to do that at first, but it seems that there isn't enough information available to detect duplicate tags from different fields.

Whenever something is encoded,

  pub fn set_bit(&mut self, tag: Tag, bit: bool) {
        self.field_bitfield.entry(tag).and_modify(|(_, b)| *b = bit);

is called, and only Tag type is accessible from encoding methods. Currently, it does not carry enough information.

XAMPPRocky commented 1 week ago

What if the map was (Tag, usize)? As in it encodes the tag and the position?

Nicceboy commented 1 week ago

This is the compiled macro for this case

#[allow(clippy::mutable_key_type)]
impl rasn::Encode for SequenceOptionals {
    fn encode_with_tag_and_constraints<'constraints, EN: rasn::Encoder>(
        &self,
        encoder: &mut EN,
        tag: rasn::Tag,
        constraints: rasn::types::Constraints<'constraints>,
    ) -> core::result::Result<(), EN::Error> {
        #[allow(unused)]
        let __rasn_field_it = &self.it;
        #[allow(unused)]
        let __rasn_field_is = &self.is;
        #[allow(unused)]
        let __rasn_field_late = &self.late;
        encoder
            .encode_sequence::<Self, _>(tag, |encoder| {
                self.it.encode(encoder)?;
                self.is.encode(encoder)?;
                self.late.encode(encoder)?;
                Ok(())
            })
            .map(drop)
    }
}

Maybe sequence encoder could actually carry current index based on the order there and then (Tag, usize) is possible without changing trait signatures.

XAMPPRocky commented 1 week ago

So another part of this, is do we need to fix this for SET as well, that would prevent some ways in which this is handled.

Nicceboy commented 1 week ago

I guess I was able to fix it and nothing else exploded. In SET, unique tags seem to be mandatory so it was not an issue.

There is OER fix first, if that seems ok, in #279