ystero-dev / hampi

Rust ASN.1 Toolkit
Other
43 stars 18 forks source link

advance() method on PerCodecData #81

Closed nplrkn closed 1 year ago

nplrkn commented 1 year ago

Hello @gabhijit, it's been a while...

Here is a small enhancement that I think is helpful to deal with unrecognised extensions. For example, consider an NGAP ProtocolExtensionContainer.

ProtocolExtensionContainer {NGAP-PROTOCOL-EXTENSION : ExtensionSetParam} ::= 
    SEQUENCE (SIZE (1..maxProtocolExtensions)) OF
    ProtocolExtensionField {{ExtensionSetParam}}

ProtocolExtensionField {NGAP-PROTOCOL-EXTENSION : ExtensionSetParam} ::= SEQUENCE {
    id                  NGAP-PROTOCOL-EXTENSION.&id             ({ExtensionSetParam}),
    criticality         NGAP-PROTOCOL-EXTENSION.&criticality    ({ExtensionSetParam}{@id}),
    extensionValue      NGAP-PROTOCOL-EXTENSION.&Extension      ({ExtensionSetParam}{@id})
}

For each ProtocolExtensionField, you decode ID and criticality and length. If you recognize the ID you decode the extension value. If you don't recognize the ID (and it is non critical), you data.advance(length)? using this new API.

I couldn't think of a good way to do this using the existing API.

gabhijit commented 1 year ago

I just checked there is advance_maybe_err 'private' method (sorry I didn't look at the actual implementation of advance before), may be we can just make that as a pub method with some documentation - but I am not sure what is the user facing usefulness of such a thing.

nplrkn commented 1 year ago

I considered that but didn't like the "maybe err" bit of it and thought a cleaner API would be just plain advance().

Here's a snippet of code that should answer your question about how to know the length to advance.

       // Process the extension container
        let mut alternative_qos_para_set_list: Option<AlternativeQosParaSetList> = None;

        if optionals[2] {
            let num_ies =
                aper::decode::decode_length_determinent(data, Some(1), Some(65535), false)?;
            for _ in 0..num_ies {
                let (id, _ext) = aper::decode::decode_integer(data, Some(0), Some(65535), false)?;
                let _criticality = Criticality::aper_decode(data)?;
                let ie_length = aper::decode::decode_length_determinent(data, None, None, false)?;
                match id {
                    343 => {
                        alternative_qos_para_set_list =
                            Some(AlternativeQosParaSetList::aper_decode(data)?)
                    }
                    _ => {
                        data.advance(ie_length)?;
                    }
                }
            }
        }

The code in question is to handle one of these:

GBR-QosFlowInformation-ExtIEs F1AP-PROTOCOL-EXTENSION ::= {
    {   ID id-AlternativeQoSParaSetList CRITICALITY ignore  EXTENSION AlternativeQoSParaSetList PRESENCE optional } ,
    ...
}

(It would be better if this would check the Criticality before advancing to ignore the unknown IE... but that is a problem for another day.)

gabhijit commented 1 year ago

@nplrkn : The more I think about it - I think simply making advance_maybe_err API public is good enough (which allows the caller to ignore the fact that we can overrun the buffer). Adding a new API which simply calls this function with a default parameter can be achieved by a macro if typing maybe_err at many places is too much, but simply adding a wrapper (without offering an option to ignore) as a public API is unnecessary.

Would you mind simply making the advance_maybe_err a public API (you might also want to implement the macro inside the crate - that's fine) with some documentation?

nplrkn commented 1 year ago

Would you mind simply making the advance_maybe_err a public API (you might also want to implement the macro inside the crate - that's fine) with some documentation?

Done!