Bug 88820 – Padding and borders can cause integer overflow in block layouts

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Vicki Pfau]

Created attachment 146968 [details]
Test case

With some display type combinations, blocks set to be 100% wide combined with padding or borders can cause an integer overflow and make the blocks be rounded to 0 pixels wide. Test case attached.

<rdar://problem/11328762>

[Vicki Pfau]

Created attachment 146977 [details]
Patch

Here's a patch, but in the case that m_*PreferredLogicalWidth is negative, it will fail. Does anyone know if either of them can be negative?

[Darin Adler]

Comment on attachment 146977 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=146977&action=review

Looks good. I wish for a slightly clearer idiom for addition with pinning, maybe a helper function, but this does seem OK as is.

> LayoutTests/fast/block/block-size-integer-overflow.html:14
> +    window.layoutTestController.dumpAsText();

Since this is a dumpAsText test, it would be nice if the test itself explained that it was testing, how, and why, with a bit of text in the page.

[Tony Chang]

Comment on attachment 146977 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=146977&action=review

> LayoutTests/fast/block/block-size-integer-overflow.html:31
> +<div style="display: -webkit-box; -webkit-box-orient: horizontal">
> +  <div style="padding-left: 1px">
> +    <div class="fail">FA</div>
> +    <div id="spacer" style="color: green; width: 100%; background-color: green"></div>
> +    <div class="fail">IL</div>
> +  </div>

Why does this trigger the bug?  A width of 100% should only be around 800px in this example.  Is it specific to table-cell?

> Source/WebCore/rendering/RenderBlock.cpp:5327
> -    m_minPreferredLogicalWidth += borderAndPadding;
> -    m_maxPreferredLogicalWidth += borderAndPadding;
> +    m_minPreferredLogicalWidth = (MAX_LAYOUT_UNIT - m_minPreferredLogicalWidth) > borderAndPadding ? m_minPreferredLogicalWidth + borderAndPadding : MAX_LAYOUT_UNIT;
> +    m_maxPreferredLogicalWidth = (MAX_LAYOUT_UNIT - m_maxPreferredLogicalWidth) > borderAndPadding ? m_maxPreferredLogicalWidth + borderAndPadding : MAX_LAYOUT_UNIT;

The same code is in RenderDeprecatedFlexibleBox.cpp and RenderFlexibleBox.cpp.  Do we need to update those as well?

[Vicki Pfau]

