Bug 41554 – Crash reading past end of block in UniscribeController::shapeAndPlaceItem

• Status: RESOLVED
• Resolution: FIXED
• Component: Text

[Alice Liu]

Under full page heap, Safari on Windows crashes at http://www.comfever.com/news/gadgets/12768423898935.

<rdar://problem/6565047>

Steps to Repro:
1) gflags -i Safari.exe +hpa (download Debugging Tools for Windows if needed)
2) http://www.comfever.com/news/gadgets/12768423898935

Results:
Crash:

>	WebKit.dll!WebCore::UniscribeController::shapeAndPlaceItem(const wchar_t * cp=0x35c86ff8, unsigned int i=0, const WebCore::SimpleFontData * fontData=0x360cfb88, WebCore::GlyphBuffer * glyphBuffer=0x00000000)  Line 291 + 0x1c bytes	C++
 	WebKit.dll!WebCore::UniscribeController::itemizeShapeAndPlace(const wchar_t * cp=0x35c86ff8, unsigned int length=4, const WebCore::SimpleFontData * fontData=0x360cfb88, WebCore::GlyphBuffer * glyphBuffer=0x00000000)  Line 199 + 0x18 bytes	C++
 	WebKit.dll!WebCore::UniscribeController::advance(unsigned int offset=4, WebCore::GlyphBuffer * glyphBuffer=0x00000000)  Line 177	C++
 	WebKit.dll!WebCore::Font::floatWidthForComplexText(const WebCore::TextRun & run={...}, WTF::HashSet<WebCore::SimpleFontData const *,WTF::PtrHash<WebCore::SimpleFontData const *>,WTF::HashTraits<WebCore::SimpleFontData const *> > * fallbackFonts=0x0060ddc4, WebCore::GlyphOverflow * glyphOverflow=0x0060ddac)  Line 98	C++
 	WebKit.dll!WebCore::Font::floatWidth(const WebCore::TextRun & run={...}, WTF::HashSet<WebCore::SimpleFontData const *,WTF::PtrHash<WebCore::SimpleFontData const *>,WTF::HashTraits<WebCore::SimpleFontData const *> > * fallbackFonts=0x0060ddc4, WebCore::GlyphOverflow * glyphOverflow=0x0060ddac)  Line 174	C++
 	WebKit.dll!WebCore::Font::width(const WebCore::TextRun & run={...}, WTF::HashSet<WebCore::SimpleFontData const *,WTF::PtrHash<WebCore::SimpleFontData const *>,WTF::HashTraits<WebCore::SimpleFontData const *> > * fallbackFonts=0x0060ddc4, WebCore::GlyphOverflow * glyphOverflow=0x0060ddac)  Line 97 + 0x22 bytes	C++
 	WebKit.dll!WebCore::RenderText::widthFromCache(const WebCore::Font & f={...}, int start=14, int len=4, int xPos=128, WTF::HashSet<WebCore::SimpleFontData const *,WTF::PtrHash<WebCore::SimpleFontData const *>,WTF::HashTraits<WebCore::SimpleFontData const *> > * fallbackFonts=0x0060ddc4, WebCore::GlyphOverflow * glyphOverflow=0x0060ddac)  Line 536	C++
 	WebKit.dll!WebCore::RenderText::calcPrefWidths(int leadWidth=61, WTF::HashSet<WebCore::SimpleFontData const *,WTF::PtrHash<WebCore::SimpleFontData const *>,WTF::HashTraits<WebCore::SimpleFontData const *> > & fallbackFonts={...}, WebCore::GlyphOverflow & glyphOverflow={...})  Line 758 + 0x23 bytes	C++
 	WebKit.dll!WebCore::RenderText::calcPrefWidths(int leadWidth=61)  Line 653	C++
 	WebKit.dll!WebCore::RenderText::trimmedPrefWidths(int leadWidth=61, int & beginMinW=0, bool & beginWS=true, int & endMinW=0, bool & endWS=true, bool & hasBreakableChar=true, bool & hasBreak=false, int & beginMaxW=-858993460, int & endMaxW=-858993460, int & minW=0, int & maxW=0, bool & stripFrontSpaces=true)  Line 550 + 0x16 bytes	C++
 	WebKit.dll!WebCore::RenderBlock::calcInlinePrefWidths()  Line 4731	C++
 	WebKit.dll!WebCore::RenderBlock::calcPrefWidths()  Line 4411	C++
 	WebKit.dll!WebCore::RenderBox::minPrefWidth()  Line 464 + 0x12 bytes	C++
 	WebKit.dll!WebCore::RenderBlock::calcBlockPrefWidths()  Line 4853 + 0x12 bytes	C++
 	WebKit.dll!WebCore::RenderBlock::calcPrefWidths()  Line 4414	C++
 	WebKit.dll!WebCore::RenderListItem::calcPrefWidths()  Line 235	C++
 	WebKit.dll!WebCore::RenderBox::minPrefWidth()  Line 464 + 0x12 bytes	C++
 	WebKit.dll!WebCore::RenderBlock::calcBlockPrefWidths()  Line 4853 + 0x12 bytes	C++
 	WebKit.dll!WebCore::RenderBlock::calcPrefWidths()  Line 4414	C++
 	WebKit.dll!WebCore::RenderBox::minPrefWidth()  Line 464 + 0x12 bytes	C++
 	WebKit.dll!WebCore::RenderBox::calcWidthUsing(WebCore::WidthType widthType=Width, int cw=970)  Line 1374 + 0x12 bytes	C++
 	WebKit.dll!WebCore::RenderBox::calcWidth()  Line 1313 + 0xe bytes	C++
 	WebKit.dll!WebCore::RenderBlock::layoutBlock(bool relayoutChildren=false)  Line 1135 + 0x12 bytes	C++
 	WebKit.dll!WebCore::RenderBlock::layout()  Line 1111 + 0x14 bytes	C++
 	WebKit.dll!WebCore::RenderObject::layoutIfNeeded()  Line 544 + 0x30 bytes	C++
 	WebKit.dll!WebCore::RenderBlock::layoutInlineChildren(bool relayoutChildren=true, int & repaintTop=0, int & repaintBottom=0)  Line 586	C++
 	WebKit.dll!WebCore::RenderBlock::layoutBlock(bool relayoutChildren=true)  Line 1188	C++
 	WebKit.dll!WebCore::RenderBlock::layout()  Line 1111 + 0x14 bytes	C++
 	WebKit.dll!WebCore::RenderBlock::layoutBlockChild(WebCore::RenderBox * child=0x35be2f7c, WebCore::RenderBlock::MarginInfo & marginInfo={...}, int & previousFloatBottom=0, int & maxFloatBottom=0)  Line 1804 + 0x12 bytes	C++
 	WebKit.dll!WebCore::RenderBlock::layoutBlockChildren(bool relayoutChildren=true, int & maxFloatBottom=0)  Line 1748	C++
 	WebKit.dll!WebCore::RenderBlock::layoutBlock(bool relayoutChildren=true)  Line 1192	C++
 	WebKit.dll!WebCore::RenderBlock::layout()  Line 1111 + 0x14 bytes	C++
 	WebKit.dll!WebCore::RenderBlock::layoutBlockChild(WebCore::RenderBox * child=0x35b9cf7c, WebCore::RenderBlock::MarginInfo & marginInfo={...}, int & previousFloatBottom=0, int & maxFloatBottom=0)  Line 1804 + 0x12 bytes	C++
 	WebKit.dll!WebCore::RenderBlock::layoutBlockChildren(bool relayoutChildren=true, int & maxFloatBottom=0)  Line 1748	C++
 	WebKit.dll!WebCore::RenderBlock::layoutBlock(bool relayoutChildren=true)  Line 1192	C++
 	WebKit.dll!WebCore::RenderBlock::layout()  Line 1111 + 0x14 bytes	C++
 	WebKit.dll!WebCore::RenderBlock::layoutBlockChild(WebCore::RenderBox * child=0x35a4bf7c, WebCore::RenderBlock::MarginInfo & marginInfo={...}, int & previousFloatBottom=0, int & maxFloatBottom=0)  Line 1804 + 0x12 bytes	C++
 	WebKit.dll!WebCore::RenderBlock::layoutBlockChildren(bool relayoutChildren=true, int & maxFloatBottom=0)  Line 1748	C++
 	WebKit.dll!WebCore::RenderBlock::layoutBlock(bool relayoutChildren=true)  Line 1192	C++
 	WebKit.dll!WebCore::RenderBlock::layout()  Line 1111 + 0x14 bytes	C++
 	WebKit.dll!WebCore::RenderBlock::layoutBlockChild(WebCore::RenderBox * child=0x34440f7c, WebCore::RenderBlock::MarginInfo & marginInfo={...}, int & previousFloatBottom=0, int & maxFloatBottom=0)  Line 1804 + 0x12 bytes	C++
 	WebKit.dll!WebCore::RenderBlock::layoutBlockChildren(bool relayoutChildren=false, int & maxFloatBottom=0)  Line 1748	C++
 	WebKit.dll!WebCore::RenderBlock::layoutBlock(bool relayoutChildren=false)  Line 1192	C++
 	WebKit.dll!WebCore::RenderBlock::layout()  Line 1111 + 0x14 bytes	C++
 	WebKit.dll!WebCore::RenderBlock::layoutBlockChild(WebCore::RenderBox * child=0x32f8ef7c, WebCore::RenderBlock::MarginInfo & marginInfo={...}, int & previousFloatBottom=0, int & maxFloatBottom=0)  Line 1804 + 0x12 bytes	C++
 	WebKit.dll!WebCore::RenderBlock::layoutBlockChildren(bool relayoutChildren=false, int & maxFloatBottom=0)  Line 1748	C++
 	WebKit.dll!WebCore::RenderBlock::layoutBlock(bool relayoutChildren=false)  Line 1192	C++
 	WebKit.dll!WebCore::RenderBlock::layout()  Line 1111 + 0x14 bytes	C++
 	WebKit.dll!WebCore::RenderView::layout()  Line 126	C++
 	WebKit.dll!WebCore::FrameView::layout(bool allowSubtree=true)  Line 764 + 0x12 bytes	C++
 	WebKit.dll!WebCore::FrameView::layoutTimerFired(WebCore::Timer<WebCore::FrameView> * __formal=0x2ee6eeb8)  Line 1316	C++
 	WebKit.dll!WebCore::Timer<WebCore::FrameView>::fired()  Line 98 + 0x29 bytes	C++
 	WebKit.dll!WebCore::ThreadTimers::sharedTimerFiredInternal()  Line 112 + 0xf bytes	C++
 	WebKit.dll!WebCore::ThreadTimers::sharedTimerFired()  Line 91	C++
 	WebKit.dll!WebCore::TimerWindowWndProc(HWND__ * hWnd=0x000909c0, unsigned int message=49681, unsigned int wParam=0, long lParam=0)  Line 103 + 0x8 bytes	C++
 	user32.dll!_InternalCallWinProc@20()  + 0x23 bytes	
 	user32.dll!_UserCallWinProcCheckWow@32()  + 0xd3 bytes	
 	user32.dll!_DispatchMessageWorker@8()  + 0xee bytes	
 	user32.dll!_DispatchMessageW@4()  + 0xf bytes	
 	Safari.dll!RunMessagePump(WTL::CMessageLoop & messageLoop={...})  Line 190 + 0xc bytes	C++
 	Safari.dll!run(int nCmdShow=1)  Line 256 + 0x9 bytes	C++
 	Safari.dll!safariMain(HINSTANCE__ * hInstance=0x69d90000, HINSTANCE__ * __formal=0x00000000, wchar_t * lpstrCmdLine=0x0006d06c, int nCmdShow=1)  Line 597 + 0x9 bytes	C++
 	Safari.dll!safariDLLMain(HINSTANCE__ * hInstance=0x00230000, HINSTANCE__ * hPrevInstance=0x00000000, wchar_t * lpstrCmdLine=0x0006d06c, int nCmdShow=1)  Line 52 + 0x15 bytes	C++
 	Safari.exe!wWinMain(HINSTANCE__ * hInstance=0x00230000, HINSTANCE__ * hPrevInstance=0x00000000, wchar_t * lpstrCmdLine=0x0006d06c, int nCmdShow=1)  Line 199 + 0x18 bytes	C++
 	Safari.exe!__tmainCRTStartup()  Line 589 + 0x1c bytes	C
 	kernel32.dll!@BaseThreadInitThunk@12()  + 0xe bytes	
 	ntdll.dll!___RtlUserThreadStart@8()  + 0x23 bytes	
 	ntdll.dll!__RtlUserThreadStart@8()  + 0x1b bytes

