Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2548243004:
Return false in TryNumberToSize if there is a cast error (Closed)
DescriptionReturn false in TryNumberToSize if the number is 1 << 64.
Currently when the number passed to TryNumberToSize is 1 << 64,
it gets away with a bug caused by rounding of mantissa.
Then the number will be casted to 0 and TryNumberToSize
will return true. This patch fix this by making the range check
more accurate.
BUG=v8:5712
Committed: https://crrev.com/9ca022fab2e7310cc96ff405baf426f6ed27a703
Cr-Commit-Position: refs/heads/master@{#41578}
Patch Set 1 #Patch Set 2 : Return false in TryNumberToSize if there is a cast error #Patch Set 3 : Return false in TryNumberToSize if there is a cast error #Patch Set 4 : Return false in TryNumberToSize if there is a cast error #Patch Set 5 : Return false in TryNumberToSize if there is a cast error #
Total comments: 6
Patch Set 6 : y #Patch Set 7 : Return false in TryNumberToSize if the number is 1 << 64. #Patch Set 8 : Return false in TryNumberToSize if the number is 1 << 64. #Patch Set 9 : Return false in TryNumberToSize if the number is 1 << 64. #
Messages
Total messages: 29 (21 generated)
Description was changed from ========== Return false in TryNumberToSize if there is a cast error BUG=https://bugs.chromium.org/p/v8/issues/detail?id=5712 ========== to ========== Return false in TryNumberToSize if there is a cast error BUG=v8:5712 ==========
The CQ bit was checked by qiuyi.zqy@alibaba-inc.com
The CQ bit was unchecked by qiuyi.zqy@alibaba-inc.com
Description was changed from ========== Return false in TryNumberToSize if there is a cast error BUG=v8:5712 ========== to ========== Return false in TryNumberToSize if there is a cast error When the number passed is slightly larger than std::numeric_limits<size_t>::max(), a floating number precision loss can cause it to escape the range check, and get casted to 0. This patch fixes that with an additional overflow check. BUG=v8:5712 R=ahaas@chromium.org ==========
Description was changed from ========== Return false in TryNumberToSize if there is a cast error When the number passed is slightly larger than std::numeric_limits<size_t>::max(), a floating number precision loss can cause it to escape the range check, and get casted to 0. This patch fixes that with an additional overflow check. BUG=v8:5712 R=ahaas@chromium.org ========== to ========== Return false in TryNumberToSize if there is a cast error When the number passed is slightly larger than std::numeric_limits<size_t>::max(), a floating number precision loss can cause it to escape the range check, and get casted to 0. This patch fixes that with an additional overflow check. BUG=v8:5712 TBR=OWNERS ==========
Description was changed from ========== Return false in TryNumberToSize if there is a cast error When the number passed is slightly larger than std::numeric_limits<size_t>::max(), a floating number precision loss can cause it to escape the range check, and get casted to 0. This patch fixes that with an additional overflow check. BUG=v8:5712 TBR=OWNERS ========== to ========== Return false in TryNumberToSize if there is a cast error When the number passed is slightly larger than std::numeric_limits<size_t>::max(), a floating number precision loss can cause it to escape the range check, and get casted to 0. This patch fixes that with an additional overflow check. BUG=v8:5712 TBR=OWNERS ==========
qiuyi.zqy@alibaba-inc.com changed reviewers: + ahaas@chromium.org, ofrobots@google.com
Hi, the bug is discovered in https://github.com/nodejs/node/pull/9924 and I tried to come up with a patch for it. This is the first time I submit code for V8 so I also request try jobs be run. I'm not very familiar with the process so any feedback would be very much appreciated.
qiuyi.zqy@alibaba-inc.com changed reviewers: + mstarzinger@chromium.org
Thanks for taking care of this issue. https://codereview.chromium.org/2548243004/diff/80001/src/conversions-inl.h File src/conversions-inl.h (right): https://codereview.chromium.org/2548243004/diff/80001/src/conversions-inl.h#n... src/conversions-inl.h:157: if (value >= 0 && value <= std::numeric_limits<size_t>::max()) { The problem here is that if you compare a double with a size_t, the size_t is cast implicitly to double, and if you cast std::numeric_limits<size_t>::max() to double you get 0x10000000000000000. Knowing that, I think the solution here should just be to write if (value >= 0 && value < static_cast<double>(std::numeric_limits<size_t>::max())) with "<" instead of "<=", and with the static_cast to make this issue more obvious. https://codereview.chromium.org/2548243004/diff/80001/test/cctest/test-conver... File test/cctest/test-conversions.cc (right): https://codereview.chromium.org/2548243004/diff/80001/test/cctest/test-conver... test/cctest/test-conversions.cc:454: double value = StringToDouble(&uc, "0x10000000000000000", I think you could just write double value = 18446744073709551616.0; which is 0x10000000000000000. Please add a comment for that, because I think this is not obvious to see. https://codereview.chromium.org/2548243004/diff/80001/test/cctest/test-conver... test/cctest/test-conversions.cc:458: if (success && value > 0) { I think you could just write "CHECK(!success);" instead of this if-block.
Description was changed from ========== Return false in TryNumberToSize if there is a cast error When the number passed is slightly larger than std::numeric_limits<size_t>::max(), a floating number precision loss can cause it to escape the range check, and get casted to 0. This patch fixes that with an additional overflow check. BUG=v8:5712 TBR=OWNERS ========== to ========== Return false in TryNumberToSize if the number is 1 << 64. Currently when the number passed to TryNumberToSize is 1 << 64, it gets away with a bug caused by rounding of mantissa. Then the number will be casted to 0 and TryNumberToSize will return true. This patch fix this by making the range check more accurate. BUG=v8:5712 ==========
qiuyi.zqy@alibaba-inc.com changed reviewers: - mstarzinger@chromium.org, ofrobots@google.com
Thank you for the review, they have been addressed. https://codereview.chromium.org/2548243004/diff/80001/src/conversions-inl.h File src/conversions-inl.h (right): https://codereview.chromium.org/2548243004/diff/80001/src/conversions-inl.h#n... src/conversions-inl.h:157: if (value >= 0 && value <= std::numeric_limits<size_t>::max()) { On 2016/12/07 16:20:38, ahaas wrote: > The problem here is that if you compare a double with a size_t, the size_t is > cast implicitly to double, and if you cast std::numeric_limits<size_t>::max() to > double you get 0x10000000000000000. Knowing that, I think the solution here > should just be to write > if (value >= 0 && value < > static_cast<double>(std::numeric_limits<size_t>::max())) with "<" instead of > "<=", and with the static_cast to make this issue more obvious. > Done. https://codereview.chromium.org/2548243004/diff/80001/test/cctest/test-conver... File test/cctest/test-conversions.cc (right): https://codereview.chromium.org/2548243004/diff/80001/test/cctest/test-conver... test/cctest/test-conversions.cc:454: double value = StringToDouble(&uc, "0x10000000000000000", On 2016/12/07 16:20:38, ahaas wrote: > I think you could just write > double value = 18446744073709551616.0; > which is 0x10000000000000000. Please add a comment for that, because I think > this is not obvious to see. Done. https://codereview.chromium.org/2548243004/diff/80001/test/cctest/test-conver... test/cctest/test-conversions.cc:458: if (success && value > 0) { On 2016/12/07 16:20:38, ahaas wrote: > I think you could just write "CHECK(!success);" instead of this if-block. Done.
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: The author qiuyi.zqy@alibaba-inc.com has not signed Google Contributor License Agreement. Please visit https://cla.developers.google.com to sign and manage CLA.
The CLA check should be cleared now, I have asked our legal staff to add me to Alibaba's CCLA. Can you rerun the CQ bot please? Sorry for not getting it right before the CQ run.
lgtm
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.
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...
CQ is committing da patch.
Bot data: {"patchset_id": 160001, "attempt_start_ts": 1481188693221380,
"parent_rev": "12464be74eb701ae789d521f376f96dd2979b072", "commit_rev":
"9edaf0d0fce867359ed189a4ac4709b41792b6ec"}
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Return false in TryNumberToSize if the number is 1 << 64. Currently when the number passed to TryNumberToSize is 1 << 64, it gets away with a bug caused by rounding of mantissa. Then the number will be casted to 0 and TryNumberToSize will return true. This patch fix this by making the range check more accurate. BUG=v8:5712 ========== to ========== Return false in TryNumberToSize if the number is 1 << 64. Currently when the number passed to TryNumberToSize is 1 << 64, it gets away with a bug caused by rounding of mantissa. Then the number will be casted to 0 and TryNumberToSize will return true. This patch fix this by making the range check more accurate. BUG=v8:5712 Committed: https://crrev.com/9ca022fab2e7310cc96ff405baf426f6ed27a703 Cr-Commit-Position: refs/heads/master@{#41578} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/9ca022fab2e7310cc96ff405baf426f6ed27a703 Cr-Commit-Position: refs/heads/master@{#41578} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
