Binary reader options and intended use cases

[SoniEx2]

Currently, we have the following binary reader options:

  Features features;
  Stream* log_stream = nullptr;
  bool read_debug_names = false;
  bool stop_on_first_error = true;
  bool fail_on_custom_section_error = true;
  bool skip_function_bodies = false;

Now, features and log_stream are... fairly universal, we wanna be able to select which post-MVP features to enable and disable and oftentimes also enable verbose logging.

However, it's another story with these other options. Most of these aren't available when running the provided tools, but API users can bring their own ReadBinaryOptions. In theory, this leads to use-cases like:

• Emitting IR without function bodies. (options skip_function_bodies can not work with ReadBinaryIr #2534 / optimize in for low memory device #2535)
• Interpreting a module without function bodies.
• Interpreting a module with errors.
• Emitting IR with errors (this may well be useful to someone, but we don't officially support it).

among other, potentially questionable ideas. However, we would like to highlight one issue in particular, and it's that our interpreter is designed to be... pretty fast. It's not trying to be the fastest thing in the world, but it does little run-time checking, instead relying entirely on the SharedValidator to do its job. As such, it's a dangerous idea to run (maliciously crafted) broken modules in the interpreter.

Would it make sense to more strictly enforce that many of these options are purely for use with the objdump reader? Ofc, we can still support use-cases like those in #2534 / #2535, but we (Soni) don't believe the API should look like this.

[sbc100]

Regarding stop_on_first_error, I don't think the idea was ever to allow the binary reader to have errors and then continue to on to interpret the module. I think the idea is that if there was 1 (or more) errors the reader would still fail, and the interpreter would not actually start. The only difference is how many errors are reported when reading the binary: at most 1 or more than one.

As a corollary to this I don't think the interpreter should have any runtime checks for things that are validation errors (perhaps a asserts are OK, but not real runtime checks). If you know of any such checks I think we should remove them or make them into asserts.

[SoniEx2]

Yes, agree. However, there's another issue:

Fuzzing issues like https://issues.oss-fuzz.com/issues/378159149 seem to be related with continuing to build interpreter state for invalid modules after finding out they're invalid. These issues cannot be triggered with a regular wasm-interp. To us, these don't really seem worth chasing, so it would be kinda nice to eliminate them.

wabt itself never uses stop_on_first_error = false; with the interpreter, and personally we don't think we should be using it for fuzzing either. We'd prefer to switch to an API like the one we propose in #2541 , so as to eliminate the possibility.