Bug 220352 – Nullptr crash in RasterShape::marginIntervals()

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Ryosuke Niwa]

Created attachment 417070 [details]
Test

e.g.

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.WebCore                   0x00007fff3b64cecd WebCore::RasterShape::marginIntervals() const + 1837
1   com.apple.WebCore                   0x00007fff3b6588c9 WebCore::RasterShape::shapeMarginLogicalBoundingBox() const + 9
2   com.apple.WebCore                   0x00007fff3b650a38 WebCore::ShapeOutsideInfo::computeDeltasForContainingBlockLine(WebCore::RenderBlockFlow const&, WebCore::FloatingObject const&, WebCore::LayoutUnit, WebCore::LayoutUnit) + 280
3   com.apple.WebCore                   0x00007fff3b62cf47 WebCore::LineWidth::shrinkAvailableWidthForNewFloatIfNeeded(WebCore::FloatingObject const&) + 455
4   com.apple.WebCore                   0x00007fff3b4a6fdc WebCore::ComplexLineLayout::positionNewFloatOnLine(WebCore::FloatingObject const&, WebCore::FloatingObject*, WebCore::LineInfo&, WebCore::LineWidth&) + 60
5   com.apple.WebCore                   0x00007fff3b625827 WebCore::LineBreaker::nextLineBreak(WebCore::BidiResolverWithIsolate<WebCore::InlineIterator, WebCore::BidiRun, WebCore::BidiIsolatedRun>&, WebCore::LineInfo&, WebCore::RenderTextInfo&, WebCore::FloatingObject*, unsigned int, WTF::Vector<WebCore::WordMeasurement, 64ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>&) + 951
6   com.apple.WebCore                   0x00007fff3b49c632 WebCore::ComplexLineLayout::layoutRunsAndFloatsInRange(WebCore::LineLayoutState&, WebCore::BidiResolverWithIsolate<WebCore::InlineIterator, WebCore::BidiRun, WebCore::BidiIsolatedRun>&, WebCore::InlineIterator const&, WebCore::BidiStatus const&, unsigned int) + 1762
7   com.apple.WebCore                   0x00007fff3b4a4dcd WebCore::ComplexLineLayout::layoutLineBoxes(bool, WebCore::LayoutUnit&, WebCore::LayoutUnit&) + 11645
8   com.apple.WebCore                   0x00007fff3b4ebd88 WebCore::RenderBlockFlow::layoutBlock(bool, WebCore::LayoutUnit) + 2312
9   com.apple.WebCore                   0x00007fff398b439a WebCore::RenderBlock::layout() + 218
10  com.apple.WebCore                   0x00007fff3b4f0eab WebCore::RenderBlockFlow::insertFloatingObject(WebCore::RenderBox&) + 651
11  com.apple.WebCore                   0x00007fff3b62580b WebCore::LineBreaker::nextLineBreak(WebCore::BidiResolverWithIsolate<WebCore::InlineIterator, WebCore::BidiRun, WebCore::BidiIsolatedRun>&, WebCore::LineInfo&, WebCore::RenderTextInfo&, WebCore::FloatingObject*, unsigned int, WTF::Vector<WebCore::WordMeasurement, 64ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>&) + 923
12  com.apple.WebCore                   0x00007fff3b49c632 WebCore::ComplexLineLayout::layoutRunsAndFloatsInRange(WebCore::LineLayoutState&, WebCore::BidiResolverWithIsolate<WebCore::InlineIterator, WebCore::BidiRun, WebCore::BidiIsolatedRun>&, WebCore::InlineIterator const&, WebCore::BidiStatus const&, unsigned int) + 1762
13  com.apple.WebCore                   0x00007fff3b4a4dcd WebCore::ComplexLineLayout::layoutLineBoxes(bool, WebCore::LayoutUnit&, WebCore::LayoutUnit&) + 11645
14  com.apple.WebCore                   0x00007fff3b4ebd88 WebCore::RenderBlockFlow::layoutBlock(bool, WebCore::LayoutUnit) + 2312
15  com.apple.WebCore                   0x00007fff398b439a WebCore::RenderBlock::layout() + 218

