57756 2011-04-04 08:03:36 -0700 chrome.dll!WebCore::RenderStyle::fontMetrics ReadAV@NULL (two crashes) 2011-06-09 14:10:48 -0700 1 1 1 Unclassified WebKit CSS 528+ (Nightly build) PC Windows Vista RESOLVED FIXED P1 Normal --- 1 skylined webkit-unassigned abarth eric hyatt inferno jchaffraix jschuh webkit.review.bot oldest_to_newest 378787 0 88060 skylined 2011-04-04 08:03:36 -0700 Created attachment 88060 Repro 34b28124d3d1bdb69ec42adc292c8e77 Repro: <script> document.writeln("<v>"); document.body.innerHTML="<style>*{border-radius:5ex;}</style>"; document.write("<title>x"); </script> id: chrome.dll!WebCore::RenderStyle::fontMetrics ReadAV@NULL (34b28124d3d1bdb69ec42adc292c8e77) description: Attempt to read from unallocated NULL pointer+0x30 in chrome.dll!WebCore::RenderStyle::fontMetrics application: Chromium 12.0.725.0 stack: chrome.dll!WebCore::RenderStyle::fontMetrics chrome.dll!WebCore::CSSPrimitiveValue::computeLengthDouble chrome.dll!WebCore::CSSPrimitiveValue::computeLengthInt chrome.dll!WebCore::CSSStyleSelector::applyProperty chrome.dll!WebCore::CSSStyleSelector::applyDeclarations<...> chrome.dll!WebCore::CSSStyleSelector::styleForElement chrome.dll!WebCore::Node::styleForRenderer chrome.dll!WebCore::HTMLTitleElement::textWithDirection chrome.dll!WebCore::HTMLTitleElement::childrenChanged chrome.dll!WebCore::ContainerNode::parserAddChild chrome.dll!WebCore::HTMLConstructionSite::attachAtSite chrome.dll!WebCore::HTMLConstructionSite::insertTextNode chrome.dll!WebCore::HTMLTreeBuilder::processCharacterBuffer chrome.dll!WebCore::HTMLTreeBuilder::processCharacter chrome.dll!WebCore::HTMLTreeBuilder::constructTreeFromAtomicToken chrome.dll!WebCore::HTMLTreeBuilder::constructTreeFromToken chrome.dll!WebCore::HTMLDocumentParser::pumpTokenizer chrome.dll!WebCore::HTMLDocumentParser::insert chrome.dll!WebCore::Document::write chrome.dll!WebCore::Document::write chrome.dll!WebCore::V8HTMLDocument::writeCallback chrome.dll!v8::internal::HandleApiCallHelper<...> chrome.dll!v8::internal::Builtin_HandleApiCall chrome.dll!v8::internal::Invoke chrome.dll!v8::internal::Execution::Call chrome.dll!v8::Script::Run - or - <script> document.writeln("<v>"); document.body.innerHTML="<style>*{-webkit-border-end-width:5ex;}</style>"; document.write("<title>x"); </script> id: chrome.dll!WebCore::RenderStyle::fontMetrics ReadAV@NULL (164e5193a19e7700092c2c9f88ca066e) (ever so slightly different stack) Both repros trigger similar NULL ptr crashes, so I assume they are the same issue. 378788 1 88061 skylined 2011-04-04 08:04:00 -0700 Created attachment 88061 Repro 164e5193a19e7700092c2c9f88ca066e 401123 2 skylined 2011-05-10 04:00:57 -0700 This is probably related to bug 51466 or a variation thereof. Crash id for debug builds: chrome.dll!WebCore::Font::primaryFont ReadAV@NULL (8f0571b4327014e218145b45bae3bada) Simplified repro: <script> document.documentElement.innerHTML = '<title style="border:0ex;">x'; </script> m_fontList is NULL in: webkit\source\webcore\platform\graphics\font.h inline const SimpleFontData* Font::primaryFont() const { ASSERT(m_fontList); return m_fontList->primarySimpleFontData(this); } Which is called by: webkit\source\webcore\platform\graphics\font.h const FontMetrics& fontMetrics() const { return primaryFont()->fontMetrics(); } Which is called by: webkit\source\webcore\rendering\style\renderstyle.h const FontMetrics& fontMetrics() const { return inherited->font.fontMetrics(); } Which is called by: webkit\source\webcore\css\cssprimitivevalue.cpp double CSSPrimitiveValue::computeLengthDouble(RenderStyle* style, RenderStyle* rootStyle, double multiplier, bool computingFontSize) { unsigned short type = primitiveType(); <<<snip>>> switch (type) { <<<snip>>> case CSS_EXS: <<<snip>>> factor = style->fontMetrics().xHeight(); A few calls up the stack we see this: webkit\source\webcore\css\cssstyleselector.cpp PassRefPtr<RenderStyle> CSSStyleSelector::styleForElement(Element* e, RenderStyle* defaultParent, bool allowSharing, bool resolveForRootDefault, bool matchVisitedPseudoClass) { <<<snip>>> // Now do the author and user normal priority properties and all the !important properties. if (!resolveForRootDefault) { applyDeclarations<false>(false, lastUARule + 1, m_matchedDecls.size() - 1); applyDeclarations<false>(true, firstAuthorRule, lastAuthorRule); I believe the problem is that Font::update should have been called before we hit this code. It set m_fontList for the font: http://codesearch.google.com/codesearch/p?hl=en#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/platform/graphics/Font.cpp&q=Font::update&exact_package=chromium&sa=N&cd=1&ct=rc&l=115 void Font::update(PassRefPtr<FontSelector> fontSelector) const { // FIXME: It is pretty crazy that we are willing to just poke into a RefPtr, but it ends up // being reasonably safe (because inherited fonts in the render tree pick up the new // style anyway. Other copies are transient, e.g., the state in the GraphicsContext, and // won't stick around long enough to get you in trouble). Still, this is pretty disgusting, // and could eventually be rectified by using RefPtrs for Fonts themselves. if (!m_fontList) m_fontList = FontFallbackList::create(); m_fontList->invalidate(fontSelector); } @hyatt: You wrote the above comment in Font::update, can you have a look? 401125 3 skylined 2011-05-10 04:01:33 -0700 Chromium: http://code.google.com/p/chromium/issues/detail?id=78321 401610 4 93034 jchaffraix 2011-05-10 16:11:42 -0700 Created attachment 93034 tentative fix I had a look at this bug yesterday and I came to the same conclusion. In one of the branch inside styleForElement, the new RenderStyle does not inherits its FontFallbackList from its parents and thus it never gets created. Attached is the fix I came with (I am not marking it for review as I would also like to hear from dhyatt), feel free to re-use the test cases if this is not the right way. 401664 5 eric 2011-05-10 17:19:40 -0700 A null pointer crash is not a security bug, right? 401977 6 jchaffraix 2011-05-11 08:10:04 -0700 (In reply to comment #5) > A null pointer crash is not a security bug, right? This bug is not marked as security (ie public). My limited understanding is that null pointer dereferences are usually considered a non-exploitable DoS (though there has been some ways to leverage them in some specific contexts). 402075 7 eric 2011-05-11 10:18:50 -0700 Sorry, my mis-read. 409855 8 93034 jchaffraix 2011-05-25 12:41:42 -0700 Comment on attachment 93034 tentative fix After a quick discussion with dhyatt, this looks like the right approach. Putting the patch up for review. 416239 9 93034 morrita 2011-06-06 19:40:38 -0700 Comment on attachment 93034 tentative fix Why not let Font class create m_fontList lazily? This fix looks fragile for me... 417746 10 jchaffraix 2011-06-08 19:04:28 -0700 > Why not let Font class create m_fontList lazily? I think that's what Font::update is about. My understanding is that we don't want to allocate m_fontList unless we really need it. Also lazy allocation would have a cost on font handling. > This fix looks fragile for me... I don't deny that this is fragile and I don't mind refactoring the code (now or later) to make the code less fragile if that's what people prefer. 418167 11 93034 hyatt 2011-06-09 12:04:34 -0700 Comment on attachment 93034 tentative fix r=me I think this is ok for now, since this particular case is not common. 418193 12 93034 webkit.review.bot 2011-06-09 12:41:22 -0700 Comment on attachment 93034 tentative fix Clearing flags on attachment: 93034 Committed r88472: <http://trac.webkit.org/changeset/88472> 418194 13 webkit.review.bot 2011-06-09 12:41:27 -0700 All reviewed patches have been landed. Closing bug. 418273 14 jchaffraix 2011-06-09 14:10:48 -0700 FYI, filed bug 62390 for the font handling refactoring. 88060 2011-04-04 08:03:36 -0700 2011-04-04 08:03:36 -0700 Repro 34b28124d3d1bdb69ec42adc292c8e77 repro.html text/html 141 skylined PHNjcmlwdD4KICBkb2N1bWVudC53cml0ZWxuKCI8dj4iKTsKICBkb2N1bWVudC5ib2R5LmlubmVy SFRNTD0iPHN0eWxlPip7Ym9yZGVyLXJhZGl1czo1ZXg7fTwvc3R5bGU+IjsKICBkb2N1bWVudC53 cml0ZSgiPHRpdGxlPngiKTsKPC9zY3JpcHQ+ 88061 2011-04-04 08:04:00 -0700 2011-04-04 08:04:00 -0700 Repro 164e5193a19e7700092c2c9f88ca066e repro.html text/html 152 skylined PHNjcmlwdD4KICBkb2N1bWVudC53cml0ZWxuKCI8dj4iKTsKICBkb2N1bWVudC5ib2R5LmlubmVy SFRNTD0iPHN0eWxlPip7LXdlYmtpdC1ib3JkZXItZW5kLXdpZHRoOjBleDt9PC9zdHlsZT4iOwog IGRvY3VtZW50LndyaXRlKCI8dGl0bGU+eCIpOwo8L3NjcmlwdD4= 93034 2011-05-10 16:11:42 -0700 2011-06-09 12:41:22 -0700 tentative fix bug57756-fontMetric-crash.diff text/plain 4654 jchaffraix ZGlmZiAtLWdpdCBhL0xheW91dFRlc3RzL0NoYW5nZUxvZyBiL0xheW91dFRlc3RzL0NoYW5nZUxv ZwppbmRleCBiYWRlZmJjLi5lM2Y5NDczIDEwMDY0NAotLS0gYS9MYXlvdXRUZXN0cy9DaGFuZ2VM b2cKKysrIGIvTGF5b3V0VGVzdHMvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTUgQEAKKzIwMTEtMDUt MDkgIEp1bGllbiBDaGFmZnJhaXggIDxqY2hhZmZyYWl4QGNvZGVhdXJvcmEub3JnPgorCisgICAg ICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIFRlc3QgZm9yIGNocm9t ZS5kbGwhV2ViQ29yZTo6UmVuZGVyU3R5bGU6OmZvbnRNZXRyaWNzIFJlYWRBVkBOVUxMICh0d28g Y3Jhc2hlcykKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lk PTU3NzU2CisKKyAgICAgICAgKiBmYXN0L2Nzcy9mb250TWV0cmljLWJvcmRlci1yYWRpdXMtbnVs bC1jcmFzaC1leHBlY3RlZC50eHQ6IEFkZGVkLgorICAgICAgICAqIGZhc3QvY3NzL2ZvbnRNZXRy aWMtYm9yZGVyLXJhZGl1cy1udWxsLWNyYXNoLmh0bWw6IEFkZGVkLgorICAgICAgICAqIGZhc3Qv Y3NzL2ZvbnRNZXRyaWMtd2Via2l0LWJvcmRlci1lbmQtd2lkdGgtbnVsbC1jcmFzaC1leHBlY3Rl ZC50eHQ6IEFkZGVkLgorICAgICAgICAqIGZhc3QvY3NzL2ZvbnRNZXRyaWMtd2Via2l0LWJvcmRl ci1lbmQtd2lkdGgtbnVsbC1jcmFzaC5odG1sOiBBZGRlZC4KKwogMjAxMS0wNS0wOCAgQWJoaXNo ZWsgQXJ5YSAgPGluZmVybm9AY2hyb21pdW0ub3JnPgogCiAgICAgICAgIFJldmlld2VkIGJ5IERh dmUgSHlhdHQuCmRpZmYgLS1naXQgYS9MYXlvdXRUZXN0cy9mYXN0L2Nzcy9mb250TWV0cmljLWJv cmRlci1yYWRpdXMtbnVsbC1jcmFzaC1leHBlY3RlZC50eHQgYi9MYXlvdXRUZXN0cy9mYXN0L2Nz cy9mb250TWV0cmljLWJvcmRlci1yYWRpdXMtbnVsbC1jcmFzaC1leHBlY3RlZC50eHQKbmV3IGZp bGUgbW9kZSAxMDA2NDQKaW5kZXggMDAwMDAwMC4uYzZjODkwMwotLS0gL2Rldi9udWxsCisrKyBi L0xheW91dFRlc3RzL2Zhc3QvY3NzL2ZvbnRNZXRyaWMtYm9yZGVyLXJhZGl1cy1udWxsLWNyYXNo LWV4cGVjdGVkLnR4dApAQCAtMCwwICsxLDIgQEAKK2Nocm9tZS5kbGwhV2ViQ29yZTo6UmVuZGVy U3R5bGU6OmZvbnRNZXRyaWNzIFJlYWRBVkBOVUxMICh0d28gY3Jhc2hlcykKK1BBU1NFRDogVGhp cyB0ZXN0IGRpZCBub3QgY3Jhc2ghCmRpZmYgLS1naXQgYS9MYXlvdXRUZXN0cy9mYXN0L2Nzcy9m b250TWV0cmljLWJvcmRlci1yYWRpdXMtbnVsbC1jcmFzaC5odG1sIGIvTGF5b3V0VGVzdHMvZmFz dC9jc3MvZm9udE1ldHJpYy1ib3JkZXItcmFkaXVzLW51bGwtY3Jhc2guaHRtbApuZXcgZmlsZSBt b2RlIDEwMDY0NAppbmRleCAwMDAwMDAwLi40MDY1NDM5Ci0tLSAvZGV2L251bGwKKysrIGIvTGF5 b3V0VGVzdHMvZmFzdC9jc3MvZm9udE1ldHJpYy1ib3JkZXItcmFkaXVzLW51bGwtY3Jhc2guaHRt bApAQCAtMCwwICsxLDggQEAKKzxzY3JpcHQ+CisgICAgaWYgKHdpbmRvdy5sYXlvdXRUZXN0Q29u dHJvbGxlcikKKyAgICAgICAgbGF5b3V0VGVzdENvbnRyb2xsZXIuZHVtcEFzVGV4dCgpOworICAg IGRvY3VtZW50LndyaXRlbG4oIjx2PiIpOworICAgIGRvY3VtZW50LmJvZHkuaW5uZXJIVE1MPSI8 c3R5bGU+Kntib3JkZXItcmFkaXVzOjVleDt9PC9zdHlsZT4iOworICAgIGRvY3VtZW50LndyaXRl KCI8dGl0bGU+eCIpOworICAgIGRvY3VtZW50LmJvZHkuaW5uZXJIVE1MID0gIjxhIGhyZWY9J2h0 dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD01Nzc1Nic+Y2hyb21lLmRsbCFX ZWJDb3JlOjpSZW5kZXJTdHlsZTo6Zm9udE1ldHJpY3MgUmVhZEFWQE5VTEwgKHR3byBjcmFzaGVz KTxicj5QQVNTRUQ6IFRoaXMgdGVzdCBkaWQgbm90IGNyYXNoISI7Cis8L3NjcmlwdD4KZGlmZiAt LWdpdCBhL0xheW91dFRlc3RzL2Zhc3QvY3NzL2ZvbnRNZXRyaWMtd2Via2l0LWJvcmRlci1lbmQt d2lkdGgtbnVsbC1jcmFzaC1leHBlY3RlZC50eHQgYi9MYXlvdXRUZXN0cy9mYXN0L2Nzcy9mb250 TWV0cmljLXdlYmtpdC1ib3JkZXItZW5kLXdpZHRoLW51bGwtY3Jhc2gtZXhwZWN0ZWQudHh0Cm5l dyBmaWxlIG1vZGUgMTAwNjQ0CmluZGV4IDAwMDAwMDAuLmM2Yzg5MDMKLS0tIC9kZXYvbnVsbAor KysgYi9MYXlvdXRUZXN0cy9mYXN0L2Nzcy9mb250TWV0cmljLXdlYmtpdC1ib3JkZXItZW5kLXdp ZHRoLW51bGwtY3Jhc2gtZXhwZWN0ZWQudHh0CkBAIC0wLDAgKzEsMiBAQAorY2hyb21lLmRsbCFX ZWJDb3JlOjpSZW5kZXJTdHlsZTo6Zm9udE1ldHJpY3MgUmVhZEFWQE5VTEwgKHR3byBjcmFzaGVz KQorUEFTU0VEOiBUaGlzIHRlc3QgZGlkIG5vdCBjcmFzaCEKZGlmZiAtLWdpdCBhL0xheW91dFRl c3RzL2Zhc3QvY3NzL2ZvbnRNZXRyaWMtd2Via2l0LWJvcmRlci1lbmQtd2lkdGgtbnVsbC1jcmFz aC5odG1sIGIvTGF5b3V0VGVzdHMvZmFzdC9jc3MvZm9udE1ldHJpYy13ZWJraXQtYm9yZGVyLWVu ZC13aWR0aC1udWxsLWNyYXNoLmh0bWwKbmV3IGZpbGUgbW9kZSAxMDA2NDQKaW5kZXggMDAwMDAw MC4uZDEyN2YwMgotLS0gL2Rldi9udWxsCisrKyBiL0xheW91dFRlc3RzL2Zhc3QvY3NzL2ZvbnRN ZXRyaWMtd2Via2l0LWJvcmRlci1lbmQtd2lkdGgtbnVsbC1jcmFzaC5odG1sCkBAIC0wLDAgKzEs OCBAQAorPHNjcmlwdD4KKyAgICBpZiAod2luZG93LmxheW91dFRlc3RDb250cm9sbGVyKQorICAg ICAgICBsYXlvdXRUZXN0Q29udHJvbGxlci5kdW1wQXNUZXh0KCk7CisgICAgZG9jdW1lbnQud3Jp dGVsbigiPHY+Iik7CisgICAgZG9jdW1lbnQuYm9keS5pbm5lckhUTUw9IjxzdHlsZT4qey13ZWJr aXQtYm9yZGVyLWVuZC13aWR0aDowZXg7fTwvc3R5bGU+IjsKKyAgICBkb2N1bWVudC53cml0ZSgi PHRpdGxlPngiKTsKKyAgICBkb2N1bWVudC5ib2R5LmlubmVySFRNTCA9ICI8YSBocmVmPSdodHRw czovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9NTc3NTYnPmNocm9tZS5kbGwhV2Vi Q29yZTo6UmVuZGVyU3R5bGU6OmZvbnRNZXRyaWNzIFJlYWRBVkBOVUxMICh0d28gY3Jhc2hlcyk8 YnI+UEFTU0VEOiBUaGlzIHRlc3QgZGlkIG5vdCBjcmFzaCEiOworPC9zY3JpcHQ+CmRpZmYgLS1n aXQgYS9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cK aW5kZXggOTIwM2M4ZC4uMjBkYWY1ZSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdl TG9nCisrKyBiL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE4IEBACisyMDEx LTA1LTA5ICBKdWxpZW4gQ2hhZmZyYWl4ICA8amNoYWZmcmFpeEBjb2RlYXVyb3JhLm9yZz4KKwor ICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBjaHJvbWUuZGxs IVdlYkNvcmU6OlJlbmRlclN0eWxlOjpmb250TWV0cmljcyBSZWFkQVZATlVMTCAodHdvIGNyYXNo ZXMpCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD01Nzc1 NgorCisgICAgICAgIFRlc3RzOiBmYXN0L2Nzcy9mb250TWV0cmljLWJvcmRlci1yYWRpdXMtbnVs bC1jcmFzaC5odG1sCisgICAgICAgICAgICAgICBmYXN0L2Nzcy9mb250TWV0cmljLXdlYmtpdC1i b3JkZXItZW5kLXdpZHRoLW51bGwtY3Jhc2guaHRtbAorCisgICAgICAgICogY3NzL0NTU1N0eWxl U2VsZWN0b3IuY3BwOgorICAgICAgICAoV2ViQ29yZTo6Q1NTU3R5bGVTZWxlY3Rvcjo6c3R5bGVG b3JFbGVtZW50KTogQWRkZWQgYSBjYWxsIHRvIEZvbnQ6OnVwZGF0ZQorICAgICAgICBzbyB0aGF0 IG91ciBGb250RmFsbGJhY2tMaXN0IGlzIGFsbG9jYXRlZCBpZiB3ZSBldmVyIG5lZWQgaXQgd2hl biBhcHBseWluZyBvdXIKKyAgICAgICAgc3R5bGUgcnVsZXMuCisKIDIwMTEtMDUtMDcgIE1hcnRp biBSb2JpbnNvbiAgPG1yb2JpbnNvbkBpZ2FsaWEuY29tPgogCiAgICAgICAgIFJldmlld2VkIGJ5 IERhbmllbCBCYXRlcy4KZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL2Nzcy9DU1NTdHlsZVNl bGVjdG9yLmNwcCBiL1NvdXJjZS9XZWJDb3JlL2Nzcy9DU1NTdHlsZVNlbGVjdG9yLmNwcAppbmRl eCBlNWNhYmM2Li5iNjI4MWYzIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9jc3MvQ1NTU3R5 bGVTZWxlY3Rvci5jcHAKKysrIGIvU291cmNlL1dlYkNvcmUvY3NzL0NTU1N0eWxlU2VsZWN0b3Iu Y3BwCkBAIC0xMzExLDggKzEzMTEsMTEgQEAgUGFzc1JlZlB0cjxSZW5kZXJTdHlsZT4gQ1NTU3R5 bGVTZWxlY3Rvcjo6c3R5bGVGb3JFbGVtZW50KEVsZW1lbnQqIGUsIFJlbmRlclN0eWwKIAogICAg IGlmIChtX3BhcmVudFN0eWxlKQogICAgICAgICBtX3N0eWxlLT5pbmhlcml0RnJvbShtX3BhcmVu dFN0eWxlKTsKLSAgICBlbHNlCisgICAgZWxzZSB7CiAgICAgICAgIG1fcGFyZW50U3R5bGUgPSBz dHlsZSgpOworICAgICAgICAvLyBNYWtlIHN1cmUgb3VyIGZvbnRzIGFyZSBpbml0aWFsaXplZCBp ZiB3ZSBkb24ndCBpbmhlcml0IHRoZW0gZnJvbSBvdXIgcGFyZW50IHN0eWxlLgorICAgICAgICBt X3N0eWxlLT5mb250KCkudXBkYXRlKDApOworICAgIH0KIAogICAgIGlmIChlLT5pc0xpbmsoKSkg ewogICAgICAgICBtX3N0eWxlLT5zZXRJc0xpbmsodHJ1ZSk7Cg==