RESOLVED FIXED 119273
Arithmetic overflow when computing max-height CSS property with subpixel layout 
https://bugs.webkit.org/show_bug.cgi?id=119273
Summary Arithmetic overflow when computing max-height CSS property with subpixel layout
Javier Fernandez
Reported 2013-07-30 13:51:00 PDT
First of all, I think this bug affects all the platforms/ports, since it was detected while looking into the bug #118595 which affects all the ports enabling SUBPIXEL_LAYOUT flag by default. Secondly, The bug is not reproducible if the SATURATED_LAYOUT_ARITHMETIC flag is enabled, at least for the WebKitGtk+ port. Finally, when debugging the test provided by the bug #118595, I found out that the max-height CSS property is handled by the WebCore::CSSPrimitiveValue::computeLength<WebCore::Length> method, which, when using SUBPIXEL_LAYOUT, the property value of 2000000000px is passed through the MathExtras::clampTo<float> function with [min, max] = [-33554430, 33554429]. At some point in the layout process, the RenderBox::constrainLogicalHeightByMinMax method is called, causing the arithmetic overflow when adjusting the box borders at RenderBox::adjustBorderBoxLogicalHeightForBoxSizing while computing the height + bordersPlusPadding operation, being height the style()->logicalMaxHeight(). Clearly, the SATURATED_LAYOUT_ARITHMETIC support will avoid this overflow, since the "+" operator uses the "saturatedAddition", but I guess it would be also possible to protect the adjustBorderBoxLogicalHeightForBoxSizing method to avoid the problem. However, I tend to think that using SUBPIXEL_LAYOUT without the SATURATED_LAYOUT_ARITHMETIC is a risk in terms of this kind of arithmetic overflow.
Attachments
Test to verify the issue. (518 bytes, text/html)
2013-07-30 13:56 PDT, Javier Fernandez 		no flags
Details
Patch (3.91 KB, patch)
2013-07-30 15:20 PDT, Javier Fernandez 		no flags
DetailsFormatted DiffDiff
Patch (5.62 KB, patch)
2013-10-30 03:46 PDT, Javier Fernandez 		no flags
DetailsFormatted DiffDiff
Patch (5.85 KB, patch)
2013-10-30 03:52 PDT, Javier Fernandez 		no flags
DetailsFormatted DiffDiff
Patch (5.78 KB, patch)
2013-10-30 04:17 PDT, Javier Fernandez 		no flags
DetailsFormatted DiffDiff
Patch (5.87 KB, patch)
2013-10-30 04:36 PDT, Javier Fernandez 		no flags
DetailsFormatted DiffDiff
Patch (3.43 KB, patch)
2013-12-11 09:08 PST, Javier Fernandez 		no flags
DetailsFormatted DiffDiff
Show Obsolete (5) View All Add attachment proposed patch, testcase, etc.
Javier Fernandez
Comment 1 2013-07-30 13:56:59 PDT
Created attachment 207763 [details] Test to verify the issue.
Javier Fernandez
Comment 2 2013-07-30 15:20:31 PDT
Created attachment 207772 [details] Patch
Mihnea Ovidenie
Comment 3 2013-07-31 04:39:59 PDT
I would very much prefer to attempt a general fix, like always use SATURATED_LAYOUT_ARITHMETIC whenever ENABLE_SUBPIXEL_LAYOUT is enabled.
Javier Fernandez
Comment 4 2013-10-30 03:46:41 PDT
Created attachment 215492 [details] Patch
Javier Fernandez
Comment 5 2013-10-30 03:52:24 PDT
Created attachment 215493 [details] Patch
EFL EWS Bot
Comment 6 2013-10-30 03:58:29 PDT
Comment on attachment 215493 [details] Patch Attachment 215493 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/17628157
EFL EWS Bot
Comment 7 2013-10-30 03:59:50 PDT
Comment on attachment 215493 [details] Patch Attachment 215493 [details] did not pass efl-wk2-ews (efl-wk2): Output: http://webkit-queues.appspot.com/results/17578145
Javier Fernandez
Comment 8 2013-10-30 04:17:11 PDT
Created attachment 215495 [details] Patch
EFL EWS Bot
Comment 9 2013-10-30 04:23:16 PDT
Comment on attachment 215495 [details] Patch Attachment 215495 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/17558153
EFL EWS Bot
Comment 10 2013-10-30 04:24:40 PDT
Comment on attachment 215495 [details] Patch Attachment 215495 [details] did not pass efl-wk2-ews (efl-wk2): Output: http://webkit-queues.appspot.com/results/17598157
Javier Fernandez
Comment 11 2013-10-30 04:36:30 PDT
Created attachment 215496 [details] Patch
Zan Dobersek
Comment 12 2013-11-09 23:06:15 PST
(In reply to comment #11) > Created an attachment (id=215496) [details] > Patch Fix for bug #123931 introduced regressions on the GTK port because the port enables subpixel layout while still disabling the saturated layout arithmetic. Landing this patch is the ideal fix, so any reviews would be greatly appreciated.
Martin Robinson
Comment 13 2013-11-12 14:00:58 PST
Comment on attachment 215496 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=215496&action=review > Source/WTF/wtf/FeatureDefines.h:294 > +#if !defined(ENABLE_SATURATED_LAYOUT_ARITHMETIC) > +#define ENABLE_SATURATED_LAYOUT_ARITHMETIC 1 > +#endif > + Let's just enable it via build-webkit and in SetupWebKitFeatures.m4 and avoiding changing this file for now. Hopefully we can remove the flag shortly. > LayoutTests/fast/css/max-height-huge-value-expected.html:2 > +<html lang="en"> Do you need the lang tag here? > LayoutTests/fast/css/max-height-huge-value-expected.html:10 > + width: 100px; > + border: 2px solid blue; > + overflow: hidden; > + max-height: 20000000px; > + } Bit of a nit, but this indentation is irregular. > LayoutTests/platform/gtk/TestExpectations:-1384 > -webkit.org/b/118595 fast/regions/auto-size/autoheight-correct-region-for-lines-2.html [ ImageOnlyFailure ] > - Does this test already cover the failure. If so, you may not need a new test.
Javier Fernandez
Comment 14 2013-12-11 09:08:28 PST
Created attachment 218970 [details] Patch Removed new test and applied suggested changes.
WebKit Commit Bot
Comment 15 2013-12-11 09:50:43 PST
Comment on attachment 218970 [details] Patch Clearing flags on attachment: 218970 Committed r160441: <http://trac.webkit.org/changeset/160441>
WebKit Commit Bot
Comment 16 2013-12-11 09:50:47 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.