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

Limits #2522

Open
SoniEx2 opened this issue Dec 26, 2024 · 6 comments
Open

Limits #2522

SoniEx2 opened this issue Dec 26, 2024 · 6 comments

Comments

@SoniEx2
Copy link
Contributor

SoniEx2 commented Dec 26, 2024

wabt currently applies no limits anywhere ever.

we would like to introduce a limits.json file that can be passed in the CLI. (we think a json would be a better fit than putting it all in the CLI.) like --limits limits.json.

the hard part is enforcing the limits tbh... but anyway.

@keithw
Copy link
Member

keithw commented Dec 26, 2024

Hmm, I'm not sure I totally follow. The BinaryReaderIR does impose limits like kMaxNestingDepth, kMaxFunctionLocals, kMaxFunctionParams, and kMaxFunctionResults, which match the values in V8 (https://chromium.googlesource.com/v8/v8/+/refs/heads/main/src/wasm/wasm-limits.h) and hopefully the other big engines.

You're saying WABT should limit other kinds of things? And/or the values should be configurable at runtime? I think prevailing practice so far has just been to sort of handshake agree on reasonable values for the various "implementation limitations" (https://webassembly.github.io/spec/core/appendix/implementation.html) across major implementations.

@SoniEx2
Copy link
Contributor Author

SoniEx2 commented Dec 27, 2024

right, so

static constexpr size_t kMaxNestingDepth = 16384; // max depth of label stack
static constexpr size_t kMaxFunctionLocals = 50000; // matches V8
static constexpr size_t kMaxFunctionParams = 1000; // matches V8
static constexpr size_t kMaxFunctionResults = 1000; // matches V8

... we would never have found these if you hadn't mentioned them. (... why aren't they in a header??? or, alternatively, configurable by the user.)

it is worth noting that these only apply to the IR reader, they do not apply to text format or - more importantly - to interpreter.

@sbc100
Copy link
Member

sbc100 commented Dec 27, 2024

Making them configurable seems like maybe unnecessary. Putting them in a header seems pretty reasonable. Maybe just in the existing header for the binary file stuff.

@SoniEx2
Copy link
Contributor Author

SoniEx2 commented Dec 27, 2024

we feel like making them configurable might be useful given the whole wabt is primarily a developer tool (tho having defaults probably wouldn't hurt), but we mean... maybe. we also need to get these to apply to the interpreter and the text format and whatnot tho.

@sbc100
Copy link
Member

sbc100 commented Dec 28, 2024

Hopefully only the parsers / validators need to enforce these limits. The interpreter can assume all its inputs have been validated. There may be some exceptions but I can't think of any

@SoniEx2
Copy link
Contributor Author

SoniEx2 commented Dec 28, 2024

hmm... we'll think about it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants