Bug 119273

Summary:

Arithmetic overflow when computing max-height CSS property with subpixel layout

Product:

WebKit

Reporter:

Javier Fernandez <jfernandez>

Component:

CSS

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

benjamin, bjonesbe, cmarcelo, commit-queue, eflews.bot, esprehn+autocc, glenn, gyuyoung.kim, jfernandez, kondapallykalyan, mihnea, sam, WebkitBugTracker, zalan, zan

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

Bug Blocks:

118595, 124234

Attachments:

Description	Flags
Test to verify the issue.	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none

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	Details Formatted Diff Diff
Patch (5.62 KB, patch) 2013-10-30 03:46 PDT, Javier Fernandez	no flags	Details Formatted Diff Diff
Patch (5.85 KB, patch) 2013-10-30 03:52 PDT, Javier Fernandez	no flags	Details Formatted Diff Diff
Patch (5.78 KB, patch) 2013-10-30 04:17 PDT, Javier Fernandez	no flags	Details Formatted Diff Diff
Patch (5.87 KB, patch) 2013-10-30 04:36 PDT, Javier Fernandez	no flags	Details Formatted Diff Diff
Patch (3.43 KB, patch) 2013-12-11 09:08 PST, Javier Fernandez	no flags	Details Formatted Diff Diff
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.