Closed Bug 593155 Opened 14 years ago Closed 14 years ago

Talos Tp4 crash [ @ gfxFontUtils::FindPreferredSubtable], apparently involving harfbuzz

Categories

(Core :: Graphics, defect)

Product:
Core
Component:
Graphics
Platform:
x86
Windows Server 2003
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

()

Status:
RESOLVED FIXED
Tracking Flags:
Tracking Status
blocking2.0 --- final+

People

(Reporter: ehsan.akhgari, Assigned: jfkthame)

References

Details

(Keywords: crash, intermittent-failure)

Attachments

(1 file)

check cmap table length before accessing fields
2.42 KB, patch
jtd
: review+
roc
: approval2.0+
Details | Diff | Splinter Review 
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1283463045.1283464010.28376.gz
Rev3 WINNT 6.1 mozilla-central talos tp4 on 2010/09/02 14:30:45

Thread 0 (crashed)
 0  xul.dll!gfxFontUtils::FindPreferredSubtable(unsigned char const *,unsigned int,unsigned int *,unsigned int *,int *) [gfxFontUtils.cpp:f2f217290bd0 : 529 + 0x6]
    eip = 0x69503057   esp = 0x001979b8   ebp = 0x001979d4   ebx = 0x0cce5b80
    esi = 0x00000000   edi = 0x0b2bae80   eax = 0x00000000   ecx = 0x00000000
    edx = 0x00000000   efl = 0x00210246
    Found by: given as instruction pointer in context
 1  xul.dll!gfxHarfBuzzShaper::InitTextRun(gfxContext *,gfxTextRun *,unsigned short const *,unsigned int,unsigned int,int) [gfxHarfBuzzShaper.cpp:f2f217290bd0 : 738 + 0x19]
    eip = 0x696dcc7f   esp = 0x001979dc   ebp = 0x00197b98
    Found by: previous frame's frame pointer
 2  xul.dll!gfxFont::InitTextRun(gfxContext *,gfxTextRun *,unsigned short const *,unsigned int,unsigned int,int) [gfxFont.cpp:f2f217290bd0 : 1338 + 0x19]
    eip = 0x69c40a00   esp = 0x00197ba0   ebp = 0x00197bc0
    Found by: previous frame's frame pointer
 3  xul.dll!gfxFontGroup::InitTextRun(gfxContext *,gfxTextRun *,unsigned short const *,unsigned int,unsigned int,unsigned int,int) [gfxFont.cpp:f2f217290bd0 : 2245 + 0x1a]
    eip = 0x6959737d   esp = 0x00197bc8   ebp = 0x00197c3c
    Found by: previous frame's frame pointer
 4  xul.dll!gfxFontGroup::InitTextRun(gfxContext *,gfxTextRun *,unsigned short const *,unsigned int) [gfxFont.cpp:f2f217290bd0 : 2213 + 0x1c]
    eip = 0x69592e75   esp = 0x00197c44   ebp = 0x00197da4
    Found by: previous frame's frame pointer
 5  xul.dll!gfxFontGroup::MakeTextRun(unsigned short const *,unsigned int,gfxTextRunFactory::Parameters const *,unsigned int) [gfxFont.cpp:f2f217290bd0 : 2188 + 0xf]
    eip = 0x6954e27f   esp = 0x00197dac   ebp = 0x00197dc8
    Found by: previous frame's frame pointer
 6  xul.dll!TextRunWordCache::MakeTextRun(unsigned short const *,unsigned int,gfxFontGroup *,gfxTextRunFactory::Parameters const *,unsigned int) [gfxTextRunWordCache.cpp:f2f217290bd0 : 693 + 0x1c]
    eip = 0x695fabb9   esp = 0x00197dd0   ebp = 0x0019841c
    Found by: previous frame's frame pointer
 7  xul.dll!MakeTextRun [nsTextFrameThebes.cpp:f2f217290bd0 : 450 + 0x18]
    eip = 0x6974cddb   esp = 0x00198424   ebp = 0x00198440
    Found by: previous frame's frame pointer
 8  xul.dll!BuildTextRunsScanner::BuildTextRunForFrames(void *) [nsTextFrameThebes.cpp:f2f217290bd0 : 1810 + 0x17]
    eip = 0x696bf62d   esp = 0x00198448   ebp = 0x0019a888
    Found by: previous frame's frame pointer
 9  xul.dll!BuildTextRunsScanner::FlushFrames(int,int) [nsTextFrameThebes.cpp:f2f217290bd0 : 1242 + 0xd]
    eip = 0x696c3de0   esp = 0x00199808   ebp = 0x0019a888
    Found by: call frame info
