Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BER: fix decoding of extended fields in sequence #351

Merged
merged 3 commits into from
Oct 23, 2024

Conversation

Nicceboy
Copy link
Contributor

BER currently does not handle the decoding of extended fields correctly.

  • If there are no root components in sequence, field decoding is skipped
  • Value of the explicit/implict tag is not carried (macros are not generating it for decode_extension_addition and function does not have the parameter), so assert of tag value will fail later in

if let Some(tag) = tag {
BerDecodeErrorKind::assert_tag(tag, identifier.tag)?;
}

I am not sure if it is possible to fix without major changes or complete removal of the assert, so I just changed assert to apply for non-universal tags.

Maybe cargo.lock update fixes the main as well?

@XAMPPRocky
Copy link
Collaborator

I am not sure if it is possible to fix without major changes or complete removal of the assert, so I just changed assert to apply for non-universal tags.

I think the solution here is adding the tag for extension additions, right? It shouldn't be that major, it would be just adding a with_tag variant for the extension addition methods, and having all the default impl methods call that method with D::TAG. They already do this for it with constraints, so it wouldn't be that much to add it for tags.

@Nicceboy
Copy link
Contributor Author

Yeah, it might be better. I just noticed that decoding logic seems to rely on that assert and it cannot be modified easily.

@Nicceboy
Copy link
Contributor Author

It was one rabbit hole of potential issues on the same area if we want to cover all the cases properly.

I fixed also #272 at the same time and other improvements for explicit prefix related.
I added method decode_optional_with_explicit_prefix so that the internal decoding error is getting carried instead of mapping it to None, if decoding fails for other reason than type being absent.

However, I did not do the same for extension additions.

It might add quite bit of complexity in there, as it feels like we need to repeat most functions for extension additions than normal types currently have if we want to get the error information upstream. Some decode_optional_extension_addition_ function must return optional type so that we can unwrap to default value when default is present. However for actual optional types the calling must initiate from decode_optional_extension_addition_ and then on pro-macro level we would need to generate different functions for every variation whether type is optional or not, and we would also need variations with decode_optional_extension_addition_with_explicit_prefix_ to handle all the cases.

Maybe there is a better way? Or maybe that error information is not necessary.

@Nicceboy
Copy link
Contributor Author

Use of explicit prefix also drops the constraints for non-extended fields.

@XAMPPRocky
Copy link
Collaborator

I'll merge this for now, and let's address those issues in a followup.

Maybe there is a better way? Or maybe that error information is not necessary.

I think it is necessary because we want to provide the best error information possible. Maybe we can reduce the codeine complexity with a macro so we are repeating ourselves less, but I don't think we can get around that need.

@XAMPPRocky XAMPPRocky merged commit 021211b into librasn:main Oct 23, 2024
65 checks passed
@Nicceboy Nicceboy deleted the fix-ber-ext-decode branch October 23, 2024 15:56
This was referenced Oct 23, 2024
@github-actions github-actions bot mentioned this pull request Nov 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants