rusticata / der-parser

BER/DER parser written in pure Rust. Fast, zero-copy, safe.
Apache License 2.0
84 stars 29 forks source link

`BerObjectHeader.structured` should be a `bool` #56

Closed lilyball closed 2 years ago

lilyball commented 2 years ago

Not a Contribution

This field is a u8 and yet it can only hold the values 0 and 1. This is confusing and leads to somewhat odd behavior such as serializing a header looks at the lsb, so hdr.structured = 2 is serialized as 0. It also means you can have a header that is neither primitive nor constructed (as both accessors do an equality test on the flag).

Instead this should be a bool. Anyone who wants to recover the 0u8/1u8 value can just cast it to u8 (hdr.structured as u8), so that actually simplifies the encoding, and it simplifies the decoding too because right now it's a conditional

let b = if i[0] & 0b0010_0000 != 0 { 1 } else { 0 };

and with this change it would just be let b = i[0] & 0b0010_0000 != 0.

Code that prints this today such as .as_pretty() currently prints e.g. s:0 or s:1, so if you wanted to preserve that you'd need to add a cast to the printing, though it might actually be clearer to just let it print as s:false or s:true anyway.

Side note

I don't know why this is called structured, that word doesn't appear anywhere in X.690, I would have expected it to be called constructed instead.

chifflier commented 2 years ago

Hi, I totally agree, this was an implementation detail that should not even has been exposed, and the naming was not following the specifications. Since I'm currently refactoring some code [*] in a dev branch, I'm adding this change for next version 7.0

[*] BerObjectHeader fields will not be accessible directly anymore, but only through getters/setters. This makes the code cleaner and much more resilient to changes in the header content.

Thanks!