10  xul.dll!BuildTextRuns [nsTextFrameThebes.cpp:f2f217290bd0 : 1176 + 0x1b]
    eip = 0x695e93c4   esp = 0x0019a834   ebp = 0x0019ab7c
    Found by: call frame info
11  xul.dll!nsTextFrame::EnsureTextRun(gfxContext *,nsIFrame *,nsLineList_iterator const *,unsigned int *) [nsTextFrameThebes.cpp:f2f217290bd0 : 2043 + 0xb]
    eip = 0x695ef221   esp = 0x0019ab84   ebp = 0x0019ad00
    Found by: previous frame's frame pointer
12  xul.dll!nsTextFrame::AddInlineMinWidthForFlow(nsIRenderingContext *,nsIFrame::InlineMinWidthData *) [nsTextFrameThebes.cpp:f2f217290bd0 : 5806 + 0x24]
    eip = 0x696ef86d   esp = 0x0019abd4   ebp = 0x0019ad00
    Found by: call frame info
13  xul.dll!nsTextFrame::AddInlineMinWidth(nsIRenderingContext *,nsIFrame::InlineMinWidthData *) [nsTextFrameThebes.cpp:f2f217290bd0 : 5918 + 0x9]
    eip = 0x695675bd   esp = 0x0019ace4   ebp = 0x0019ad00
    Found by: call frame info
14  xul.dll!nsContainerFrame::DoInlineIntrinsicWidth(nsIRenderingContext *,nsIFrame::InlineIntrinsicWidthData *,nsLayoutUtils::IntrinsicWidthType) [nsContainerFrame.cpp:f2f217290bd0 : 647 + 0x13]
    eip = 0x6951e30f   esp = 0x0019ad08   ebp = 0x0019ad44
    Found by: previous frame's frame pointer
15  xul.dll!nsInlineFrame::AddInlineMinWidth(nsIRenderingContext *,nsIFrame::InlineMinWidthData *) [nsInlineFrame.cpp:f2f217290bd0 : 209 + 0xf]
    eip = 0x69773f80   esp = 0x0019ad4c   ebp = 0x0019ad9c
    Found by: previous frame's frame pointer
16  xul.dll!nsContainerFrame::DoInlineIntrinsicWidth(nsIRenderingContext *,nsIFrame::InlineIntrinsicWidthData *,nsLayoutUtils::IntrinsicWidthType) [nsContainerFrame.cpp:f2f217290bd0 : 647 + 0x13]
    eip = 0x6951e30f   esp = 0x0019ad60   ebp = 0x0019ad9c
    Found by: call frame info with scanning
17  xul.dll!nsInlineFrame::AddInlineMinWidth(nsIRenderingContext *,nsIFrame::InlineMinWidthData *) [nsInlineFrame.cpp:f2f217290bd0 : 209 + 0xf]
    eip = 0x69773f80   esp = 0x0019ada4   ebp = 0x0019ae0c
    Found by: previous frame's frame pointer
18  xul.dll!nsBlockFrame::GetMinWidth(nsIRenderingContext *) [nsBlockFrame.cpp:f2f217290bd0 : 736 + 0x11]
    eip = 0x695bfec2   esp = 0x0019adb8   ebp = 0x0019ae0c
    Found by: call frame info with scanning
19  xul.dll!nsLayoutUtils::IntrinsicForContainer(nsIRenderingContext *,nsIFrame *,nsLayoutUtils::IntrinsicWidthType) [nsLayoutUtils.cpp:f2f217290bd0 : 1984 + 0x3]
    eip = 0x696df92c   esp = 0x0019ae14   ebp = 0x18899ccc
    Found by: previous frame's frame pointer
