Bug 220352 - Nullptr crash in RasterShape::marginIntervals()
Summary: Nullptr crash in RasterShape::marginIntervals()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-01-05 22:39 PST by Ryosuke Niwa
Modified: 2021-01-21 22:56 PST (History)
15 users (show)

See Also:


Attachments
Test (204 bytes, text/html)
2021-01-05 22:39 PST, Ryosuke Niwa
no flags Details
Patch (3.80 KB, patch)
2021-01-12 07:36 PST, Rob Buis
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2021-01-05 22:39:21 PST
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>
Comment 1 zalan 2021-01-09 09:56:13 PST
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();
Comment 2 Rob Buis 2021-01-12 07:36:07 PST
Created attachment 417454 [details]
Patch
Comment 3 Ryosuke Niwa 2021-01-13 19:44:51 PST
Is this a security problem or just a nullptr crash?
Comment 4 Rob Buis 2021-01-14 06:57:59 PST
(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.
Comment 5 Ryosuke Niwa 2021-01-14 22:56:48 PST
(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?
Comment 6 Rob Buis 2021-01-15 12:17:15 PST
(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.
Comment 7 EWS 2021-01-21 22:23:02 PST
Committed r271738: <https://trac.webkit.org/changeset/271738>

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