WebAssembly API: improve data section errors

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[JF Bastien]

As suggested in bug #165696.

[JF Bastien]

Created attachment 296820 [details]
patch

[Saam Barati]

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.

[JF Bastien]

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.

[Saam Barati]

(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.

[Saam Barati]

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.

[JF Bastien]

(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.

[JF Bastien]

Created attachment 297197 [details]
patch

Address comments, this is now ready for landing (and blocks bug #163919).

[Keith Miller]

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?

[JF Bastien]

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.

[WebKit Commit Bot]

Comment on attachment 297197 [details]
patch

Clearing flags on attachment: 297197

Committed r209874: <http://trac.webkit.org/changeset/209874>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.