507452 - Crash (too much recursion in BindToTree) with feed full of "<p/><p/>"

Status: RESOLVED WORKSFORME

(Core :: DOM: HTML Parser, defect)

Description

[Carsten Book [:Tomcat]]

Attachment: stack

Version=3.5.3pre BuildID=20090730054515

Steps to reproduce:
-> Load http://inside.nike.com/blogs/nikefootball-en_IN/feeds/posts

(934.f10): Stack overflow - code c00000fd (first chance)
First chance exceptions are reported before any exception handling.
This exception may be expected and handled.
eax=0000000d ebx=7ffdb000 ecx=08250690 edx=082506a4 esi=00d05340 edi=011ef6f0
eip=0045111a esp=00033000 ebp=00033008 iopl=0         nv up ei pl nz na po nc
cs=001b  ss=0023  ds=0023  es=0023  fs=003b  gs=0000             efl=00010202
nspr4!_MD_CURRENT_THREAD+0xa:
0045111a ff155c864600    call    dword ptr [nspr4!_imp__TlsGetValue (0046865c)] ds:0023:0046865c={kernel32!TlsGetValue (7c8097e0)}

Event Type: Exception

Exception Faulting Address: 0x45111a
First Chance Exception Type: STATUS_STACK_OVERFLOW (0xC00000FD)

Faulting Instruction:0045111a call dword ptr [nspr4!_imp__tlsgetvalue (0046865c)]

Basic Block:
    0045111a call dword ptr [nspr4!_imp__tlsgetvalue (0046865c)]

Exception Hash (Major/Minor): 0x2c027849.0x143d2732

Stack Trace:
nspr4!_MD_CURRENT_THREAD+0xa
nspr4!PR_GetCurrentThread+0x16
nspr4!PR_GetThreadPrivate+0xb
xpcom_core!NS_LogAddRef_P+0x1b
gklayout!nsGenericElement::AddRef+0xa1
gklayout!nsHTMLParagraphElement::AddRef+0xd
gklayout!nsGenericElement::QueryInterface+0x3b0
gklayout!nsHTMLParagraphElement::QueryInterface+0x65
xpcom_core!nsQueryInterface::operator()+0x2d
gklayout!nsCOMPtr<nsIContent>::assign_from_qi+0x19
gklayout!nsCOMPtr<nsIContent>::nsCOMPtr<nsIContent>+0x34
gklayout!nsCOMPtr<nsIContent>::Assert_NoQueryNeeded+0x2e
gklayout!nsCOMPtr<nsIContent>::nsCOMPtr<nsIContent>+0x48
gklayout!nsGenericElement::BindToTree+0x66c
gklayout!nsStyledElement::BindToTree+0x21
gklayout!nsMappedAttributeElement::BindToTree+0x21
gklayout!nsGenericHTMLElement::BindToTree+0x21
gklayout!nsGenericElement::BindToTree+0x697
gklayout!nsStyledElement::BindToTree+0x21
gklayout!nsMappedAttributeElement::BindToTree+0x21
gklayout!nsGenericHTMLElement::BindToTree+0x21
gklayout!nsGenericElement::BindToTree+0x697
gklayout!nsStyledElement::BindToTree+0x21
gklayout!nsMappedAttributeElement::BindToTree+0x21
gklayout!nsGenericHTMLElement::BindToTree+0x21
gklayout!nsGenericElement::BindToTree+0x697
gklayout!nsStyledElement::BindToTree+0x21
gklayout!nsMappedAttributeElement::BindToTree+0x21
gklayout!nsGenericHTMLElement::BindToTree+0x21
gklayout!nsGenericElement::BindToTree+0x697
gklayout!nsStyledElement::BindToTree+0x21
gklayout!nsMappedAttributeElement::BindToTree+0x21
gklayout!nsGenericHTMLElement::BindToTree+0x21
gklayout!nsGenericElement::BindToTree+0x697
gklayout!nsStyledElement::BindToTree+0x21
gklayout!nsMappedAttributeElement::BindToTree+0x21
gklayout!nsGenericHTMLElement::BindToTree+0x21
gklayout!nsGenericElement::BindToTree+0x697
gklayout!nsStyledElement::BindToTree+0x21
gklayout!nsMappedAttributeElement::BindToTree+0x21
gklayout!nsGenericHTMLElement::BindToTree+0x21
gklayout!nsGenericElement::BindToTree+0x697
gklayout!nsStyledElement::BindToTree+0x21
gklayout!nsMappedAttributeElement::BindToTree+0x21
gklayout!nsGenericHTMLElement::BindToTree+0x21
gklayout!nsGenericElement::BindToTree+0x697
gklayout!nsStyledElement::BindToTree+0x21
gklayout!nsMappedAttributeElement::BindToTree+0x21
gklayout!nsGenericHTMLElement::BindToTree+0x21
gklayout!nsGenericElement::BindToTree+0x697
gklayout!nsStyledElement::BindToTree+0x21
gklayout!nsMappedAttributeElement::BindToTree+0x21
gklayout!nsGenericHTMLElement::BindToTree+0x21
gklayout!nsGenericElement::BindToTree+0x697
gklayout!nsStyledElement::BindToTree+0x21
gklayout!nsMappedAttributeElement::BindToTree+0x21
gklayout!nsGenericHTMLElement::BindToTree+0x21
gklayout!nsGenericElement::BindToTree+0x697
gklayout!nsStyledElement::BindToTree+0x21
gklayout!nsMappedAttributeElement::BindToTree+0x21
gklayout!nsGenericHTMLElement::BindToTree+0x21
gklayout!nsGenericElement::BindToTree+0x697
gklayout!nsStyledElement::BindToTree+0x21
gklayout!nsMappedAttributeElement::BindToTree+0x21
Instruction Address: 0x000000000045111a

