Integer-overflow in blink::compareAspectRatioValue |
|||||||||
Issue descriptionDetailed report: https://cluster-fuzz.appspot.com/testcase?key=5574437900648448 Fuzzer: inferno_twister Job Type: linux_ubsan_chrome Platform Id: linux Crash Type: Integer-overflow Crash Address: Crash State: blink::compareAspectRatioValue blink::MediaQueryEvaluator::eval blink::MediaQueryEvaluator::eval Regressed: https://cluster-fuzz.appspot.com/revisions?job=linux_ubsan_chrome&range=370022:370027 Minimized Testcase (0.42 Kb): Download: https://cluster-fuzz.appspot.com/download/AMIfv97Z3ciNFYwTK81tFJUv0nwAKdoY7g1cdZ9JQAfaNl6gwsdfa3wl5Xo2KUQPNV2aJneSsmgb0k6tNlq-4ygzSynvvHnrdia9u1NbaIogIQiJ6UyLrHfXK0aU2OoJaPfWT-r1OnRoNL4hwQbl_sEUU-T_ASYN4w?testcase_id=5574437900648448 <head id=myhead> <script> var headElement = document.getElementById("myhead"); var linkElement = document.createElement("link"); linkElement.rel = "stylesheet"; width = 2147483548; height = 2147483575; linkElement.media = "screen and (max-device-aspect-ratio: " + width + "/" + height + ")"; linkElement.href = "resources/device-aspect-ratio.css"; headElement.appendChild(linkElement); </script> Issue filed automatically. See https://dev.chromium.org/Home/chromium-security/bugs/reproducing-clusterfuzz-bugs for more information.
,
Feb 13 2017
pilgrim: Did you make any progress investigating this?
,
Mar 27 2017
Setting to Unconfirmed to send this back through Blink>CSS daily triage.
,
Mar 27 2017
Requested CF redo the blame.
,
Mar 27 2017
I guess the clang change... But.. seems simple enough, height is 2147483575 and that can't fit once multiplied by 1280 ../../third_party/WebKit/Source/core/css/MediaQueryEvaluator.cpp:192:35: runtime error: signed integer overflow: 1280 * 2147483575 cannot be represented in type int #0 0x7f7b2fc63827 in blink::compareAspectRatioValue(blink::MediaQueryExpValue const&, int, int, blink::MediaFeaturePrefix) third_party/WebKit/Source/core/css/MediaQueryEvaluator.cpp:192:35
,
Mar 28 2017
,
Mar 28 2017
compareAspectRatioValue takes two int arguments. In one location it is called with two ints. In the other location it is called with two doubles and so in that case presumably has an implicit conversion from double to int. These int arguments are each multiplied by static_cast<int>(x) where x is of type unsigned. Prior to https://codereview.chromium.org/240453010 (from 2014), x was of type float. This is all kinds of confusing, and not knowing the media query code, I'm cautious about making assertions about what numerical types should be used here. yoav: As author of the patch above, plus the recent https://codereview.chromium.org/1904523002 that deals with numeric types in this code, would you mind taking a look?
,
Mar 28 2017
> compareAspectRatioValue takes two int arguments. In one location it is called with two ints. In the other location it is called with two doubles and so in that case presumably has an implicit conversion from double to int. The implicit conversion seems bad, and is most probably observable. We should fix it. > These int arguments are each multiplied by static_cast<int>(x) where x is of type unsigned. Prior to https://codereview.chromium.org/240453010 (from 2014), x was of type float. x is supposed to be unsigned (see https://drafts.csswg.org/mediaqueries/#aspect-ratio) At the same time, the static_cast from unsigned to int seems unnecessary. Adding Rune, as he's more active is this code than myself
,
Apr 4 2017
Rune, can you take a look?
,
Apr 27 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/cf115265d17e7eec4e8c6c2d4441942988dc84db commit cf115265d17e7eec4e8c6c2d4441942988dc84db Author: rune <rune@opera.com> Date: Thu Apr 27 07:13:49 2017 Better overflow handling for aspect-ratio MQ. This change fixes two issues: 1. Clamp instead of casting double values from parser to internal unsigned storage. 2. Promote width/height/numerator/denominator multiplications to double to avoid integer overflow for large numerator/denominators. R=yoav@yoav.ws,suzyh@chromium.org BUG= 676709 , 714079 Review-Url: https://codereview.chromium.org/2836613002 Cr-Commit-Position: refs/heads/master@{#467615} [add] https://crrev.com/cf115265d17e7eec4e8c6c2d4441942988dc84db/third_party/WebKit/LayoutTests/fast/media/mq-aspect-ratio-overflow.html [modify] https://crrev.com/cf115265d17e7eec4e8c6c2d4441942988dc84db/third_party/WebKit/Source/core/css/MediaQueryEvaluator.cpp [modify] https://crrev.com/cf115265d17e7eec4e8c6c2d4441942988dc84db/third_party/WebKit/Source/core/css/MediaQueryEvaluatorTest.cpp [modify] https://crrev.com/cf115265d17e7eec4e8c6c2d4441942988dc84db/third_party/WebKit/Source/core/css/MediaQueryExp.cpp
,
Apr 27 2017
,
Apr 28 2017
ClusterFuzz has detected this issue as fixed in range 467606:467617. Detailed report: https://clusterfuzz.com/testcase?key=5574437900648448 Fuzzer: inferno_twister Job Type: linux_ubsan_chrome Platform Id: linux Crash Type: Integer-overflow Crash Address: Crash State: blink::compareAspectRatioValue blink::MediaQueryEvaluator::eval blink::MediaQueryEvaluator::eval Sanitizer: undefined (UBSAN) Regressed: https://clusterfuzz.com/revisions?job=linux_ubsan_chrome&range=370022:370027 Fixed: https://clusterfuzz.com/revisions?job=linux_ubsan_chrome&range=467606:467617 Reproducer Testcase: https://clusterfuzz.com/download?testcase_id=5574437900648448 See https://dev.chromium.org/Home/chromium-security/bugs/reproducing-clusterfuzz-bugs for more information. If you suspect that the result above is incorrect, try re-doing that job on the test case report page. |
|||||||||
►
Sign in to add a comment |
|||||||||
Comment 1 by msrchandra@chromium.org
, Dec 23 2016Components: Blink>CSS
Labels: Test-Predator-Wrong-CLs
Owner: pilgrim@chromium.org
Status: Assigned (was: Untriaged)