20  xul.dll!nsTableCellFrame::GetMinWidth(nsIRenderingContext *) [nsTableCellFrame.cpp:f2f217290bd0 : 726 + 0xb]
    eip = 0x697347d5   esp = 0x0019ae7c   ebp = 0x0019aed4
    Found by: call frame info
21  xul.dll!GetWidthInfo [BasicTableLayoutStrategy.cpp:f2f217290bd0 : 113 + 0xc]
    eip = 0x69530f71   esp = 0x0019ae8c   ebp = 0x0019aed4
    Found by: call frame info
22  xul.dll!BasicTableLayoutStrategy::ComputeColumnIntrinsicWidths(nsIRenderingContext *) [BasicTableLayoutStrategy.cpp:f2f217290bd0 : 308 + 0x10]
    eip = 0x69734c09   esp = 0x0019aedc   ebp = 0x0019afb0
    Found by: previous frame's frame pointer
23  xul.dll!BasicTableLayoutStrategy::ComputeIntrinsicWidths(nsIRenderingContext *) [BasicTableLayoutStrategy.cpp:f2f217290bd0 : 418 + 0x8]
    eip = 0x694f41fc   esp = 0x0019afb8   ebp = 0x0019aff0
    Found by: previous frame's frame pointer
24  xul.dll!BasicTableLayoutStrategy::GetMinWidth(nsIRenderingContext *) [BasicTableLayoutStrategy.cpp:f2f217290bd0 : 74 + 0xb]
    eip = 0x6975d14d   esp = 0x0019aff8   ebp = 0x0019b014
    Found by: previous frame's frame pointer
25  xul.dll!nsTableFrame::GetMinWidth(nsIRenderingContext *) [nsTableFrame.cpp:f2f217290bd0 : 1491 + 0x19]
    eip = 0x6975d03f   esp = 0x0019b004   ebp = 0x0019b014
    Found by: call frame info with scanning
26  xul.dll!nsLayoutUtils::IntrinsicForContainer(nsIRenderingContext *,nsIFrame *,nsLayoutUtils::IntrinsicWidthType) [nsLayoutUtils.cpp:f2f217290bd0 : 1984 + 0x3]
    eip = 0x696df92c   esp = 0x0019b01c   ebp = 0x0b71dab4
    Found by: previous frame's frame pointer
27  xul.dll!nsTableOuterFrame::GetMinWidth(nsIRenderingContext *) [nsTableOuterFrame.cpp:f2f217290bd0 : 501 + 0xd]
    eip = 0x69d128f8   esp = 0x0019b084   ebp = 0x198ed604
    Found by: call frame info
28  xul.dll!nsLayoutUtils::IntrinsicForContainer(nsIRenderingContext *,nsIFrame *,nsLayoutUtils::IntrinsicWidthType) [nsLayoutUtils.cpp:f2f217290bd0 : 1984 + 0x3]
    eip = 0x696df92c   esp = 0x0019b09c   ebp = 0x198ed604
    Found by: call frame info with scanning
29  xul.dll!nsBlockFrame::GetMinWidth(nsIRenderingContext *) [nsBlockFrame.cpp:f2f217290bd0 : 717 + 0xf]
    eip = 0x695bff4d   esp = 0x0019b104   ebp = 0x0019b15c
    Found by: call frame info
30  xul.dll!nsLayoutUtils::IntrinsicForContainer(nsIRenderingContext *,nsIFrame *,nsLayoutUtils::IntrinsicWidthType) [nsLayoutUtils.cpp:f2f217290bd0 : 1984 + 0x3]
    eip = 0x696df92c   esp = 0x0019b164   ebp = 0x18899ccc
    Found by: previous frame's frame pointer
31  xul.dll!nsTableCellFrame::GetMinWidth(nsIRenderingContext *) [nsTableCellFrame.cpp:f2f217290bd0 : 726 + 0xb]
    eip = 0x697347d5   esp = 0x0019b1cc   ebp = 0x0019b224
    Found by: call frame info
32  xul.dll!GetWidthInfo [BasicTableLayoutStrategy.cpp:f2f217290bd0 : 113 + 0xc]
    eip = 0x69530f71   esp = 0x0019b1dc   ebp = 0x0019b224
    Found by: call frame info
