Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2870743004:
[value-serializer] Ensure deserialized JSRegExp flags are valid (Closed)
Description[value-serializer] Verify deserialized JSRegExp flags
One of the serializer fuzzers passes in random data to the deserializer,
which can then be used to deserialize a JSRegExp instance with random flag
contents. This can cause issues since the JSRegExp::Flag enum statically
contains kDotAll - but it is only valid to set kDotAll iff
FLAG_harmony_regexp_dotall is set.
This CL verifies deserialized flags before constructing the JSRegExp
and bails out if they are invalid.
R=jbroman@chromium.org,yangguo@chromium.org
BUG=chromium:719280
Review-Url: https://codereview.chromium.org/2870743004
Cr-Commit-Position: refs/heads/master@{#45222}
Committed: https://chromium.googlesource.com/v8/v8/+/540419b660ae8da1d24767be5b95fc0421680580
Patch Set 1 #
Total comments: 2
Patch Set 2 : Address comments #Patch Set 3 : Add a unit test #
Messages
Total messages: 32 (21 generated)
The CQ bit was checked by jgruber@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...
Hi, please take a look.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2870743004/diff/1/src/value-serializer.cc File src/value-serializer.cc (right): https://codereview.chromium.org/2870743004/diff/1/src/value-serializer.cc#new... src/value-serializer.cc:1477: const uint32_t masked_flags = raw_flags & flags_mask; Hmm. WDYT of rejecting such cases, rather than masking them out (which seems like it would lead to a different regexp on the deserializing end)? i.e. if (raw_flags & ~flags_mask || !JSRegExp::New(...).ToHandle(®exp)) { return MaybeHandle<JSRegExp>(); }
The CQ bit was checked by jgruber@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/2870743004/diff/1/src/value-serializer.cc File src/value-serializer.cc (right): https://codereview.chromium.org/2870743004/diff/1/src/value-serializer.cc#new... src/value-serializer.cc:1477: const uint32_t masked_flags = raw_flags & flags_mask; On 2017/05/09 14:52:23, jbroman wrote: > Hmm. WDYT of rejecting such cases, rather than masking them out (which seems > like it would lead to a different regexp on the deserializing end)? > > i.e. > > if (raw_flags & ~flags_mask || !JSRegExp::New(...).ToHandle(®exp)) { > return MaybeHandle<JSRegExp>(); > } SGTM. I was worried about rejecting a majority of the fuzzer inputs, but I guess we're already very restrictive when deserializing objects in general. Changed in the newest patchset.
lgtm
On 2017/05/09 at 15:05:01, jbroman wrote: > lgtm (though a unit test in value-serializer-unittest.cc might be nice)
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 jgruber@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...
On 2017/05/09 15:05:24, jbroman wrote: > On 2017/05/09 at 15:05:01, jbroman wrote: > > lgtm > > (though a unit test in value-serializer-unittest.cc might be nice) (added one :)
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 jgruber@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jbroman@chromium.org Link to the patchset: https://codereview.chromium.org/2870743004/#ps40001 (title: "Add a unit test")
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 jgruber@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
No L-G-T-M from a valid reviewer yet. CQ run can only be started once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer,_not_ a full super star committer. Committers are members of the group "project-v8-committers". Note that this has nothing to do with OWNERS files.
Description was changed from ========== [value-serializer] Mask to valid JSRegExp flags One of the serializer fuzzers passes in random data to the deserializer. This causes issues in JSRegExp::Flags since the Flag enum statically contains kDotAll but it is only valid iff FLAG_harmony_regexp_dotall is set. This CL masks out deserialized flags to a valid state. R=jbroman@chromium.org,yangguo@chromium.org BUG=chromium:719280 ========== to ========== [value-serializer] Verify deserialized JSRegExp flags One of the serializer fuzzers passes in random data to the deserializer, which can then be used to deserialize a JSRegExp instance with random flag contents. This can cause issues since the JSRegExp::Flag enum statically contains kDotAll - but it is only valid to set kDotAll iff FLAG_harmony_regexp_dotall is set. This CL verifies deserialized flags before constructing the JSRegExp and bails out if they are invalid. R=jbroman@chromium.org,yangguo@chromium.org BUG=chromium:719280 ==========
jgruber@chromium.org changed reviewers: + jkummerow@chromium.org
lgtm
The CQ bit was checked by jgruber@chromium.org
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": 40001, "attempt_start_ts": 1494406245956820,
"parent_rev": "0679765dafee0bc17b0f89cc557cd91b936e4d27", "commit_rev":
"540419b660ae8da1d24767be5b95fc0421680580"}
Message was sent while issue was closed.
Description was changed from ========== [value-serializer] Verify deserialized JSRegExp flags One of the serializer fuzzers passes in random data to the deserializer, which can then be used to deserialize a JSRegExp instance with random flag contents. This can cause issues since the JSRegExp::Flag enum statically contains kDotAll - but it is only valid to set kDotAll iff FLAG_harmony_regexp_dotall is set. This CL verifies deserialized flags before constructing the JSRegExp and bails out if they are invalid. R=jbroman@chromium.org,yangguo@chromium.org BUG=chromium:719280 ========== to ========== [value-serializer] Verify deserialized JSRegExp flags One of the serializer fuzzers passes in random data to the deserializer, which can then be used to deserialize a JSRegExp instance with random flag contents. This can cause issues since the JSRegExp::Flag enum statically contains kDotAll - but it is only valid to set kDotAll iff FLAG_harmony_regexp_dotall is set. This CL verifies deserialized flags before constructing the JSRegExp and bails out if they are invalid. R=jbroman@chromium.org,yangguo@chromium.org BUG=chromium:719280 Review-Url: https://codereview.chromium.org/2870743004 Cr-Commit-Position: refs/heads/master@{#45222} Committed: https://chromium.googlesource.com/v8/v8/+/540419b660ae8da1d24767be5b95fc04216... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/v8/v8/+/540419b660ae8da1d24767be5b95fc04216... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
