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

Move contents of wasm.Module to an internal package and replace it with an interface #232

Closed
codefromthecrypt opened this issue Feb 14, 2022 · 1 comment

Comments

@codefromthecrypt
Copy link
Contributor

Currently, we are doing some re-validation because wasm.Module is not required to be created by a ModuleDecoder. For example, we double-check certain things like the start function pointing to a valid function type. These sorts of checks can and should be done when parsing/decoding as otherwise you don't know where the failure line/col was. Moreover, there are so many rules that it is a fools errand to repeat each one in instantiate: we'd end up with a lot of code that isn't necessary if decoded properly. Finally, exposing fields like the CodeSection to mutation allow some pretty severe bugs to be possible. For example, someone could by accident mess up a signature order, something that decoders already enforce.

I don't think 3rd party manual instantiation of wasm.Module is viable. The only sustainable way is via validating decoders which we already maintain. I believe that we should instead move the existing logic of wasm.Module internal, guaranteeing it is only created by our trusted decoders. We'd backfill wasm.Module as a struct or interface that has no exported mutable fields, but could be implemented by or contain an internal wasm.Module created by the decoder.

Doing this allows us to move all validation possible to do during decoding to the edge and guarantee we can count on that having happened prior to instantiate. This also removes a large bug source area of random or misunderstood contents in function code or constant declarations.

codefromthecrypt pushed a commit that referenced this issue Feb 14, 2022
Per #232, we will be unexporting fields only used during instantiate. Any
additional custom sections are unusable otherwise, so this skips them.
In doing so, this eliminates a length check bug, some TODO and some
logic that enforced strict name uniqueness even though that only rule
only applied to the name section.

Note: this doesn't restrict future use of custom sections, ex for DWARF,
just we don't buffer sections we don't use anymore.

See https://www.w3.org/TR/wasm-core-1/#custom-section%E2%91%A0

Signed-off-by: Adrian Cole <adrian@tetrate.io>
codefromthecrypt added a commit that referenced this issue Feb 14, 2022
Per #232, we will be unexporting fields only used during instantiate. Any
additional custom sections are unusable otherwise, so this skips them.
In doing so, this eliminates a length check bug, some TODO and some
logic that enforced strict name uniqueness even though that only rule
only applied to the name section.

Note: this doesn't restrict future use of custom sections, ex for DWARF,
just we don't buffer sections we don't use anymore.

See https://www.w3.org/TR/wasm-core-1/#custom-section%E2%91%A0

Signed-off-by: Adrian Cole <adrian@tetrate.io>
r8d8 pushed a commit to r8d8/wazero that referenced this issue Feb 14, 2022
Per tetratelabs#232, we will be unexporting fields only used during instantiate. Any
additional custom sections are unusable otherwise, so this skips them.
In doing so, this eliminates a length check bug, some TODO and some
logic that enforced strict name uniqueness even though that only rule
only applied to the name section.

Note: this doesn't restrict future use of custom sections, ex for DWARF,
just we don't buffer sections we don't use anymore.

See https://www.w3.org/TR/wasm-core-1/#custom-section%E2%91%A0

Signed-off-by: Adrian Cole <adrian@tetrate.io>
Signed-off-by: r8d8 <ckryvomaz@gmail.com>
@codefromthecrypt
Copy link
Contributor Author

this is done via api.Module

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

1 participant