33  xul.dll!BasicTableLayoutStrategy::ComputeColumnIntrinsicWidths(nsIRenderingContext *) [BasicTableLayoutStrategy.cpp:f2f217290bd0 : 308 + 0x10]
    eip = 0x69734c09   esp = 0x0019b22c   ebp = 0x0019b300
    Found by: previous frame's frame pointer
34  xul.dll!BasicTableLayoutStrategy::ComputeIntrinsicWidths(nsIRenderingContext *) [BasicTableLayoutStrategy.cpp:f2f217290bd0 : 418 + 0x8]
    eip = 0x694f41fc   esp = 0x0019b308   ebp = 0x0019b340
    Found by: previous frame's frame pointer
35  xul.dll!BasicTableLayoutStrategy::GetMinWidth(nsIRenderingContext *) [BasicTableLayoutStrategy.cpp:f2f217290bd0 : 74 + 0xb]
    eip = 0x6975d14d   esp = 0x0019b348   ebp = 0x0019b364
    Found by: previous frame's frame pointer
36  xul.dll!nsTableFrame::GetMinWidth(nsIRenderingContext *) [nsTableFrame.cpp:f2f217290bd0 : 1491 + 0x19]
    eip = 0x6975d03f   esp = 0x0019b354   ebp = 0x0019b364
    Found by: call frame info with scanning
37  xul.dll!nsLayoutUtils::IntrinsicForContainer(nsIRenderingContext *,nsIFrame *,nsLayoutUtils::IntrinsicWidthType) [nsLayoutUtils.cpp:f2f217290bd0 : 1984 + 0x3]
    eip = 0x696df92c   esp = 0x0019b36c   ebp = 0x2693d074
    Found by: previous frame's frame pointer
38  xul.dll!nsTableOuterFrame::GetMinWidth(nsIRenderingContext *) [nsTableOuterFrame.cpp:f2f217290bd0 : 501 + 0xd]
    eip = 0x69d128f8   esp = 0x0019b3d4   ebp = 0x2695ca5c
    Found by: call frame info
39  xul.dll!nsLayoutUtils::IntrinsicForContainer(nsIRenderingContext *,nsIFrame *,nsLayoutUtils::IntrinsicWidthType) [nsLayoutUtils.cpp:f2f217290bd0 : 1984 + 0x3]
    eip = 0x696df92c   esp = 0x0019b3ec   ebp = 0x2695ca5c
    Found by: call frame info with scanning
40  xul.dll!nsBlockFrame::GetMinWidth(nsIRenderingContext *) [nsBlockFrame.cpp:f2f217290bd0 : 717 + 0xf]
    eip = 0x695bff4d   esp = 0x0019b454   ebp = 0x0019b4ac
    Found by: call frame info
41  xul.dll!nsLayoutUtils::IntrinsicForContainer(nsIRenderingContext *,nsIFrame *,nsLayoutUtils::IntrinsicWidthType) [nsLayoutUtils.cpp:f2f217290bd0 : 1984 + 0x3]
    eip = 0x696df92c   esp = 0x0019b4b4   ebp = 0x18899ccc
    Found by: previous frame's frame pointer
42  xul.dll!nsTableCellFrame::GetMinWidth(nsIRenderingContext *) [nsTableCellFrame.cpp:f2f217290bd0 : 726 + 0xb]
    eip = 0x697347d5   esp = 0x0019b51c   ebp = 0x0019b574
    Found by: call frame info
43  xul.dll!GetWidthInfo [BasicTableLayoutStrategy.cpp:f2f217290bd0 : 113 + 0xc]
    eip = 0x69530f71   esp = 0x0019b52c   ebp = 0x0019b574
    Found by: call frame info
44  xul.dll!BasicTableLayoutStrategy::ComputeColumnIntrinsicWidths(nsIRenderingContext *) [BasicTableLayoutStrategy.cpp:f2f217290bd0 : 308 + 0x10]
    eip = 0x69734c09   esp = 0x0019b57c   ebp = 0x0019b650
    Found by: previous frame's frame pointer