(In reply to comment #3)
> (From update of attachment 146977 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=146977&action=review
> 
> > LayoutTests/fast/block/block-size-integer-overflow.html:31
> > +<div style="display: -webkit-box; -webkit-box-orient: horizontal">
> > +  <div style="padding-left: 1px">
> > +    <div class="fail">FA</div>
> > +    <div id="spacer" style="color: green; width: 100%; background-color: green"></div>
> > +    <div class="fail">IL</div>
> > +  </div>
> 
> Why does this trigger the bug?  A width of 100% should only be around 800px in this example.  Is it specific to table-cell?

When width is 100%, the preferred width is MAX_LAYOUT_UNIT, which is INT_MAX. This gets scaled down to 800px later.

> 
> > Source/WebCore/rendering/RenderBlock.cpp:5327
> > -    m_minPreferredLogicalWidth += borderAndPadding;
> > -    m_maxPreferredLogicalWidth += borderAndPadding;
> > +    m_minPreferredLogicalWidth = (MAX_LAYOUT_UNIT - m_minPreferredLogicalWidth) > borderAndPadding ? m_minPreferredLogicalWidth + borderAndPadding : MAX_LAYOUT_UNIT;
> > +    m_maxPreferredLogicalWidth = (MAX_LAYOUT_UNIT - m_maxPreferredLogicalWidth) > borderAndPadding ? m_maxPreferredLogicalWidth + borderAndPadding : MAX_LAYOUT_UNIT;
> 
> The same code is in RenderDeprecatedFlexibleBox.cpp and RenderFlexibleBox.cpp.  Do we need to update those as well?

It's possible. I should probably take a look at those too, to see if anything can come in as INT_MAX.

[Vicki Pfau]

(In reply to comment #4)
> (In reply to comment #3)
> > (From update of attachment 146977 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=146977&action=review
> > 
> > > LayoutTests/fast/block/block-size-integer-overflow.html:31
> > > +<div style="display: -webkit-box; -webkit-box-orient: horizontal">
> > > +  <div style="padding-left: 1px">
> > > +    <div class="fail">FA</div>
> > > +    <div id="spacer" style="color: green; width: 100%; background-color: green"></div>
> > > +    <div class="fail">IL</div>
> > > +  </div>
> > 
> > Why does this trigger the bug?  A width of 100% should only be around 800px in this example.  Is it specific to table-cell?
> 
> When width is 100%, the preferred width is MAX_LAYOUT_UNIT, which is INT_MAX. This gets scaled down to 800px later.
> 

Specifically, width of 100% is MAX_LAYOUT_UNIT when used in conjunction with AutoTableLayout. In other cases, it's not. This bug is only likely to come up when AutoTableLayout is in use. I'm not 100% sure what uses AutoTableLayout, though. It's possible that this could come up if people specify absurdly large (e.g. close to INT_MAX) block widths.

Actually, looking back at this, the line that sets it to MAX_LAYOUT_UNIT is prefixed by this comment:
// if there was no remaining percent, maxWidth is invalid
So it makes me wonder if we're doing the right thing here anyway. (This is in AutoTableLayout::computePreferredLogicalWidths)

[Tony Chang]

(In reply to comment #5)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > (From update of attachment 146977 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=146977&action=review
> > > 
> > > > LayoutTests/fast/block/block-size-integer-overflow.html:31
> > > > +<div style="display: -webkit-box; -webkit-box-orient: horizontal">
> > > > +  <div style="padding-left: 1px">
> > > > +    <div class="fail">FA</div>
> > > > +    <div id="spacer" style="color: green; width: 100%; background-color: green"></div>
> > > > +    <div class="fail">IL</div>
> > > > +  </div>
> > > 
> > > Why does this trigger the bug?  A width of 100% should only be around 800px in this example.  Is it specific to table-cell?
> > 
> > When width is 100%, the preferred width is MAX_LAYOUT_UNIT, which is INT_MAX. This gets scaled down to 800px later.
> > 
> 
> Specifically, width of 100% is MAX_LAYOUT_UNIT when used in conjunction with AutoTableLayout. In other cases, it's not. This bug is only likely to come up when AutoTableLayout is in use. I'm not 100% sure what uses AutoTableLayout, though. It's possible that this could come up if people specify absurdly large (e.g. close to INT_MAX) block widths.

Ah, thanks, I see the code in AutoTableLayout.cpp now.  It looks like only max preferred width is set to MAX_LAYOUT_UNIT.  Should we remove the check on min?

I'll see if I can trigger an overflow in some of the flexbox cases.

> Actually, looking back at this, the line that sets it to MAX_LAYOUT_UNIT is prefixed by this comment:
> // if there was no remaining percent, maxWidth is invalid
> So it makes me wonder if we're doing the right thing here anyway. (This is in AutoTableLayout::computePreferredLogicalWidths)

Adding Julien who may know something about this class.

[Vicki Pfau]

(In reply to comment #6)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > (In reply to comment #3)
> > > > (From update of attachment 146977 [details] [details] [details] [details])
> > > > View in context: https://bugs.webkit.org/attachment.cgi?id=146977&action=review
> > > > 
> > > > > LayoutTests/fast/block/block-size-integer-overflow.html:31
> > > > > +<div style="display: -webkit-box; -webkit-box-orient: horizontal">
> > > > > +  <div style="padding-left: 1px">
> > > > > +    <div class="fail">FA</div>
> > > > > +    <div id="spacer" style="color: green; width: 100%; background-color: green"></div>
> > > > > +    <div class="fail">IL</div>
> > > > > +  </div>
> > > > 
> > > > Why does this trigger the bug?  A width of 100% should only be around 800px in this example.  Is it specific to table-cell?
> > > 
> > > When width is 100%, the preferred width is MAX_LAYOUT_UNIT, which is INT_MAX. This gets scaled down to 800px later.
> > > 
> > 
> > Specifically, width of 100% is MAX_LAYOUT_UNIT when used in conjunction with AutoTableLayout. In other cases, it's not. This bug is only likely to come up when AutoTableLayout is in use. I'm not 100% sure what uses AutoTableLayout, though. It's possible that this could come up if people specify absurdly large (e.g. close to INT_MAX) block widths.
> 
> Ah, thanks, I see the code in AutoTableLayout.cpp now.  It looks like only max preferred width is set to MAX_LAYOUT_UNIT.  Should we remove the check on min?
> 
> I'll see if I can trigger an overflow in some of the flexbox cases.

After looking over and stepping through the code, I think only RenderBox needs to be changed, and the check on min can be removed. It looks like this only happens when RenderBox::sizesToIntrinsicLogicalWidth returns true during RenderBlock::computeLogicalWidth occurs. This causes the intrinsic width to get calculated and then overflown when padding are added on. I am seeing other places that might overflow, though.

[Tony Chang]

Created attachment 147180 [details]
overflow from margin

Here's another test case which overflows RenderBlock.cpp:5761 (w = childMaxPreferredLogicalWidth + margin;).

I suspect any time we try to add to m_maxPreferredLogicalWidth, we can possibly overflow. One option would be to add a helper function for adding to maxPreferredLogicalWidth and try to update all callers to use it.

We could also try to make LayoutUnit smart and have it detect when it's going to overflow.  That might have a runtime cost since it would impact all adds.

Or maybe the code in AutoTableLayout is fishy?  Maybe there's some way to produce the same layout without using MAX_LAYOUT_UNIT? If a cell has a percentage width, can we calculate the size based on the table width?

[Tony Chang]

Created attachment 147182 [details]
overflow in deprecated flexbox

Here's an example of overflowing RenderDeprecatedFlexibleBox.cpp:225.  Changing display: -webkit-box to display: -webkit-flex would overflow RenderFlexibleBox.cpp.

[Tony Chang]

I think this is a regression from https://bugs.webkit.org/show_bug.cgi?id=56052 . See comment #7 where Hyatt asks if there's a layout test for this logic change.

Looking in AutoTableLayout.cpp, there are other cases where we clamp the value of maxWidth to MAX_LAYOUT_UNIT / 2.0 in the same function.  I think we should do that here.  This doesn't appear to break any of the table layout tests for me.

[Vicki Pfau]

(In reply to comment #10)
> Looking in AutoTableLayout.cpp, there are other cases where we clamp the value of maxWidth to MAX_LAYOUT_UNIT / 2.0 in the same function.  I think we should do that here.  This doesn't appear to break any of the table layout tests for me.

I'm a bit suspicious of clamping it to MAX_LAYOUT_UNIT / 2.0f. It seems a bit like a hack, and the fact that we're using f means that we lose some bits to the mantissa (or, depending on how it rounds, we only lose one bit, but the value increases). And although I highly doubt that anyone will set any of the dimensions to anywhere near MAX_LAYOUT_UNIT (within several orders of magnitude), it still makes me wonder what will happen if some of that addition can overflow.

It will prevent the trivial cases, which are demonstrated in these repros, from breaking, though.

[Vicki Pfau]

(In reply to comment #8)
> Or maybe the code in AutoTableLayout is fishy?  Maybe there's some way to produce the same layout without using MAX_LAYOUT_UNIT? If a cell has a percentage width, can we calculate the size based on the table width?

I looked into this, and that's how width is generally calculated, except that the codepath that's gone down asks the table cell how wide it WANTS to be, if it doesn't care about how wide anything is. As such, the available size isn't passed down, and we can't find it because the operation is supposed to be independent of it.

[Julien Chaffraix]

> Maybe there's some way to produce the same layout without using MAX_LAYOUT_UNIT? 

I think Tony's proposal would work. We don't care about the exact value, it just has to be arbitrary large. The code already uses a cap of MAX_LAYOUT_UNIT / 2.0f as a magic constant for the percent case so it would just be an extension of that.

As a side note, several other places are using the magic constant of 15,000 to represent the max width (BLOCK_MAX_WIDTH, TABLE_MAX_WIDTH (for fixed table layout) and cLargeLogicalWidth for RenderMathMLBlock). I would support going with this value as it makes the 2 table layout more consistent and alleviate the risk of overflow by a bigger margin.

[Ojan Vafai]

(In reply to comment #13)
> > Maybe there's some way to produce the same layout without using MAX_LAYOUT_UNIT? 
> 
> I think Tony's proposal would work. We don't care about the exact value, it just has to be arbitrary large. The code already uses a cap of MAX_LAYOUT_UNIT / 2.0f as a magic constant for the percent case so it would just be an extension of that.
> 
> As a side note, several other places are using the magic constant of 15,000 to represent the max width (BLOCK_MAX_WIDTH, TABLE_MAX_WIDTH (for fixed table layout) and cLargeLogicalWidth for RenderMathMLBlock). I would support going with this value as it makes the 2 table layout more consistent and alleviate the risk of overflow by a bigger margin.

I think the smaller magic constant is the way to go. 15,000 seems good enough. Another case where MAX/2 wouldn't work is calculating the preferred width of a horizontal flexbox containing 100% width table cells. The preferred width of a horizontal flexbox is the sum of it's flex-items' preferred widths. So, if you had 3 items, you'd overflow again. With 15,000, you'd need a ridiculous number of items before we hit this case.

In short, good enough.

[Vicki Pfau]

Created attachment 147398 [details]
Patch

[Tony Chang]

Comment on attachment 147398 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=147398&action=review

> Source/WebCore/rendering/AutoTableLayout.cpp:224
> +// Use a very large value (in effect infinite). But not too large!
> +// Keep this in synch with BLOCK_MAX_WIDTH in RenderBlock.cpp and TABLE_MAX_WIDTH in FixedTableLayout.cpp
> +#define TABLE_MAX_WIDTH 15000

Can you remove BLOCK_MAX_WIDTH from RenderBlock.cpp?  It doesn't look like it's used anymore.  Then you can update the comment here and in FixedTableLayout.cpp.  We might just want to somehow share the value between FixedTableLayout.cpp and AutoTableLayout.cpp (maybe declare a static in TableLayout.h and instantiate it in RenderTable.cpp?), but that seems less important.

[Julien Chaffraix]

Comment on attachment 147398 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=147398&action=review

>> Source/WebCore/rendering/AutoTableLayout.cpp:224
>> +#define TABLE_MAX_WIDTH 15000
> 
> Can you remove BLOCK_MAX_WIDTH from RenderBlock.cpp?  It doesn't look like it's used anymore.  Then you can update the comment here and in FixedTableLayout.cpp.  We might just want to somehow share the value between FixedTableLayout.cpp and AutoTableLayout.cpp (maybe declare a static in TableLayout.h and instantiate it in RenderTable.cpp?), but that seems less important.

I would argue that moving the #define to TableLayout.h is the way to go. That would avoid any duplication and unneeded sync'ing between the 2 values.

Also this could be turned into a proper constant (this is really a nit).

[Vicki Pfau]

Created attachment 147414 [details]
Patch

[Vicki Pfau]

Committed r120257: <http://trac.webkit.org/changeset/120257>