Bug 220352

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();

Created attachment 417454 [details]
Patch

Is this a security problem or just a nullptr crash?

(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.

(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?

(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.

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

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