Description: Stack Overflow
Short Description: StackOverflow
Exploitability Classification: UNKNOWN
Recommended Bug Title: Stack Overflow starting at nspr4!_MD_CURRENT_THREAD+0x000000000000000a (Hash=0x2c027849.0x143d2732)

FAULTING_IP: 
gklayout!nsStyledElement::BindToTree+21 [c:\work\mozilla\builds\1.9.1\mozilla\content\base\src\nsstyledelement.cpp @ 157]
02e80411 8945fc          mov     dword ptr [ebp-4],eax

EXCEPTION_RECORD:  ffffffff -- (.exr ffffffffffffffff)
ExceptionAddress: 0045111a (nspr4!_MD_CURRENT_THREAD+0x0000000a)
   ExceptionCode: c00000fd (Stack overflow)
  ExceptionFlags: 00000000
NumberParameters: 2
   Parameter[0]: 00000001
   Parameter[1]: 00032ffc

FAULTING_THREAD:  00000f10

BUGCHECK_STR:  c00000fd

DEFAULT_BUCKET_ID:  STATUS_STACKOVERFLOW

PROCESS_NAME:  firefox.exe

ERROR_CODE: (NTSTATUS) 0xc00000fd - A new guard page for the stack cannot be created.

RECURRING_STACK: From frames 0xe to 0x11

LAST_CONTROL_TRANSFER:  from 004495a6 to 0045111a

FOLLOWUP_IP: 
gklayout!nsStyledElement::BindToTree+21 [c:\work\mozilla\builds\1.9.1\mozilla\content\base\src\nsstyledelement.cpp @ 157]
02e80411 8945fc          mov     dword ptr [ebp-4],eax