[Alice Liu]

Created attachment 60429 [details]
patch and layout test

Comment on attachment 60429 [details]
patch and layout test

+            if (boundary < m_run.length()) {
I wonder how this condition (which the patch doesn’t touch) could ever be false.

[Darin Adler]

Comment on attachment 60429 [details]
patch and layout test

A couple drive-by comments?

> +            // Instead look ahead to the first character of next item, if there is a next one. 
> +            if (k + 1 == len) {  

I noticed some stray spaces on the ends of these lines.

> +                if (i + 2 < m_items.size() // Next item is not the terminating item
> +                    && Font::isRoundingHackCharacter(*(cp + m_items[i + 1].iCharPos)))

Could this comment explain further why this is "i + 2 < size" rather than "i + 1 < size"? Is there some reason we would not look at a character within the last item?

[Alice Liu]

(In reply to comment #2)
> (From update of attachment 60429 [details])
> +            if (boundary < m_run.length()) {
> I wonder how this condition (which the patch doesn’t touch) could ever be false.

Let's ask Hyatt =)
It's likely that I just don't understand it at this point, but the calculation of boundary seems strange, like it's over-counting or something.  So ya, i wouldn't be surprised if it ran over the run length.

[Alice Liu]

(In reply to comment #3)
> (From update of attachment 60429 [details])
> A couple drive-by comments?

Drive by comments are the reason I sometimes delay my patch commitment!  I addressed these suggestions.

committed http://trac.webkit.org/changeset/62477

(In reply to comment #5)
> committed http://trac.webkit.org/changeset/62477

Looks like you forgot to land the regression test!

[Alice Liu]

omg whoops!  committed from one directory too deep. 
http://trac.webkit.org/changeset/62512

[WebKit Review Bot]

http://trac.webkit.org/changeset/62512 might have broken Leopard Intel Release (Tests)