Bug 165696

Summary: webassembly: implement data section
Product: WebKit Reporter: JF Bastien <jfbastien>
Component: JavaScriptCoreAssignee: JF Bastien <jfbastien>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, jfbastien, keith_miller, mark.lam, msaboff, sbarati, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 161709, 165700, 165733    
Attachments:
Description Flags
sample test
jfbastien: commit-queue-
Builder and its test
jfbastien: commit-queue-
more tests, some C++, missing ::evaluate after Instance is created
jfbastien: commit-queue-
patch for review
none
patch none

Comment 1 JF Bastien 2016-12-09 15:46:53 PST
Created attachment 296727 [details]
sample test

Sample test. PLAT and tell me if you don't like the API.
Comment 2 JF Bastien 2016-12-09 16:27:11 PST
Created attachment 296736 [details]
Builder and its test
Comment 3 JF Bastien 2016-12-09 18:57:40 PST
Created attachment 296766 [details]
more tests, some C++, missing ::evaluate after Instance is created
Comment 4 JF Bastien 2016-12-09 19:01:47 PST
rdar://problem/29599594
Comment 5 JF Bastien 2016-12-09 21:15:51 PST
Created attachment 296776 [details]
patch for review
Comment 6 WebKit Commit Bot 2016-12-09 21:18:14 PST
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.
Comment 7 JF Bastien 2016-12-09 21:25:03 PST
Created attachment 296777 [details]
patch

Fix include case.
Comment 8 Keith Miller 2016-12-09 22:42:49 PST
Comment on attachment 296777 [details]
patch

r=me.
Comment 9 WebKit Commit Bot 2016-12-09 23:09:01 PST
Comment on attachment 296777 [details]
patch

Clearing flags on attachment: 296777

Committed r209651: <http://trac.webkit.org/changeset/209651>
Comment 10 WebKit Commit Bot 2016-12-09 23:09:06 PST
All reviewed patches have been landed.  Closing bug.
Comment 11 Saam Barati 2016-12-10 00:49:44 PST
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.
Comment 12 JF Bastien 2016-12-10 08:52:09 PST
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?
Comment 13 Saam Barati 2016-12-10 10:20:40 PST
(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).
Comment 14 JF Bastien 2016-12-10 13:42:46 PST
(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.