FAULTING_SOURCE_CODE:  
   153:                             PRBool aCompileEventHandlers)
   154: {
   155:   nsresult rv = nsStyledElementBase::BindToTree(aDocument, aParent,
   156:                                                 aBindingParent,
>  157:                                                 aCompileEventHandlers);
   158:   NS_ENSURE_SUCCESS(rv, rv);
   159: 
   160:   // XXXbz if we already have a style attr parsed, this won't do
   161:   // anything... need to fix that.
   162:   ReparseStyleAttribute(PR_FALSE);


SYMBOL_STACK_INDEX:  e

SYMBOL_NAME:  gklayout!nsStyledElement::BindToTree+21

FOLLOWUP_NAME:  MachineOwner

MODULE_NAME: gklayout

IMAGE_NAME:  gklayout.dll

DEBUG_FLR_IMAGE_TIMESTAMP:  4a719fa1

STACK_COMMAND:  ~0s ; kb

FAILURE_BUCKET_ID:  c00000fd_gklayout!nsStyledElement::BindToTree+21

BUCKET_ID:  c00000fd_gklayout!nsStyledElement::BindToTree+21

Comment 1

[Ludovic Hirlimann [:Usul]]

crashes on Mac too 94fde5f8-9ad6-4b33-86af-dd0082090730

Comment 2

[Martijn Wargers (dead)]

Attachment: testcase

Comment 3

[Jesse Ruderman]

Stack exhaustion is usually not a security hole.  Any particular reason this bug is marked as security-sensitive?

Comment 4

[Jesse Ruderman]

Related to bug 474181, or is it just a coincidence that they both crash with too-much-recursion in BindToTree?

Comment 5

[Boris Zbarsky [:bzbarsky]]

Coincidence; I get a crash in a different place with this bug.

That said, this is just a dup of our various other "deep dom tree will run out of stack" bugs.

Comment 6

[Jesse Ruderman]

Like bug 323394?  I could believe that, but:

1) What's going on at the parsing level?  Our HTML parser would normally treat these paragraphs as siblings and limit the depth.  Our XML parser would normally treat these paragraphs as closed.  So how are we ending up with a deep tree?

2) We should inform Nike that their feed crashes Firefox.

Comment 7

[Boris Zbarsky [:bzbarsky]]

> Like bug 323394? 

Yes.

> 1) What's going on at the parsing level?

Good question.  Far as I can see, parser returns a document fragment with 30000-some sibling <p> nodes (<p> closes <p> in HTML parser, so all of them are empty here too).

I'll try to do some digging on Monday into why the end up as descendants of each other.

Comment 8

[Boris Zbarsky [:bzbarsky]]

OK.  So I was totally wrong.  This particular testcase causes the parser to produce lots of nested <p>.

Why?

First, the caller (nsScriptableUnescapeHTML::ParseFragment) uses eViewFragment as the mode.  This means the parser should suspend containment checking, and in particular means that <p> does not close <p>.

Second, in CNavDTD::WillHandleStartTag we do hit the stackDepth > MAX_REFLOW_DEPTH case, but since <p> has kHandleStrayTag set, we don't close it out.  Usually this is ok, since <p> can't contain blocks and non-blocks we close out earlier in the same function, so we can't get into trouble here... unless we have a bunch of nested <p>.

Over to HTML parser, since it seems like eViewFragment ought not to lead to bustage like this; maybe we should not do the kHandleStrayTag check in eViewFragment mode?

Comment 9

[Boris Zbarsky [:bzbarsky]]

Note that I hit a similar crash with the HTML5 parser enabled; I don't know whether we hit the same codepath in that case or not.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Note that we have exactly three consumers who can trigger eViewFragment as the mode (using eDTDMode_fragment):

1)  HTML paste code.
2)  Copy code converting HTML to plaintext via nsHTMLFormatConverter.
3)  Feed parsing code.

Of these, only #3 allows random people to control the string coming in; perhaps it should stop using the fragment parsing mode?

Comment 11

[Johnny Stenback (:jst)]

Is this still an issue? Either way, doesn't sound critical to Firefox 4, but if this is more common than it sounds, please say so here and renominate.

Comment 12

[Daniel Veditz [:dveditz]]

Although the crash is a duplicate of bug 485941 we can leave this one open to figure out why we think the paragraph elements are nested in the first place.

Comment 13

[Andrei Purice]

Closing this as Resolved > Worksforme as there are no more crashes on the latest versions Firefox Nightly 91.0a1, beta 90.0b7 or release 89.0 using the provided link and test case.