<rdar://problem/72101811>

[alan]

Accumulated zoom ends up being computed to INF which makes the RenderStyle::shapeMargin NAN. We could fix it either at the style builder level or at ShapeOutsideInfo::computedShape ->


diff --git a/Source/WebCore/rendering/shapes/ShapeOutsideInfo.cpp b/Source/WebCore/rendering/shapes/ShapeOutsideInfo.cpp
index 723f1d561f77..2fa330370671 100644
--- a/Source/WebCore/rendering/shapes/ShapeOutsideInfo.cpp
+++ b/Source/WebCore/rendering/shapes/ShapeOutsideInfo.cpp
@@ -170,7 +170,10 @@ const Shape& ShapeOutsideInfo::computedShape() const
     const RenderStyle& containingBlockStyle = m_renderer.containingBlock()->style();
 
     WritingMode writingMode = containingBlockStyle.writingMode();
-    float margin = floatValueForLength(m_renderer.style().shapeMargin(), m_renderer.containingBlock() ? m_renderer.containingBlock()->contentWidth() : 0_lu);
+    auto margin = [&] {
+        auto shapeMargin = floatValueForLength(m_renderer.style().shapeMargin(), m_renderer.containingBlock() ? m_renderer.containingBlock()->contentWidth() : 0_lu);
+        return !isnan(shapeMargin) ? shapeMargin : 0.0f;
+    }();
     float shapeImageThreshold = style.shapeImageThreshold();
     const ShapeValue& shapeValue = *style.shapeOutside();

[Rob Buis]

Created attachment 417454 [details]
Patch

[Ryosuke Niwa]

Is this a security problem or just a nullptr crash?

[Rob Buis]

(In reply to Ryosuke Niwa from comment #3)
> Is this a security problem or just a nullptr crash?

I do not think it is a security problem but a logic/assertion error:

const RasterShapeIntervals& RasterShape::marginIntervals() const
{
    ASSERT(shapeMargin() >= 0);
    if (!shapeMargin())
        return *m_intervals;

With zalan's suggested fix we do not hit this ASSERT anymore. In release there still should be not be a crash for negative shape margin, even before this patch.

[Ryosuke Niwa]

(In reply to Rob Buis from comment #4)
> (In reply to Ryosuke Niwa from comment #3)
> > Is this a security problem or just a nullptr crash?
> 
> I do not think it is a security problem but a logic/assertion error:
> 
> const RasterShapeIntervals& RasterShape::marginIntervals() const
> {
>     ASSERT(shapeMargin() >= 0);
>     if (!shapeMargin())
>         return *m_intervals;
> 
> With zalan's suggested fix we do not hit this ASSERT anymore. In release
> there still should be not be a crash for negative shape margin, even before
> this patch.

Alan says we'd use this later to resize Vector. Are we sure we don't end up allocating with 0 or -1 length there?

[Rob Buis]

(In reply to Ryosuke Niwa from comment #5)
> (In reply to Rob Buis from comment #4)
> > (In reply to Ryosuke Niwa from comment #3)
> > > Is this a security problem or just a nullptr crash?
> > 
> > I do not think it is a security problem but a logic/assertion error:
> > 
> > const RasterShapeIntervals& RasterShape::marginIntervals() const
> > {
> >     ASSERT(shapeMargin() >= 0);
> >     if (!shapeMargin())
> >         return *m_intervals;
> > 
> > With zalan's suggested fix we do not hit this ASSERT anymore. In release
> > there still should be not be a crash for negative shape margin, even before
> > this patch.
> 
> Alan says we'd use this later to resize Vector. Are we sure we don't end up
> allocating with 0 or -1 length there?

I think it is ok, if margin is zero, then m_intervals is used, which is based on the unchanged marginRect, and m_marginIntervals is not computed or used.

[EWS]

Committed r271738: <https://trac.webkit.org/changeset/271738>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 417454 [details].