45  xul.dll!BasicTableLayoutStrategy::ComputeIntrinsicWidths(nsIRenderingContext *) [BasicTableLayoutStrategy.cpp:f2f217290bd0 : 418 + 0x8]
    eip = 0x694f41fc   esp = 0x0019b658   ebp = 0x0019b690
    Found by: previous frame's frame pointer
46  xul.dll!BasicTableLayoutStrategy::GetMinWidth(nsIRenderingContext *) [BasicTableLayoutStrategy.cpp:f2f217290bd0 : 74 + 0xb]
    eip = 0x6975d14d   esp = 0x0019b698   ebp = 0x0019b6b0
    Found by: previous frame's frame pointer
47  xul.dll!nsTableFrame::GetMinWidth(nsIRenderingContext *) [nsTableFrame.cpp:f2f217290bd0 : 1491 + 0x19]
    eip = 0x6975d03f   esp = 0x0019b6a4   ebp = 0x0019b6b0
    Found by: call frame info with scanning
48  xul.dll!nsTableFrame::TableShrinkWidthToFit(nsIRenderingContext *,int) [nsTableFrame.cpp:f2f217290bd0 : 1546 + 0xc]
    eip = 0x6975d15f   esp = 0x0019b6b8   ebp = 0x0019b6bc
    Found by: previous frame's frame pointer
49  xul.dll!nsTableFrame::ComputeAutoSize(nsIRenderingContext *,nsSize,int,nsSize,nsSize,nsSize,int) [nsTableFrame.cpp:f2f217290bd0 : 1578 + 0x17]
    eip = 0x6975d1a9   esp = 0x0019b6c4   ebp = 0x0019b6d0
    Found by: previous frame's frame pointer
50  xul.dll!nsFrame::ComputeSize(nsIRenderingContext *,nsSize,int,nsSize,nsSize,nsSize,int) [nsFrame.cpp:f2f217290bd0 : 3192 + 0x60]
    eip = 0x6969d1aa   esp = 0x0019b6d8   ebp = 0x0019b728
    Found by: previous frame's frame pointer
51  xul.dll!nsFieldSetFrame::ComputeSize(nsIRenderingContext *,nsSize,int,nsSize,nsSize,nsSize,int) [nsFieldSetFrame.cpp:f2f217290bd0 : 400 + 0x51]
    eip = 0x6975cce0   esp = 0x0019b730   ebp = 0x0019b768
    Found by: previous frame's frame pointer
52  xul.dll!ChildShrinkWrapWidth [nsTableOuterFrame.cpp:f2f217290bd0 : 584 + 0x76]
    eip = 0x6975f71a   esp = 0x0019b770   ebp = 0x0019b7ec
    Found by: previous frame's frame pointer
53  xul.dll!nsTableOuterFrame::ComputeAutoSize(nsIRenderingContext *,nsSize,int,nsSize,nsSize,nsSize,int) [nsTableOuterFrame.cpp:f2f217290bd0 : 611 + 0x1d]
    eip = 0x6975f774   esp = 0x0019b7f4   ebp = 0x0019b818
    Found by: previous frame's frame pointer
54  xul.dll!nsFrame::ComputeSize(nsIRenderingContext *,nsSize,int,nsSize,nsSize,nsSize,int) [nsFrame.cpp:f2f217290bd0 : 3192 + 0x60]
    eip = 0x6969d1aa   esp = 0x0019b820   ebp = 0x0019b870
    Found by: previous frame's frame pointer
55  xul.dll!nsBlockFrame::WidthToClearPastFloats(nsBlockReflowState &,nsRect const &,nsIFrame *) [nsBlockFrame.cpp:f2f217290bd0 : 6924 + 0x81]
    eip = 0x69736a3f   esp = 0x0019b878   ebp = 0x0019ba00
    Found by: previous frame's frame pointer
56  xul.dll!nsBlockReflowState::ComputeBlockAvailSpace(nsIFrame *,nsStyleDisplay const *,nsFlowAreaRect const &,int,nsRect &) [nsBlockReflowState.cpp:f2f217290bd0 : 292 + 0x16]
    eip = 0x696f09b0   esp = 0x0019ba08   ebp = 0x0019ba4c
    Found by: previous frame's frame pointer
