Bug 285429 – Release assertion failure in LegacyRenderSVGShape::strokeBoundingBox()

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Ryosuke Niwa]

Created attachment 473802 [details]
Test case (reduced by Said)

e.g.

ASSERTION FAILED: bool(*this)
/Volumes/Data/safari-2/OpenSource/WebKitBuild/Debug/usr/local/include/wtf/Markable.h(146) : T &WTF::Markable<WebCore::FloatRect, WebCore::FloatRect::MarkableTraits>::operator*() & [T = WebCore::FloatRect, Traits = WebCore::FloatRect::MarkableTraits]
1   0x3071aa1dc WTF::Markable<WebCore::FloatRect, WebCore::FloatRect::MarkableTraits>::operator*() &
2   0x30725499c WebCore::LegacyRenderSVGShape::strokeBoundingBox() const
3   0x307230d78 WebCore::LegacyRenderSVGPath::updateShapeFromElement()
4   0x3072533c0 WebCore::LegacyRenderSVGShape::layout()
5   0x3071f99ac WebCore::SVGRenderSupport::layoutChildren(WebCore::RenderElement&, bool)
6   0x307250498 WebCore::LegacyRenderSVGRoot::layout()
7   0x306e4f1d4 WebCore::RenderElement::layoutIfNeeded()
8   0x305b16858 WebCore::LayoutIntegration::layoutWithFormattingContextForBox(WebCore::Layout::ElementBox const&, std::__1::optional<WebCore::LayoutUnit>, WebCore::Layout::LayoutState&)
9   0x30709b07c WTF::Detail::CallableWrapper<void (*)(WebCore::Layout::ElementBox const&, std::__1::optional<WebCore::LayoutUnit>, WebCore::Layout::LayoutState&), void, WebCore::Layout::ElementBox const&, std::__1::optional<WebCore::LayoutUnit>, WebCore::Layout::LayoutState&>::call(WebCore::Layout::ElementBox const&, std::__1::optional<WebCore::LayoutUnit>, WebCore::Layout::LayoutState&)
10  0x3059ae1d4 WTF::Function<void (WebCore::Layout::ElementBox const&, std::__1::optional<WebCore::LayoutUnit>, WebCore::Layout::LayoutState&)>::operator()(WebCore::Layout::ElementBox const&, std::__1::optional<WebCore::LayoutUnit>, WebCore::Layout::LayoutState&) const
11  0x3059ae0f0 WebCore::Layout::LayoutState::layoutWithFormattingContextForBox(WebCore::Layout::ElementBox const&, std::__1::optional<WebCore::LayoutUnit>) const
12  0x305b16e10 WebCore::Layout::IntegrationUtils::layoutWithFormattingContextForBox(WebCore::Layout::ElementBox const&, std::__1::optional<WebCore::LayoutUnit>) const
13  0x305a72fb4 WebCore::Layout::LineBuilder::candidateContentForLine(WebCore::Layout::LineCandidate&, unsigned long, WebCore::Layout::InlineItemRange const&, float)
14  0x305a71b70 WebCore::Layout::LineBuilder::placeInlineAndFloatContent(WebCore::Layout::InlineItemRange const&)::$_1::operator()() const
15  0x305a705f8 WebCore::Layout::LineBuilder::placeInlineAndFloatContent(WebCore::Layout::InlineItemRange const&)
16  0x305a6fb24 WebCore::Layout::LineBuilder::layoutInlineContent(WebCore::Layout::LineInput const&, std::__1::optional<WebCore::Layout::PreviousLine> const&)
17  0x305a251d0 WebCore::Layout::InlineFormattingContext::lineLayout(WebCore::Layout::AbstractLineBuilder&, WTF::Vector<WebCore::Layout::InlineItem, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc> const&, WebCore::Layout::InlineItemRange, std::__1::optional<WebCore::Layout::PreviousLine>, WebCore::Layout::ConstraintsForInlineContent const&, WebCore::Layout::InlineDamage const*)
18  0x305a24224 WebCore::Layout::InlineFormattingContext::layout(WebCore::Layout::ConstraintsForInlineContent const&, WebCore::Layout::InlineDamage*)
19  0x305b36e34 WebCore::LayoutIntegration::LineLayout::layout()
20  0x306db845c WebCore::RenderBlockFlow::layoutInlineContent(bool, WebCore::LayoutUnit&, WebCore::LayoutUnit&)
21  0x306db5b88 WebCore::RenderBlockFlow::layoutInlineChildren(bool, WebCore::LayoutUnit&, WebCore::LayoutUnit&)
22  0x306db43cc WebCore::RenderBlockFlow::layoutInFlowChildren(bool, WebCore::LayoutUnit&, WebCore::LayoutUnit&, WebCore::LayoutUnit&)
23  0x306db314c WebCore::RenderBlockFlow::layoutBlock(bool, WebCore::LayoutUnit)
24  0x306d933dc WebCore::RenderBlock::layout()
25  0x306db6cc8 WebCore::RenderBlockFlow::layoutBlockChild(WebCore::RenderBox&, WebCore::RenderBlockFlow::MarginInfo&, WebCore::LayoutUnit&, WebCore::LayoutUnit&)
26  0x306db5f90 WebCore::RenderBlockFlow::layoutBlockChildren(bool, WebCore::LayoutUnit&)
27  0x306db4414 WebCore::RenderBlockFlow::layoutInFlowChildren(bool, WebCore::LayoutUnit&, WebCore::LayoutUnit&, WebCore::LayoutUnit&)
28  0x306db314c WebCore::RenderBlockFlow::layoutBlock(bool, WebCore::LayoutUnit)
29  0x306d933dc WebCore::RenderBlock::layout()

