jam1garner / binrw

A Rust crate for helping parse and rebuild binary data using ✨macro magic✨.
https://binrw.rs
MIT License
545 stars 35 forks source link

TakeSeek + until_eof with enum pre asserts #217

Open MinusGix opened 11 months ago

MinusGix commented 11 months ago

Example:

use binrw::{io::TakeSeekExt, until_eof, BinRead};

fn main() {    let data = std::fs::read("/home/minus/test.bin").unwrap();
    let mut cursor = std::io::Cursor::new(&data);
    let data = Data::read(&mut cursor).unwrap();
    println!("Data: {:?}", data);
}

#[derive(Debug, BinRead)]
#[br(little)]
struct Data {
    v: u8,
    garbage: u8,
    #[br(map_stream = |s| s.take_seek(16), parse_with = |r, e, _:()| until_eof(r, e, (v,)))]
    data: Vec<Thing>,
}

#[derive(Debug, BinRead)]
#[br(little, import(v: u8))]
enum Thing {
    #[br(pre_assert(v == 0x00))]
    Thing0(Thing0),
    #[br(pre_assert(v == 0x01))]
    Thing1(Thing1),
}

#[derive(Debug, BinRead)]
#[br(little)]
struct Thing0 {
    #[br(dbg)]
    val0: u16,
}

#[derive(Debug, BinRead)]
#[br(little)]
pub struct Thing1 {
    #[br(dbg)]
    val1: u16,
}

And I just made a 16byte file to test it with.
(Side question: is there a better way to pass args to the inner part with until eof?)

This results in a panic.

thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: 
 ╺━━━━━━━━━━━━━━━━━━━━┅ Backtrace ┅━━━━━━━━━━━━━━━━━━━━╸

 0: Error: no variants matched at 0x10...
   ╭───────────────────────┄ Thing0 ┄────────────────────┄
   ┆
   ┆ 0: Error: failed to fill whole buffer
   ┆           While parsing field 'val0' in Thing0
   ┆     at lsf/src/main.rs:37
   ┆ 1: While parsing field 'self_0' in Thing::Thing0
   ┆     at lsf/src/main.rs:24
   ┆
   ╰─────────────────────────────────────────────────────┄
   ╭───────────────────────┄ Thing1 ┄────────────────────┄
   ┆
   ┆assertion failed: `v == 0x01` at 0x10
   ╰─────────────────────────────────────────────────────┄
    ...While parsing field 'data' in Data
     at lsf/src/main.rs:21

 ╺━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━╸

I think the issue is that is_eof looks into EnumErrors but doesn't test if the first is a pre-assert and the second a backtrace with an eof? I don't know if there's a more general proper way.
I reimplemented is_eof locally with the manual check for EnumAssert with [Assert, Backtrace that has an eof error]

csnover commented 11 months ago

Thanks for your report!

I am not sure of the best path forward here since it seems to be an ambiguous failure condition.

In your test case, it seems like EOF condition should be considered for any variant instead of all variants, but then this would be more likely to mask parser bugs in other cases. For example, it could be the case that Thing1 fails due to an error in the middle of parsing, then Thing0 does not have any magic or precondition because it is some fallback, tries to parse, and fails with EOF because it is the wrong variant entirely. In this case you would want the error of Thing1 to be reported but instead it would be silently treated as a successful EOF.

So it seems like there are a few different potential conditions someone might want for a successful EOF and to resolve this it might be necessary to expose a control for the author to decide what they want:

The third option I think would require adding new error variant to discriminate between a pre-assert variant failure and an assertion failure somewhere else (which is fine).