Summary: | Creating a wasm memory that is bigger than the ArrayBuffer limit but smaller than the spec limit should throw OOME not RangeError | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Filip Pizlo <fpizlo> | ||||||||
Component: | JavaScriptCore | Assignee: | Mark Lam <mark.lam> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | commit-queue, ews-watchlist, keith_miller, mark.lam, msaboff, rmorisset, saam, tzagallo, webkit-bug-importer, ysuzuki | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | All | ||||||||||
OS: | All | ||||||||||
Attachments: |
|
Description
Filip Pizlo
2018-11-16 14:38:47 PST
Created attachment 355436 [details]
proposed patch.
Comment on attachment 355436 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=355436&action=review > JSTests/wasm/regress/wasm-memory-requested-more-than-MAX_ARRAY_BUFFER_SIZE-2.js:88 > +var exception; > +try { > + var module = new WasmModuleBuilder(); > + module.addMemory(32768); > + module.instantiate(); > +} catch (e) { > + exception = e; > +} > + > +if (exception != "Error: Out of memory") { > + print(exception); > + throw "FAILED"; > +} Why not just write this as a wasm test so you're not copying part of the module builder? Comment on attachment 355436 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=355436&action=review Thanks for the review. >> JSTests/wasm/regress/wasm-memory-requested-more-than-MAX_ARRAY_BUFFER_SIZE-2.js:88 >> +} > > Why not just write this as a wasm test so you're not copying part of the module builder? This test is a cleaned up version of one provided by an external contributor. I rather keep it close to its original form. Also, this "Out of memory" behavior is not required (though allowed) by the WASM spec: it is an artifact of our implementation that does not allow ArrayBuffers to be greater than MAX_ARRAY_BUFFER_SIZE in size. The bug is that we were previously throwing a RangeError or crashing on a RELEASE_ASSERT. This patch fixes it to throw an OOME instead. Since this is not spec required behavior, I don't think it's right to put it in the wasm/spec-tests directory. Comment on attachment 355436 [details] proposed patch. Attachment 355436 [details] did not pass jsc-ews (mac): Output: https://webkit-queues.webkit.org/results/10102730 New failing tests: stress/big-wasm-memory-grow-no-max.js.ftl-eager-no-cjit stress/big-wasm-memory.js.ftl-eager stress/big-wasm-memory-grow-no-max.js.no-cjit-validate-phases stress/big-wasm-memory-grow.js.dfg-eager stress/big-wasm-memory-grow.js.no-cjit-collect-continuously stress/big-wasm-memory.js.ftl-eager-no-cjit-b3o1 stress/big-wasm-memory-grow.js.no-cjit-validate-phases stress/big-wasm-memory.js.ftl-no-cjit-small-pool stress/big-wasm-memory-grow-no-max.js.ftl-eager-no-cjit-b3o1 stress/big-wasm-memory-grow.js.default stress/big-wasm-memory-grow-no-max.js.ftl-eager stress/big-wasm-memory-grow-no-max.js.no-ftl stress/big-wasm-memory-grow-no-max.js.default stress/big-wasm-memory-grow-no-max.js.dfg-eager-no-cjit-validate stress/big-wasm-memory-grow.js.ftl-no-cjit-small-pool stress/big-wasm-memory-grow.js.ftl-eager stress/big-wasm-memory-grow.js.ftl-no-cjit-validate-sampling-profiler stress/big-wasm-memory.js.ftl-no-cjit-no-inline-validate stress/big-wasm-memory.js.ftl-no-cjit-validate-sampling-profiler stress/big-wasm-memory-grow-no-max.js.no-llint stress/big-wasm-memory-grow.js.ftl-no-cjit-no-inline-validate stress/big-wasm-memory.js.default stress/big-wasm-memory.js.dfg-eager-no-cjit-validate stress/big-wasm-memory-grow.js.no-ftl stress/big-wasm-memory-grow-no-max.js.ftl-no-cjit-no-inline-validate stress/big-wasm-memory.js.no-llint stress/big-wasm-memory.js.ftl-eager-no-cjit stress/big-wasm-memory-grow.js.ftl-no-cjit-no-put-stack-validate stress/big-wasm-memory.js.no-cjit-validate-phases stress/big-wasm-memory-grow-no-max.js.dfg-eager stress/big-wasm-memory-grow.js.dfg-eager-no-cjit-validate stress/big-wasm-memory.js.no-cjit-collect-continuously stress/big-wasm-memory-grow.js.dfg-maximal-flush-validate-no-cjit stress/big-wasm-memory-grow.js.no-llint stress/big-wasm-memory.js.ftl-no-cjit-b3o1 stress/big-wasm-memory.js.dfg-maximal-flush-validate-no-cjit stress/big-wasm-memory.js.ftl-no-cjit-no-put-stack-validate stress/big-wasm-memory-grow-no-max.js.dfg-maximal-flush-validate-no-cjit stress/big-wasm-memory-grow.js.ftl-no-cjit-b3o1 stress/big-wasm-memory-grow-no-max.js.ftl-no-cjit-small-pool stress/big-wasm-memory-grow.js.ftl-eager-no-cjit-b3o1 stress/big-wasm-memory.js.dfg-eager stress/big-wasm-memory-grow-no-max.js.no-cjit-collect-continuously stress/big-wasm-memory-grow-no-max.js.ftl-no-cjit-validate-sampling-profiler stress/big-wasm-memory-grow-no-max.js.ftl-no-cjit-no-put-stack-validate stress/big-wasm-memory-grow-no-max.js.ftl-no-cjit-b3o1 stress/big-wasm-memory.js.no-ftl stress/big-wasm-memory-grow.js.ftl-eager-no-cjit apiTests Created attachment 355442 [details]
patch for landing.
Created attachment 355443 [details]
patch for landing.
Comment on attachment 355443 [details] patch for landing. Clearing flags on attachment: 355443 Committed r238433: <https://trac.webkit.org/changeset/238433> All reviewed patches have been landed. Closing bug. |