Refactor BrTable

[Robbepop]

Associated issue: https://github.com/bytecodealliance/wasmparser/issues/174

This PR slightly refactors the BrTable abstraction of the Wasm br_table.

• Adds a new field default_offset to BrTable
• Removes the IntoIterator impl for &'a BrTable
• Adds targets and targets_and_default iterator methods to BrTable
• Improves correctness of iteration of BrTableTargets (the iterator type)

Let me explain the changes.

• Since the default offset is a required field in the br_table we simply add it as a field instead of merging it with the other non-default target offsets.
• I had the feeling that the IntoIterator impl was confusing since it was not clear what exactly is being iterated over (with or without default offset?). So the new interface provides two different methods for those two use cases.
• The last thing is a bit more involved. In essence this changes the very simple iterator next implementation from

fn next(&mut self) -> Option<Self::Item> {
    self.reader.read_var_u32().ok()
}

to

fn next(&mut self) -> Option<Self::Item> {
    if self.start == self.end {
        return None;
    }
    let next = self
        .reader
        .read_var_u32()
        // We can expect this not to fail since we can only contruct a
        // `BrTableTargets` iterator from a proper `BrTable` instance
        // which checks the underlying buffer for being a valid `br_table`
        // entries buffer.
        .expect("expected a `br_table` target var_u32");
    self.start += 1;
    Some(next)
}

While this might look like a really bad change it actually makes the whole iteration more correct from what I can tell. Please prove me wrong on this because I am actually still a bit confused about what I found: The empty-br-table test broke after this change - note, that every other test of the whole test suite continued to work under this change.

From looking at the test snippet it seems to me that the empty-br-table.wasm is actually syntactically incorrect, not semantically so it should fail during parsing, not during validation but I might be wrong here.

Why is the old and super fancy small interface working? Tbh I cannot tell for sure but it obviously is more robust against too small buffers. So the iterator will silently yield less elements than there are which is what is incorrect about it imo. But I am still a bit confused because the create_br_table function should in theory sort out those malicious cases before they can even exist. Need to dig in deeper.

Please tell me if I am completely wrong about my assumptions so far. @yurydelendik

[yurydelendik]

From looking at the test snippet it seems to me that the empty-br-table.wasm is actually syntactically incorrect, not semantically so it should fail during parsing, not during validation but I might be wrong here.

The test is correct. There is an issue with skip_var_32() that is skipping a number and not checking if they are in range -- that's why it happens during validation. The skip_var_xx logic needs to be fixed.

[yurydelendik]

wonder why targets_len renamed to len_targets?

[Robbepop]

tbh no good reasons. i just personally liked the name better during refactoring. will undo. ;)

[yurydelendik]

I think we can still keep IntoIterator for BrTable: type IntoIter = Chain<BrTableTargets, Once<u32>>?

[Robbepop]

We can do that. I just removed it because it is not directly clear what exactly BrTable is iterating over given IntoIterator whereas for types like Vec it is totally clear. So I thought maybe methods are a superior fit in this case and therefore replaced the IntoIterator impl with two methods for each explicit use case.

[yurydelendik]

We can keep it after skip_var_32 is fixed, though it shall never be raised during testing.

[Robbepop]

I wasn't sure if skip_var_32 is actually broken but good to know. :) I think this implementation is actually more robust since it pointed out this potential bug and it shouldn't be a performance hit at all. But we should make sure that this is actually true. Fortunately wasmparser has a decent benchmark suite.

[yurydelendik]

default_offset -> default_target

[Robbepop]

ok 👍

[Robbepop]

From looking at the test snippet it seems to me that the empty-br-table.wasm is actually syntactically incorrect, not semantically so it should fail during parsing, not during validation but I might be wrong here.

The test is correct. There is an issue with skip_var_32() that is skipping a number and not checking if they are in range -- that's why it happens during validation. The skip_var_xx logic needs to be fixed.

Thank you for letting me know. I was really confused. 😅

[Robbepop]

Closed in favor of #216.