Bug 165696 – webassembly: implement data section

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[JF Bastien]

https://github.com/WebAssembly/design/blob/master/BinaryEncoding.md#data-section

[JF Bastien]

Created attachment 296727 [details]
sample test

Sample test. PLAT and tell me if you don't like the API.

[JF Bastien]

Created attachment 296736 [details]
Builder and its test

[JF Bastien]

Created attachment 296766 [details]
more tests, some C++, missing ::evaluate after Instance is created

[JF Bastien]

rdar://problem/29599594

[JF Bastien]

Created attachment 296776 [details]
patch for review

[WebKit Commit Bot]

Attachment 296776 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/ChangeLog:16:  Please consider whether the use of security-sensitive phrasing could help someone exploit WebKit: malicious  [changelog/unwantedsecurityterms] [3]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[JF Bastien]

Created attachment 296777 [details]
patch

Fix include case.

[Keith Miller]

Comment on attachment 296777 [details]
patch

r=me.

[WebKit Commit Bot]

Comment on attachment 296777 [details]
patch

Clearing flags on attachment: 296777

Committed r209651: <http://trac.webkit.org/changeset/209651>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Saam Barati]

Comment on attachment 296777 [details]
patch

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

> Source/JavaScriptCore/wasm/js/WebAssemblyModuleRecord.cpp:201
> +                        || segment->offset > sizeInBytes

I don't think this check adds anything and it also feels wrong b/c it looks suspiciously off by 1.
However, that said, the below check looks OK to me, I couldn't poke holes in it.

[JF Bastien]

Comment on attachment 296777 [details]
patch

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

>> Source/JavaScriptCore/wasm/js/WebAssemblyModuleRecord.cpp:201
>> +                        || segment->offset > sizeInBytes
> 
> I don't think this check adds anything and it also feels wrong b/c it looks suspiciously off by 1.
> However, that said, the below check looks OK to me, I couldn't poke holes in it.

My thinking was: this is the check for a segment which is totally off the end, the below one is for a segment which is only partly off the end. I'm not sure what the spec says about the second case (do we perform the valid writes and then error out? That's what wasm code guarantees). I'd also like both checks to have their own error message later, when we improve messages and testing.

I don't think it's off by one: say a memory is sized at 1024, and the offset is 1024 (and write one byte), then that's one off the end. An offset of 1023 would be fine. Or do I have that wrong?

[Saam Barati]

(In reply to comment #12)
> Comment on attachment 296777 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=296777&action=review
> 
> >> Source/JavaScriptCore/wasm/js/WebAssemblyModuleRecord.cpp:201
> >> +                        || segment->offset > sizeInBytes
> > 
> > I don't think this check adds anything and it also feels wrong b/c it looks suspiciously off by 1.
> > However, that said, the below check looks OK to me, I couldn't poke holes in it.
> 
> My thinking was: this is the check for a segment which is totally off the
> end, the below one is for a segment which is only partly off the end. I'm
> not sure what the spec says about the second case (do we perform the valid
> writes and then error out? That's what wasm code guarantees). I'd also like
> both checks to have their own error message later, when we improve messages
> and testing.
> 
> I don't think it's off by one: say a memory is sized at 1024, and the offset
> is 1024 (and write one byte), then that's one off the end. An offset of 1023
> would be fine. Or do I have that wrong?
I think you're wrong. I'm just thinking about this check the way I think about all other array memory access. In your example:
memory = [0, 1023]
sizeInBytes = 1024 
offset = 1024
We *could* still do the write with this check. But as I said, I think the below check should handle all cases (including this).

[JF Bastien]

(In reply to comment #13)
> (In reply to comment #12)
> > Comment on attachment 296777 [details]
> > patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=296777&action=review
> > 
> > >> Source/JavaScriptCore/wasm/js/WebAssemblyModuleRecord.cpp:201
> > >> +                        || segment->offset > sizeInBytes
> > > 
> > > I don't think this check adds anything and it also feels wrong b/c it looks suspiciously off by 1.
> > > However, that said, the below check looks OK to me, I couldn't poke holes in it.
> > 
> > My thinking was: this is the check for a segment which is totally off the
> > end, the below one is for a segment which is only partly off the end. I'm
> > not sure what the spec says about the second case (do we perform the valid
> > writes and then error out? That's what wasm code guarantees). I'd also like
> > both checks to have their own error message later, when we improve messages
> > and testing.
> > 
> > I don't think it's off by one: say a memory is sized at 1024, and the offset
> > is 1024 (and write one byte), then that's one off the end. An offset of 1023
> > would be fine. Or do I have that wrong?
> I think you're wrong. I'm just thinking about this check the way I think
> about all other array memory access. In your example:
> memory = [0, 1023]
> sizeInBytes = 1024 
> offset = 1024
> We *could* still do the write with this check. But as I said, I think the
> below check should handle all cases (including this).

Ah gotcha, you're right. Here's some improved version (with other tests):
  https://bugs.webkit.org/show_bug.cgi?id=165733
The spec is still ambiguous, but this seems sensible.