<rdar://141024421>

[Ryosuke Niwa]

Said's analysis:
> This is a RELEASE_ASSERT which happens in strokeBoundingBox() when accessing the value of m_strokeBoundingBox
> which is of type Markable. In this scenario, m_strokeBoundingBox is all NaN which isEmptyValue() for FloatRect.
>
> The real cause of this bug is setting the y value of the filter element to 1 in this script:
> 
> ```
> let length = x21.y.baseVal;
> length.value = 1;
> ```
>
> Because the m_lengthType of the filter y is always created as a percentage length
> 
> ```
>  Ref<SVGAnimatedLength> m_y { SVGAnimatedLength::create(this, SVGLengthMode::Height, "-10%"_s) };
> ```
>
> Setting its value to number requires converting this number to percentage.
> See SVGLengthValue::setValue() and SVGLengthContext::convertValueFromUserUnitsToPercentage().
> To do this conversion we have to get the element height. But the filter height is zero,
> we just end up dividing by zero and hence the new y value is NaN.

[Rob Buis]

I can confirm the stated behaviour and agree with Said's analysis after debugging the problem.

[Rob Buis]

There is no need for html to trigger a crash, I.e. a standalone SVG shows the problem as well:

```
<svg width="0" height="0" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink">
  <filter id="filter"></filter>
  <marker id="marker" orient="1" filter="url(#filter)"></marker>
  <line marker-end="url(#marker)"></line>
  <script>
    let filter = document.getElementById("filter");
    let length = filter.x.baseVal;
    length.value = 1;
  </script>
</svg>
```

[Rob Buis]

Pull request: https://github.com/WebKit/WebKit-security/pull/76

[Rob Buis]

Note that chromium has had a fix for the same problem (https://issues.chromium.org/issues/41173256).

[Ryosuke Niwa]

Is this a security bug??? Isn't it just a regular zero division?

[Rob Buis]

Pull request: https://github.com/WebKit/WebKit/pull/39684

[Rob Buis]

A bit more complete bt I see locally:
1   0x306e08e84 WTF::Markable<WebCore::FloatRect, WebCore::FloatRect::MarkableTraits>::operator*() &
2   0x306e83514 WebCore::LegacyRenderSVGContainer::repaintRectInLocalCoordinates(WebCore::RepaintRectCalculation) const
3   0x306ea5210 WebCore::LegacyRenderSVGResourceMarker::markerBoundaries(WebCore::RepaintRectCalculation, WebCore::AffineTransform const&) const
4   0x306e8b338 WebCore::LegacyRenderSVGPath::markerRect(WebCore::RepaintRectCalculation, float) const
5   0x306e8af78 WebCore::LegacyRenderSVGPath::adjustStrokeBoundingBoxForMarkersAndZeroLengthLinecaps(WebCore::RepaintRectCalculation, WebCore::FloatRect) const
6   0x306eae7ac WebCore::LegacyRenderSVGShape::calculateStrokeBoundingBox() const
7   0x306eae528 WebCore::LegacyRenderSVGShape::strokeBoundingBox() const
8   0x306e8ad24 WebCore::LegacyRenderSVGPath::updateShapeFromElement()
.....

[EWS]

Committed 289664@main (6866936923f7): <https://commits.webkit.org/289664@main>

Reviewed commits have been landed. Closing PR #39684 and removing active labels.

[EWS]

Committed 289651.248@safari-7621-branch (660da3e4b5cc): <https://commits.webkit.org/289651.248@safari-7621-branch>

Reviewed commits have been landed. Closing PR #2767 and removing active labels.