formats icon indicating copy to clipboard operation
formats copied to clipboard

der: Move `error.position` from Error to Reader

Open dishmaker opened this issue 1 year ago • 5 comments

Currently, it is annoying to track error.position in every function returning der::Error.

From another issue:

Attempting to even construct der::Error was confusing [...] It is also unclear to me whether the position that this method uses will be correct [...]

  • #1492

https://github.com/RustCrypto/formats/blob/9813667a89ff9cd91f99784ab13e9ae434fffdc2/der/src/error.rs#L23-L24

Actually, most functions don't return the position - example: https://github.com/RustCrypto/formats/blob/9813667a89ff9cd91f99784ab13e9ae434fffdc2/der/src/datetime.rs#L79-L83

What I suggest is something like serde + quick-xml:

let mut reader = SliceReader::new(EXAMPLE_MSG).unwrap();
let err = SomeStruct::decode(&mut reader);
assert!(err.is_err());

assert_eq!(reader.error_position(), 28);

https://docs.rs/quick-xml/latest/quick_xml/de/struct.Deserializer.html#method.get_ref

https://github.com/tafia/quick-xml/pull/743

dishmaker avatar Mar 14 '25 10:03 dishmaker

Position tracking was somewhat retrofitted. The current idea is to do reader.error(ErrorKind::DateTime) to populate the position.

An error_position may be more foolproof, though, and could replace Reader::error. Though isn't that just Reader::position anyway?

tarcieri avatar Jun 21 '25 13:06 tarcieri

In quick-xml crate, error_position gives a more fine-grained position than buffer_position.

Sometimes big chunks of XML are read, but actual error is somewhere else. In der I don't know if read_nested limits the error position propagation. I don't think it will be the same as Reader::position.

For example, when IA5String fails, error_position might point to the first incorrect byte.

https://github.com/RustCrypto/formats/blob/ed08fc043e7abb38a86fca10f114d15a86f4cb25/der/src/asn1/ia5_string.rs#L51-L53

dishmaker avatar Jun 21 '25 15:06 dishmaker

read_nested modifies the input length and keeps the current position relative to the original start of the message, FWIW

tarcieri avatar Jun 21 '25 15:06 tarcieri

I don't think it will be the same as Reader::position.

@dishmaker if it isn't the reader's current position, then how does it get set?

tarcieri avatar Jun 21 '25 16:06 tarcieri

It is set when error occurs

https://github.com/search?q=repo%3Atafia%2Fquick-xml%20last_error_offset&type=code

dishmaker avatar Jun 21 '25 20:06 dishmaker