57  xul.dll!nsBlockFrame::ReflowBlockFrame(nsBlockReflowState &,nsLineList_iterator,int *) [nsBlockFrame.cpp:f2f217290bd0 : 3091 + 0x2c]
    eip = 0x696c629d   esp = 0x0019ba54   ebp = 0x0019bf30
    Found by: previous frame's frame pointer
58  xul.dll!nsRefPtr<nsEventListenerManager>::~nsRefPtr<nsEventListenerManager>() [nsCOMPtr.cpp:f2f217290bd0 : 81 + 0x7]
    eip = 0x6966b32e   esp = 0x0019bd48   ebp = 0x0019bfa0
    Found by: call frame info with scanning
59  xul.dll!nsTextFrame::ReflowText(nsLineLayout &,int,nsIRenderingContext *,int,nsHTMLReflowMetrics &,unsigned int &) [nsTextFrameThebes.cpp:f2f217290bd0 : 6464 + 0xc4]
    eip = 0x695ffba0   esp = 0x0019bd84   ebp = 0x0019bfa0
    Found by: call frame info with scanning
FWIW, the crashes in comment 0 and 1 happened on different pages in the Tp4 suite.
hb_blob_lock can apparently return NULL, but we seem to be passing its result to gfxFontUtils::FindPreferredSubtable without null-checking:

<http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxHarfBuzzShaper.cpp#731>
More precisely, I think you should check the blob length before accessing what hb_blob_lock() returns.
We need this fixed ASAP...
Assignee: nobody → jfkthame
blocking2.0: --- → ?
In gfxHarfBuzzShaper.cpp:

725         // get the cmap table and find offset to our subtable
726         mCmapTable = mFont->GetFontTable(TRUETYPE_TAG('c','m','a','p'));
727         if (!mCmapTable) {
728             NS_WARNING("failed to load cmap, glyphs will be missing");
729             return PR_FALSE;
730         }
731         const PRUint8* data = (const PRUint8*)hb_blob_lock(mCmapTable);

The conditional is wrong.  GetFontTable never returns NULL.  It returns a nil blob.  Should check for hb_blob_get_length() != 0 instead.

However, that *still* would be buggy because a CMAP table of, say, 1 byte, is not nil, but is too short and result in invalid memory read.  Now, one may argue that Windows doesn't accept such a font anyway, but I'm not sure what other codepaths reach this point.

Taking a quick look at the DWrite implementation, it looks to me like the DWrite backend accessing font tables without checking for their size first.  Ie. misses the equivalent of harfbuzz's sanitize().  Not sure about the rest of the code.
Correcting myself: GetFontTable is supposed to return null on table-not-found indeed.  But the very last line of gfxDWriteFonts.cpp returns a nil blob instead of null.

The rest of my comment is still valid and cause of concern though.
Attachment #471788 - Flags: review?(jdaggett) → review+
Attachment #471788 - Flags: approval2.0?
Comment on attachment 471788 [details] [diff] [review]
check cmap table length before accessing fields

+        if (aBufLength - 2 < offset) {
+            // this subtable is not valid - beyond end of buffer
+            continue;
+        }

This is OK, but wouldn't it be better to return 0 as soon as we spot something invalid? Makes life harder for the fuzzers.
Attachment #471788 - Flags: approval2.0? → approval2.0+
(In reply to comment #12)
> Comment on attachment 471788 [details] [diff] [review]
> check cmap table length before accessing fields
> 
> +        if (aBufLength - 2 < offset) {
> +            // this subtable is not valid - beyond end of buffer
> +            continue;
> +        }
> 
> This is OK, but wouldn't it be better to return 0 as soon as we spot something
> invalid? Makes life harder for the fuzzers.

Could do; though for downloaded fonts, bug 527276 will prevent bad cmap tables ever reaching this code anyway, so the only potential issue would be with installed platform fonts.
Pushed http://hg.mozilla.org/mozilla-central/rev/4b72a30f251e.
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
blocking2.0: ? → final+
Whiteboard: [orange]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: