Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2288773002:
[wasm] Check the input of grow-memory before calling the runtime. (Closed)
Description[wasm] Check the input of grow-memory before calling the runtime.
If the input of grow-memory was not representable as a SMI, then the
input was not passed correctly to the runtime, which caused a crash.
With this CL the input of grow-memory is checked before the runtime is
called.
R=titzer@chromium.org, gdeepti@chromium.org
TEST=mjsunit/wasm/grow-memory.js:testGrowMemoryTrapsWithNonSmiInput()
Committed: https://crrev.com/9f747be5a7efa72d4e2d75c796a5f54a93d3b307
Cr-Commit-Position: refs/heads/master@{#39022}
Patch Set 1 #
Total comments: 7
Patch Set 2 : Comments. #
Messages
Total messages: 24 (12 generated)
The CQ bit was checked by ahaas@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2288773002/diff/1/src/compiler/wasm-compiler.cc File src/compiler/wasm-compiler.cc (right): https://codereview.chromium.org/2288773002/diff/1/src/compiler/wasm-compiler.... src/compiler/wasm-compiler.cc:1684: input = BuildChangeUint32ToSmi(input); If I remember correctly this will also have a check inside. Can we avoid that somehow?
https://codereview.chromium.org/2288773002/diff/1/src/compiler/wasm-compiler.cc File src/compiler/wasm-compiler.cc (right): https://codereview.chromium.org/2288773002/diff/1/src/compiler/wasm-compiler.... src/compiler/wasm-compiler.cc:1676: BranchHint::kTrue); This makes the first check in the GrowMemory runtime function (for 0 initial memory) redundant, remove the one in wasm-runtime.cc? https://codereview.chromium.org/2288773002/diff/1/test/mjsunit/wasm/grow-memo... File test/mjsunit/wasm/grow-memory.js (right): https://codereview.chromium.org/2288773002/diff/1/test/mjsunit/wasm/grow-memo... test/mjsunit/wasm/grow-memory.js:128: })(); Nit: Test not consistent with the format of other tests in this file. Fix to be consistent?
The CQ bit was checked by ahaas@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2288773002/diff/1/src/compiler/wasm-compiler.cc File src/compiler/wasm-compiler.cc (right): https://codereview.chromium.org/2288773002/diff/1/src/compiler/wasm-compiler.... src/compiler/wasm-compiler.cc:1676: BranchHint::kTrue); On 2016/08/30 at 02:19:56, gdeepti wrote: > This makes the first check in the GrowMemory runtime function (for 0 initial memory) redundant, remove the one in wasm-runtime.cc? I replaced the check in runtime-wasm.cc with a DCHECK. https://codereview.chromium.org/2288773002/diff/1/src/compiler/wasm-compiler.... src/compiler/wasm-compiler.cc:1684: input = BuildChangeUint32ToSmi(input); On 2016/08/29 at 16:57:58, titzer wrote: > If I remember correctly this will also have a check inside. Can we avoid that somehow? I do not understand this comment, do you mean the check in the GrowMemory runtime function? I also wonder if it is worth to optimize code here because GrowMemory should be quite cold. https://codereview.chromium.org/2288773002/diff/1/test/mjsunit/wasm/grow-memo... File test/mjsunit/wasm/grow-memory.js (right): https://codereview.chromium.org/2288773002/diff/1/test/mjsunit/wasm/grow-memo... test/mjsunit/wasm/grow-memory.js:128: })(); On 2016/08/30 at 02:19:56, gdeepti wrote: > Nit: Test not consistent with the format of other tests in this file. Fix to be consistent? You are right, it should be consistent. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: v8_linux_arm_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng/builds/...) v8_linux_arm_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_arm_rel_ng_trigger...)
https://codereview.chromium.org/2288773002/diff/1/src/compiler/wasm-compiler.cc File src/compiler/wasm-compiler.cc (right): https://codereview.chromium.org/2288773002/diff/1/src/compiler/wasm-compiler.... src/compiler/wasm-compiler.cc:1684: input = BuildChangeUint32ToSmi(input); On 2016/08/30 06:49:53, ahaas wrote: > On 2016/08/29 at 16:57:58, titzer wrote: > > If I remember correctly this will also have a check inside. Can we avoid that > somehow? > > I do not understand this comment, do you mean the check in the GrowMemory > runtime function? I also wonder if it is worth to optimize code here because > GrowMemory should be quite cold. Sorry, I was thinking that BuildChangeUint32ToSmi() had a branch inside, but apparently it doesn't.
lgtm
The CQ bit was checked by ahaas@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: v8_presubmit on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_presubmit/builds/22891)
lgtm
The CQ bit was checked by ahaas@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== [wasm] Check the input of grow-memory before calling the runtime. If the input of grow-memory was not representable as a SMI, then the input was not passed correctly to the runtime, which caused a crash. With this CL the input of grow-memory is checked before the runtime is called. R=titzer@chromium.org, gdeepti@chromium.org TEST=mjsunit/wasm/grow-memory.js:testGrowMemoryTrapsWithNonSmiInput() ========== to ========== [wasm] Check the input of grow-memory before calling the runtime. If the input of grow-memory was not representable as a SMI, then the input was not passed correctly to the runtime, which caused a crash. With this CL the input of grow-memory is checked before the runtime is called. R=titzer@chromium.org, gdeepti@chromium.org TEST=mjsunit/wasm/grow-memory.js:testGrowMemoryTrapsWithNonSmiInput() Committed: https://crrev.com/9f747be5a7efa72d4e2d75c796a5f54a93d3b307 Cr-Commit-Position: refs/heads/master@{#39022} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/9f747be5a7efa72d4e2d75c796a5f54a93d3b307 Cr-Commit-Position: refs/heads/master@{#39022} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
