Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2399873002:
ValueSerializer: Add more checks before trying to allocate memory for a dense array. (Closed)
DescriptionValueSerializer: Add more checks before trying to allocate memory for a dense array.
Found with libfuzzer. The length is automatically converted to int (thus
large sizes could become negative, even though they are legal "array sizes").
Besides that, the length is coerced to a SMI (which is an even tighter
constraint on 32-bit systems, where it limits the legal sizes to 2^30 - 1).
Add checks that the length of a dense array is below that threshold, and also
fail fast if a length that is provided obviously could not be the correct dense
length (because there isn't enough data left in the buffer to populate such an
array).
BUG=chromium:148757
Committed: https://crrev.com/0004733c0814a5f8c5cf519a062e525dec591224
Cr-Commit-Position: refs/heads/master@{#40094}
Patch Set 1 #Patch Set 2 : be explicit to prevent signed/unsigned comparison #
Total comments: 2
Patch Set 3 : use FixedArray::kMaxLength #Patch Set 4 : signed/unsigned #
Messages
Total messages: 27 (19 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_win_rel_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_win_rel_ng/builds/15690)
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: This issue passed the CQ dry run.
jbroman@chromium.org changed reviewers: + cbruni@chromium.org, jkummerow@chromium.org
lgtm https://codereview.chromium.org/2399873002/diff/20001/src/value-serializer.cc File src/value-serializer.cc (right): https://codereview.chromium.org/2399873002/diff/20001/src/value-serializer.cc... src/value-serializer.cc:26: static const uint32_t kMaxDenseArrayLength = 0x3FFFFFFF; Why not FixedArray::kMaxLength?
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/2399873002/#ps40001 (title: "use FixedArray::kMaxLength")
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_linux_dbg_ng on master.tryserver.v8 (JOB_FAILED, http://build.chromium.org/p/tryserver.v8/builders/v8_linux_dbg_ng/builds/14186)
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: This issue passed the CQ dry run.
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/2399873002/#ps60001 (title: "signed/unsigned")
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 #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== ValueSerializer: Add more checks before trying to allocate memory for a dense array. Found with libfuzzer. The length is automatically converted to int (thus large sizes could become negative, even though they are legal "array sizes"). Besides that, the length is coerced to a SMI (which is an even tighter constraint on 32-bit systems, where it limits the legal sizes to 2^30 - 1). Add checks that the length of a dense array is below that threshold, and also fail fast if a length that is provided obviously could not be the correct dense length (because there isn't enough data left in the buffer to populate such an array). BUG=chromium:148757 ========== to ========== ValueSerializer: Add more checks before trying to allocate memory for a dense array. Found with libfuzzer. The length is automatically converted to int (thus large sizes could become negative, even though they are legal "array sizes"). Besides that, the length is coerced to a SMI (which is an even tighter constraint on 32-bit systems, where it limits the legal sizes to 2^30 - 1). Add checks that the length of a dense array is below that threshold, and also fail fast if a length that is provided obviously could not be the correct dense length (because there isn't enough data left in the buffer to populate such an array). BUG=chromium:148757 Committed: https://crrev.com/0004733c0814a5f8c5cf519a062e525dec591224 Cr-Commit-Position: refs/heads/master@{#40094} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/0004733c0814a5f8c5cf519a062e525dec591224 Cr-Commit-Position: refs/heads/master@{#40094}
Message was sent while issue was closed.
https://codereview.chromium.org/2399873002/diff/20001/src/value-serializer.cc File src/value-serializer.cc (right): https://codereview.chromium.org/2399873002/diff/20001/src/value-serializer.cc... src/value-serializer.cc:26: static const uint32_t kMaxDenseArrayLength = 0x3FFFFFFF; On 2016/10/07 at 13:10:17, Jakob Kummerow wrote: > Why not FixedArray::kMaxLength? OK, done. I have some vague unease about that value changing, but it doesn't seem to have done so. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
