Bug 165696

Created attachment 296727 [details] sample test Sample test. PLAT and tell me if you don't like the API.

Created attachment 296736 [details] Builder and its test

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

Created attachment 296776 [details] patch for review

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.

Created attachment 296777 [details] patch Fix include case.

Comment on attachment 296777 [details] patch r=me.

Comment on attachment 296777 [details] patch Clearing flags on attachment: 296777 Committed r209651: <http://trac.webkit.org/changeset/209651>

All reviewed patches have been landed. Closing bug.

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 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?

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

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