Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2641953002:
Trigger OOM crash if no memory returned in v8::ArrayBuffer::New and v8::SharedArrayBuffer::New. (Closed)
|
Created:
3 years, 11 months ago by jbroman Modified:
3 years, 11 months ago Reviewers:
Jakob Kummerow CC:
v8-reviews_googlegroups.com Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
DescriptionTrigger OOM crash if no memory returned in v8::ArrayBuffer::New and v8::SharedArrayBuffer::New.
This API does not allow reporting failure, but we should crash rather than have
the caller get an ArrayBuffer that isn't properly set up.
BUG=chromium:681843
Review-Url: https://codereview.chromium.org/2641953002
Cr-Commit-Position: refs/heads/master@{#42511}
Committed: https://chromium.googlesource.com/v8/v8/+/ca0f957329828c61f02437f640ed8004a549018a
Patch Set 1 #Patch Set 2 : is a GTEST_HAS_DEATH_TEST guard needed? #Patch Set 3 : remove msg on windows #Patch Set 4 : remove test #Messages
Total messages: 22 (17 generated)
The CQ bit was checked by jbroman@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: Try jobs failed on following builders: v8_win64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng/builds/21096) v8_win64_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng_triggered/b...)
The CQ bit was checked by jbroman@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: Try jobs failed on following builders: v8_win64_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng/builds/21103) v8_win64_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win64_rel_ng_triggered/b...) v8_win_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng/builds/21187) v8_win_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng_triggered/bui...)
The CQ bit was checked by jbroman@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: Try jobs failed on following builders: v8_linux64_asan_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng/buil...) v8_linux64_asan_rel_ng_triggered on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_asan_rel_ng_trig...)
jbroman@chromium.org changed reviewers: + jkummerow@chromium.org
LGTM, thanks for fixing this. I'm not convinced of the usefulness of the test. If you like it, you can keep it (it does test something we didn't test before), but I don't see any particularly likely regressions it might prevent (e.g., new callers of SetupAllocatingData won't be caught, and the possible future API function your TODO mentions will need new or adapted tests too). Please annotate SetupAllocatingData with MUST_USE_RESULT. That should catch any future abuse, and even find any additional existing places that might need updating :-)
On 2017/01/19 at 13:44:15, jkummerow wrote: > LGTM, thanks for fixing this. > > I'm not convinced of the usefulness of the test. If you like it, you can keep it (it does test something we didn't test before), but I don't see any particularly likely regressions it might prevent (e.g., new callers of SetupAllocatingData won't be caught, and the possible future API function your TODO mentions will need new or adapted tests too). OK, removed. I'm in the habit of adding unit tests when fixing bugs, but each project has a different line about what should and should not be unit tested. > Please annotate SetupAllocatingData with MUST_USE_RESULT. That should catch any future abuse, and even find any additional existing places that might need updating :-) Will add in a followup patch. (I might request merging this one.)
The CQ bit was checked by jbroman@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jkummerow@chromium.org Link to the patchset: https://codereview.chromium.org/2641953002/#ps60001 (title: "remove test")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1484840301432680,
"parent_rev": "5e30385d62496544adacc7cb8e3c86694854b9f1", "commit_rev":
"ca0f957329828c61f02437f640ed8004a549018a"}
Message was sent while issue was closed.
Description was changed from ========== Trigger OOM crash if no memory returned in v8::ArrayBuffer::New and v8::SharedArrayBuffer::New. This API does not allow reporting failure, but we should crash rather than have the caller get an ArrayBuffer that isn't properly set up. BUG=chromium:681843 ========== to ========== Trigger OOM crash if no memory returned in v8::ArrayBuffer::New and v8::SharedArrayBuffer::New. This API does not allow reporting failure, but we should crash rather than have the caller get an ArrayBuffer that isn't properly set up. BUG=chromium:681843 Review-Url: https://codereview.chromium.org/2641953002 Cr-Commit-Position: refs/heads/master@{#42511} Committed: https://chromium.googlesource.com/v8/v8/+/ca0f957329828c61f02437f640ed8004a54... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/v8/v8/+/ca0f957329828c61f02437f640ed8004a54... |
