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

TakeSeek + until_eof with enum pre asserts #217

Open
MinusGix opened this issue Aug 7, 2023 · 1 comment
Open

TakeSeek + until_eof with enum pre asserts #217

MinusGix opened this issue Aug 7, 2023 · 1 comment
Labels
confusing-api Oops! Confusion occurred

Comments

@MinusGix
Copy link

MinusGix commented Aug 7, 2023

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
Copy link
Collaborator

csnover commented Aug 9, 2023

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:

  • All variants must EOF at the first byte
  • All variants must EOF (current behaviour)
  • All variants must either EOF or pre-assertion failure
  • Any variant can EOF
  • Something else?

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confusing-api Oops! Confusion occurred
Projects
None yet
Development

No branches or pull requests

2 participants