220352 2021-01-05 22:39:21 -0800 Nullptr crash in RasterShape::marginIntervals() 2021-01-21 22:56:03 -0800 1 1 1 Unclassified WebKit Layout and Rendering WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 rniwa webkit-unassigned bfulgham cgarcia dino ews-feeder fred.wang gpoo koivisto mmaxfield product-security rbuis simon.fraser svillar webkit-bug-importer wenson_hsieh zalan oldest_to_newest 1717594 0 417070 rniwa 2021-01-05 22:39:21 -0800 Created attachment 417070 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> 1718572 1 zalan 2021-01-09 09:56:13 -0800 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(); 1718986 2 417454 rbuis 2021-01-12 07:36:07 -0800 Created attachment 417454 Patch 1719521 3 rniwa 2021-01-13 19:44:51 -0800 Is this a security problem or just a nullptr crash? 1719587 4 rbuis 2021-01-14 06:57:59 -0800 (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. 1719789 5 rniwa 2021-01-14 22:56:48 -0800 (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? 1719942 6 rbuis 2021-01-15 12:17:15 -0800 (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. 1721491 7 ews-feeder 2021-01-21 22:23:02 -0800 Committed r271738: <https://trac.webkit.org/changeset/271738> All reviewed patches have been landed. Closing bug and clearing flags on attachment 417454. 417070 2021-01-05 22:39:21 -0800 2021-01-05 22:39:21 -0800 Test repro_450.html text/html 204 rniwa PHN0eWxlPgogICogewogICAgZGlzcGxheTogaW5saW5lOwogICAgZmlsdGVyOiBkcm9wLXNoYWRv dygwIDAgMCBncmVlbik7CiAgICBmbG9hdDogbGVmdDsKICAgIHNoYXBlLW1hcmdpbjogMDsKICAg IHNoYXBlLW91dHNpZGU6IHJhZGlhbC1ncmFkaWVudChncmVlbiAwLCBncmVlbiAwKTsKICAgIHpv b206IDQxMjM0NTY3ODkwMTIzOwogIH0KPC9zdHlsZT4K 417454 2021-01-12 07:36:07 -0800 2021-01-21 22:23:03 -0800 Patch bug-220352-20210112163606.patch text/plain 3888 rbuis U3VidmVyc2lvbiBSZXZpc2lvbjogMjcxNDAwCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggOTBhZjdlN2VhMjMzMWEz NDZlMGNkMzZlN2NjZTBhZmY5Mjc4OTBhMC4uZWYyZTIxZjFmZTViOGNhNTU4MGU2MzI0OGZiZDg2 M2JiOWY0ZDEzOCAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE4IEBACisyMDIxLTAxLTEyICBSb2Ig QnVpcyAgPHJidWlzQGlnYWxpYS5jb20+CisKKyAgICAgICAgSGFuZGxlIHNoYXBlTWFyZ2luIGJl Y29taW5nIE5hTgorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/ aWQ9MjIwMzUyCisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAg ICAgRm9yIGxhcmdlIHpvb20gZmFjdG9ycyBzaGFwZU1hcmdpbiBjYW4gYmVjb21lIE5hTiwgaW4g dGhhdCBjYXNlIHRyZWF0CisgICAgICAgIGl0IGFzIHplcm8uCisKKyAgICAgICAgVGVzdDogZmFz dC9zaGFwZXMvc2hhcGUtb3V0c2lkZS1mbG9hdHMvc2hhcGUtb3V0c2lkZS1sYXJnZS16b29tLmh0 bWwKKworICAgICAgICAqIHJlbmRlcmluZy9zaGFwZXMvU2hhcGVPdXRzaWRlSW5mby5jcHA6Cisg ICAgICAgIChXZWJDb3JlOjpTaGFwZU91dHNpZGVJbmZvOjpjb21wdXRlZFNoYXBlIGNvbnN0KToK KwogMjAyMS0wMS0xMiAgUGhpbGlwcGUgTm9ybWFuZCAgPHBub3JtYW5kQGlnYWxpYS5jb20+CiAK ICAgICAgICAgW0dTdHJlYW1lcl0gQnVtcCB2ZXJzaW9uIHJlcXVpcmVtZW50CmRpZmYgLS1naXQg YS9Tb3VyY2UvV2ViQ29yZS9yZW5kZXJpbmcvc2hhcGVzL1NoYXBlT3V0c2lkZUluZm8uY3BwIGIv U291cmNlL1dlYkNvcmUvcmVuZGVyaW5nL3NoYXBlcy9TaGFwZU91dHNpZGVJbmZvLmNwcAppbmRl eCA3MjNmMWQ1NjFmNzc1OTM4ZjE5OGI2YjM2OGJiNTkyZDdhNTc5MGY3Li5jMWI4YzRhMzMyYjky ZWY2ODkwY2ZmZTZkMjY3NjZmYjlhZjJkODZlIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9y ZW5kZXJpbmcvc2hhcGVzL1NoYXBlT3V0c2lkZUluZm8uY3BwCisrKyBiL1NvdXJjZS9XZWJDb3Jl L3JlbmRlcmluZy9zaGFwZXMvU2hhcGVPdXRzaWRlSW5mby5jcHAKQEAgLTE3MCw3ICsxNzAsMTAg QEAgY29uc3QgU2hhcGUmIFNoYXBlT3V0c2lkZUluZm86OmNvbXB1dGVkU2hhcGUoKSBjb25zdAog ICAgIGNvbnN0IFJlbmRlclN0eWxlJiBjb250YWluaW5nQmxvY2tTdHlsZSA9IG1fcmVuZGVyZXIu Y29udGFpbmluZ0Jsb2NrKCktPnN0eWxlKCk7CiAKICAgICBXcml0aW5nTW9kZSB3cml0aW5nTW9k ZSA9IGNvbnRhaW5pbmdCbG9ja1N0eWxlLndyaXRpbmdNb2RlKCk7Ci0gICAgZmxvYXQgbWFyZ2lu ID0gZmxvYXRWYWx1ZUZvckxlbmd0aChtX3JlbmRlcmVyLnN0eWxlKCkuc2hhcGVNYXJnaW4oKSwg bV9yZW5kZXJlci5jb250YWluaW5nQmxvY2soKSA/IG1fcmVuZGVyZXIuY29udGFpbmluZ0Jsb2Nr KCktPmNvbnRlbnRXaWR0aCgpIDogMF9sdSk7CisgICAgYXV0byBtYXJnaW4gPSBbJl0geworICAg ICAgICBhdXRvIHNoYXBlTWFyZ2luID0gZmxvYXRWYWx1ZUZvckxlbmd0aChtX3JlbmRlcmVyLnN0 eWxlKCkuc2hhcGVNYXJnaW4oKSwgbV9yZW5kZXJlci5jb250YWluaW5nQmxvY2soKSA/IG1fcmVu ZGVyZXIuY29udGFpbmluZ0Jsb2NrKCktPmNvbnRlbnRXaWR0aCgpIDogMF9sdSk7CisgICAgICAg IHJldHVybiBpc25hbihzaGFwZU1hcmdpbikgPyAwLjBmIDogc2hhcGVNYXJnaW47CisgICAgfSgp OwogICAgIGZsb2F0IHNoYXBlSW1hZ2VUaHJlc2hvbGQgPSBzdHlsZS5zaGFwZUltYWdlVGhyZXNo b2xkKCk7CiAgICAgY29uc3QgU2hhcGVWYWx1ZSYgc2hhcGVWYWx1ZSA9ICpzdHlsZS5zaGFwZU91 dHNpZGUoKTsKIApkaWZmIC0tZ2l0IGEvTGF5b3V0VGVzdHMvQ2hhbmdlTG9nIGIvTGF5b3V0VGVz dHMvQ2hhbmdlTG9nCmluZGV4IDgwNzc0ZDA1NDc5MTk5ZDc5MTZlYzA2MDVjOTM4NmQwMDBkYTI4 MjIuLmM5YTIyMWZkYzA0ZDdiNzQyZmJkYzdjYjEyNmI2NmM1NmRkYTg2NmYgMTAwNjQ0Ci0tLSBh L0xheW91dFRlc3RzL0NoYW5nZUxvZworKysgYi9MYXlvdXRUZXN0cy9DaGFuZ2VMb2cKQEAgLTEs MyArMSwxNSBAQAorMjAyMS0wMS0xMiAgUm9iIEJ1aXMgIDxyYnVpc0BpZ2FsaWEuY29tPgorCisg ICAgICAgIEhhbmRsZSBzaGFwZU1hcmdpbiBiZWNvbWluZyBOYU4KKyAgICAgICAgaHR0cHM6Ly9i dWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTIyMDM1MgorCisgICAgICAgIFJldmlld2Vk IGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIEFkZCB0ZXN0IGZvciB0aGlzLgorCisgICAg ICAgICogZmFzdC9zaGFwZXMvc2hhcGUtb3V0c2lkZS1mbG9hdHMvc2hhcGUtb3V0c2lkZS1sYXJn ZS16b29tLWV4cGVjdGVkLnR4dDogQWRkZWQuCisgICAgICAgICogZmFzdC9zaGFwZXMvc2hhcGUt b3V0c2lkZS1mbG9hdHMvc2hhcGUtb3V0c2lkZS1sYXJnZS16b29tLmh0bWw6IEFkZGVkLgorCiAy MDIxLTAxLTEyICBQaGlsaXBwZSBOb3JtYW5kICA8cG5vcm1hbmRAaWdhbGlhLmNvbT4KIAogICAg ICAgICBVbnJldmlld2VkLCBHVEsgZ2FyZGVuaW5nCmRpZmYgLS1naXQgYS9MYXlvdXRUZXN0cy9m YXN0L3NoYXBlcy9zaGFwZS1vdXRzaWRlLWZsb2F0cy9zaGFwZS1vdXRzaWRlLWxhcmdlLXpvb20t ZXhwZWN0ZWQudHh0IGIvTGF5b3V0VGVzdHMvZmFzdC9zaGFwZXMvc2hhcGUtb3V0c2lkZS1mbG9h dHMvc2hhcGUtb3V0c2lkZS1sYXJnZS16b29tLWV4cGVjdGVkLnR4dApuZXcgZmlsZSBtb2RlIDEw MDY0NAppbmRleCAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwLi43MzBl YmY2NmEwZGE0OTgwMDE3ZTliYzAzN2M2ZTNmODRhYjdlNzllCi0tLSAvZGV2L251bGwKKysrIGIv TGF5b3V0VGVzdHMvZmFzdC9zaGFwZXMvc2hhcGUtb3V0c2lkZS1mbG9hdHMvc2hhcGUtb3V0c2lk ZS1sYXJnZS16b29tLWV4cGVjdGVkLnR4dApAQCAtMCwwICsxIEBACitUaGlzIHRlc3QgcGFzc2Vz IGlmIGl0IGRvZXNuJ3QgY3Jhc2guCmRpZmYgLS1naXQgYS9MYXlvdXRUZXN0cy9mYXN0L3NoYXBl cy9zaGFwZS1vdXRzaWRlLWZsb2F0cy9zaGFwZS1vdXRzaWRlLWxhcmdlLXpvb20uaHRtbCBiL0xh eW91dFRlc3RzL2Zhc3Qvc2hhcGVzL3NoYXBlLW91dHNpZGUtZmxvYXRzL3NoYXBlLW91dHNpZGUt bGFyZ2Utem9vbS5odG1sCm5ldyBmaWxlIG1vZGUgMTAwNjQ0CmluZGV4IDAwMDAwMDAwMDAwMDAw MDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAuLjFlZDg0OWEyZTEzZTlmZWFjNWVjZThjZDI0Mjk4 MWZlNTc2Y2FjNGQKLS0tIC9kZXYvbnVsbAorKysgYi9MYXlvdXRUZXN0cy9mYXN0L3NoYXBlcy9z aGFwZS1vdXRzaWRlLWZsb2F0cy9zaGFwZS1vdXRzaWRlLWxhcmdlLXpvb20uaHRtbApAQCAtMCww ICsxLDI0IEBACis8IURPQ1RZUEUgaHRtbD4KKzxodG1sPgorPGhlYWQ+Cis8c3R5bGU+Citib2R5 LCBoZWFkLCBzdHlsZSB7CisgIGRpc3BsYXk6IGlubGluZTsKKyAgZmxvYXQ6IGxlZnQ7CisgIHNo YXBlLW1hcmdpbjogMDsKKyAgc2hhcGUtb3V0c2lkZTogcmFkaWFsLWdyYWRpZW50KGdyZWVuIDAs IGdyZWVuIDApOworICB6b29tOiA0MTIzNDU2Nzg5MDEyMzsKK30KK3N0eWxlLCBzY3JpcHQgewor ICB2aXNpYmlsaXR5OiBoaWRkZW47Cit9Cis8L3N0eWxlPgorPC9oZWFkPgorPGJvZHk+CitUaGlz IHRlc3QgcGFzc2VzIGlmIGl0IGRvZXNuJ3QgY3Jhc2guCis8c2NyaXB0PgoraWYgKHdpbmRvdy50 ZXN0UnVubmVyKQorICAgIHRlc3RSdW5uZXIuZHVtcEFzVGV4dCgpCis8L3NjcmlwdD4KKzwvYm9k eT4KKzwvaHRtbD4K