Bug 165733 - WebAssembly API: improve data section errors
Summary: WebAssembly API: improve data section errors
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: JF Bastien
URL:
Keywords:
Depends on: 165696
Blocks: 163919
  Show dependency treegraph
 
Reported: 2016-12-10 13:38 PST by JF Bastien
Modified: 2016-12-15 14:09 PST (History)
6 users (show)

See Also:


Attachments
patch (10.76 KB, patch)
2016-12-10 13:41 PST, JF Bastien
sbarati: review-
sbarati: commit-queue-
Details | Formatted Diff | Diff
patch (14.10 KB, patch)
2016-12-15 10:17 PST, JF Bastien
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description JF Bastien 2016-12-10 13:38:57 PST
As suggested in bug #165696.
Comment 1 JF Bastien 2016-12-10 13:41:56 PST
Created attachment 296820 [details]
patch
Comment 2 Saam Barati 2016-12-12 17:14:48 PST
Comment on attachment 296820 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=296820&action=review

> JSTests/wasm/js-api/test_Data.js:58
> +(function DataSectionWithoutMemory() {
> +    const builder = (new Builder())
> +        .Type().End()
> +        .Data()
> +          .Segment([0xff]).Offset(0).End()
> +        .End();
> +    const bin = builder.WebAssembly().get();
> +    const module = new WebAssembly.Module(bin);
> +    const memory = new WebAssembly.Memory(memoryDescription);
> +    assert.throws(() => new WebAssembly.Instance(module, { imp: { memory: memory } }), RangeError, `cannot initialize non-zero data segments when no memory is present`);
> +})();

This sure looks like it should be a validation error.

> JSTests/wasm/js-api/test_Data.js:70
> +(function EmptyDataSectionWithoutMemory() {
> +    const builder = (new Builder())
> +        .Type().End()
> +        .Data()
> +          .Segment([]).Offset(0).End()
> +        .End();
> +    const bin = builder.WebAssembly().get();
> +    const module = new WebAssembly.Module(bin);
> +    const memory = new WebAssembly.Memory(memoryDescription);
> +    const instance = new WebAssembly.Instance(module, { imp: { memory: memory } });
> +})();

Ditto

> Source/JavaScriptCore/wasm/js/WebAssemblyModuleRecord.cpp:218
> +    } else {
> +        for (auto& segment : data) {
> +            if (UNLIKELY(segment->sizeInBytes))
> +                return throwException(state, scope, createRangeError(state, makeString(ASCIILiteral("cannot initialize non-zero data segments when no memory is present"))));
> +        }
>      }

This seems like it should be caught by validation.
Comment 3 JF Bastien 2016-12-13 11:44:11 PST
Andreas doesn't seem to want to move things to validation time:
https://github.com/WebAssembly/design/issues/897#issuecomment-266505611

I proposed some clarifications here:
https://github.com/WebAssembly/design/pull/902

For now I'd just go with what I implemented, and revisit when we reach spec consensus.
Comment 4 Saam Barati 2016-12-13 19:18:11 PST
(In reply to comment #3)
> Andreas doesn't seem to want to move things to validation time:
> https://github.com/WebAssembly/design/issues/897#issuecomment-266505611
> 
> I proposed some clarifications here:
> https://github.com/WebAssembly/design/pull/902
> 
> For now I'd just go with what I implemented, and revisit when we reach spec
> consensus.

Using a Data when no memory is present is definitely a validation error.
Comment 5 Saam Barati 2016-12-13 19:18:40 PST
Comment on attachment 296820 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=296820&action=review

> Source/JavaScriptCore/wasm/js/WebAssemblyModuleRecord.cpp:202
> +    if (memory) {

This should turn into an assert.
Comment 6 JF Bastien 2016-12-14 10:25:00 PST
(In reply to comment #4)
> (In reply to comment #3)
> > Andreas doesn't seem to want to move things to validation time:
> > https://github.com/WebAssembly/design/issues/897#issuecomment-266505611
> > 
> > I proposed some clarifications here:
> > https://github.com/WebAssembly/design/pull/902
> > 
> > For now I'd just go with what I implemented, and revisit when we reach spec
> > consensus.
> 
> Using a Data when no memory is present is definitely a validation error.

Is it? I can make it so, but we don't do that at the moment and I'm not sure the spec says so.
Comment 7 JF Bastien 2016-12-15 10:17:04 PST
Created attachment 297197 [details]
patch

Address comments, this is now ready for landing (and blocks bug #163919).
Comment 8 Keith Miller 2016-12-15 11:13:56 PST
Comment on attachment 297197 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=297197&action=review

r=me with comments.

> JSTests/wasm/js-api/element-data.js:8
> +(function ElementBeforeData() {

What's the advantage of wrapping this in a function?

> Source/JavaScriptCore/wasm/js/WebAssemblyModuleRecord.cpp:197
> +template <typename Scope, typename N, typename ...Args>

Why is N needed it looks like you only use it with size_t anyway?
Comment 9 JF Bastien 2016-12-15 13:43:58 PST
Comment on attachment 297197 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=297197&action=review

>> JSTests/wasm/js-api/element-data.js:8
>> +(function ElementBeforeData() {
> 
> What's the advantage of wrapping this in a function?

I've been doing that for all the tests: it appears in the stack traces if it fails, and it's simpler than having a comment IMO.

>> Source/JavaScriptCore/wasm/js/WebAssemblyModuleRecord.cpp:197
>> +template <typename Scope, typename N, typename ...Args>
> 
> Why is N needed it looks like you only use it with size_t anyway?

It just means we don't have to change the type if it changes at the call site (and we don't get inadvertent coercion). It also catch cases where the 3 parameters don't have the same type.
Comment 10 WebKit Commit Bot 2016-12-15 14:09:19 PST
Comment on attachment 297197 [details]
patch

Clearing flags on attachment: 297197

Committed r209874: <http://trac.webkit.org/changeset/209874>
Comment 11 WebKit Commit Bot 2016-12-15 14:09:23 PST
All reviewed patches have been landed.  Closing bug.