Crash [@ nsHtml5Parser::Parse(const nsAString_internal &, void *, const nsACString_internal & aContentType={...}, int, nsDTDMode) ] | [@ mozalloc.dll@0x193d ]

RESOLVED FIXED in mozilla10

Status

()

Core
HTML: Parser
--
critical
RESOLVED FIXED
6 years ago
2 years ago

People

(Reporter: bc, Assigned: hsivonen)

Tracking

(Blocks: 1 bug, {crash})

Trunk
mozilla10
x86
Windows XP
crash
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:investigate][inbound], crash signature, URL)

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

6 years ago
1. http://www.hunger.hu/win.html
2. Crash Nightly Windows XP debug and opt. I don't see it anywhere else.

>	msvcr80d.dll!memcpy(unsigned char * dst=0x3e1c0040, unsigned char * src=0x0012afb8, unsigned long count=164102720)  Line 188	Asm
 	xul.dll!nsHtml5Parser::Parse(const nsAString_internal & aSourceBuffer={...}, void * aKey=0x073602bc, const nsACString_internal & aContentType={...}, int aLastCall=1, nsDTDMode aMode=eDTDMode_autodetect)  Line 302 + 0x29 bytes	C++
 	xul.dll!nsHTMLDocument::WriteCommon(JSContext * cx=0x07253ad8, const nsAString_internal & aText={...}, int aNewlineTerminate=0)  Line 1952 + 0x62 bytes	C++
 	xul.dll!nsHTMLDocument::Write(const nsAString_internal & aText={...}, JSContext * cx=0x07253ad8)  Line 1966	C++
 	xul.dll!nsIDOMHTMLDocument_Write(JSContext * cx=0x07253ad8, unsigned int argc=1, jsval_layout * vp=0x04e40108)  Line 17991 + 0x27 bytes	C++

ted's exploitable tool ranks this as highly exploitable. Beta/Aurora aborts with out of memory.

not a recent regression

bp-051617db-90ed-4493-8390-152602110919
bp-943d9cb3-0c2c-46a8-9342-d71952110919
bp-9af50bd2-debb-444c-a0dd-8bb762110919

low volume
https://crash-stats.mozilla.com/query/query?do_query=1&query_type=contains&query=mozalloc.dll%400x193d

https://crash-stats.mozilla.com/query/query?do_query=1&query_type=contains&query=nsHtml5Parser%3A%3AParse

This testcase is old and public. see bug 300288.
(Assignee)

Comment 1

6 years ago
(In reply to Bob Clary [:bc:] from comment #0)
> >	msvcr80d.dll!memcpy(unsigned char * dst=0x3e1c0040, unsigned char * src=0x0012afb8, unsigned long count=164102720)  Line 188	Asm

Count is less than 2^31 here, so it appears we are *not* hitting a signed vs. unsigned issue leading to negative length.

> ted's exploitable tool ranks this as highly exploitable.

What does the tool look at?

> Beta/Aurora aborts with out of memory.

That makes sense to me. Right before the memcpy is called, the target buffer is allocated with |new PRUnichar[size]| which should be infallible.

> bp-051617db-90ed-4493-8390-152602110919
> bp-943d9cb3-0c2c-46a8-9342-d71952110919
> bp-9af50bd2-debb-444c-a0dd-8bb762110919

These look like the crash is in mozalloc, which implements infallibility by crashing deliberately. Where did the msvcr80d.dll!memcpy crash location come from?
(Reporter)

Comment 2

6 years ago
(In reply to Henri Sivonen (:hsivonen) from comment #1)
> (In reply to Bob Clary [:bc:] from comment #0)
> > >	msvcr80d.dll!memcpy(unsigned char * dst=0x3e1c0040, unsigned char * src=0x0012afb8, unsigned long count=164102720)  Line 188	Asm
> 
> Count is less than 2^31 here, so it appears we are *not* hitting a signed
> vs. unsigned issue leading to negative length.
> 
> > ted's exploitable tool ranks this as highly exploitable.
> 
> What does the tool look at?
> 

https://hg.mozilla.org/users/tmielczarek_mozilla.com/exploitable/

> > Beta/Aurora aborts with out of memory.
> 
 
> These look like the crash is in mozalloc, which implements infallibility by
> crashing deliberately. Where did the msvcr80d.dll!memcpy crash location come
> from?

A debug build.
(Assignee)

Comment 3

6 years ago
Should I be considering more than the following possibilities:
 1) The length variable overflows. (Doesn't seem to be the case from the stack or from doing the math of what the test case does.)
 2) The target buffer is bogus. (Should not happen unless the infallible malloc is broken.)
 3) The infallible malloc is killing the process. (This would need the opt build stack traces to be right and the debug build stack trace to be wrong.)
 4) The input string has a bogus buffer.
?
(Reporter)

Comment 4

6 years ago
Another data point is that the vm where I tested this only has 2G of ram.
Note that I didn't write the underlying exploitability checker, I just wrote a little wrapper to make its analysis available. The underlying implementation is here:
http://code.google.com/p/google-breakpad/source/browse/trunk/src/processor/exploitability_win.cc

It checks what type of exception was encountered. For illegal accesses, it also checks whether it was read/write/execute, and also whether the access was near NULL or not. It then also tries to disassemble the instructions around the faulting instruction pointer and determine whether the faulting instruction is a control flow or string operation.
(Reporter)

Updated

6 years ago
Crash Signature: [@ mozalloc.dll@0x193d ] → [@ mozalloc.dll@0x193d ] [@ nsHtml5Parser::Parse(const nsAString_internal &, void *, const nsACString_internal & aContentType={...}, int, nsDTDMode) ]
(Assignee)

Comment 6

6 years ago
(In reply to Ted Mielczarek [:ted, :luser] from comment #5)
> It checks what type of exception was encountered. For illegal accesses, it
> also checks whether it was read/write/execute, and also whether the access
> was near NULL or not. It then also tries to disassemble the instructions
> around the faulting instruction pointer and determine whether the faulting
> instruction is a control flow or string operation.

Is it possible that this crash is actually a deliberate mozalloc abort that's misclassified and the crash location in the debug build in wrong and in the opt builds it's right?

If the debug build crash location is right, I don't really know what's wrong and how to fix. Ideas?
(Assignee)

Comment 7

6 years ago
I'll develop a fix with the assumption that the string passed to nsHtml5Parser::Parse is too large.
(Assignee)

Comment 8

6 years ago
Created attachment 562733 [details] [diff] [review]
Deal better with large strings passed to the HTML parser

This patch does four things:
 1) Signal OOM if a string passed to the parser is too long to have its length represented by a signed 32-bit integer. (The parser uses signed integers internally due to its Java heritage.)
 2) Avoid copying the data of strings passed to the parser.
 3) If some data has to be copied (when documnet.write blocks), allocate the new buffer in the fallible mode and signal OOM in case of failure.
 4) If document.write() signals OOM, mark the parser as broken and terminate it to avoid cases where e.g. a script end tag is dropped due to OOM and the rest of the input becomes executable script.

I haven't pushed this to try, since pushing this to try would reveal what the bug is. Since what the bug is is pretty obvious from the fix, I wrote a commit comment that says what's being fixed.

Dealing with OOM in the case of data originating from network is bug 573078. I think that one should be fixed on top of this fix.
Assignee: nobody → hsivonen
Status: NEW → ASSIGNED
Attachment #562733 - Flags: review?(Olli.Pettay)
Comment on attachment 562733 [details] [diff] [review]
Deal better with large strings passed to the HTML parser


>+class nsHtml5ParserTerminator : public nsRunnable
>+{
>+  private:
>+    nsRefPtr<nsHtml5Parser> mParser;
>+  public:
>+    nsHtml5ParserTerminator(nsHtml5Parser* aParser)
>+      : mParser(aParser)
>+    {}
>+    NS_IMETHODIMP Run()
>+    {
>+      mParser->Terminate();
>+      return NS_OK;
>+    }
>+};
>+
>+void
>+nsHtml5TreeOpExecutor::MarkAsBroken()
>+{
>+  NS_ASSERTION(NS_IsMainThread(), "Wrong thread!");
>+  NS_ASSERTION(!mFragmentMode, "Fragment parsers can't be broken!");
>+  mBroken = true;
>+  if (mStreamParser) {
>+    mStreamParser->Terminate();
>+  }
>+  // We are under memory pressure, but let's hope the following allocation
>+  // works out so that we get to terminate and clean up the parser from
>+  // a safer point.
>+  if (mParser) { // can mParser ever be null here?
>+    nsCOMPtr<nsIRunnable> terminator = new nsHtml5ParserTerminator(GetParser());
>+    if (NS_FAILED(NS_DispatchToMainThread(terminator))) {
>+      NS_WARNING("failed to dispatch executor flush event");
>+    }

Could you use NS_NewRunnableMethod here?

>+    bool                        mBroken;
This is initialized to false somewhere, right?


>+    inline bool IsBroken() {
{ should be in the next line, but apparently the file has its own coding style,
so no need to change.

> nsHtml5UTF16Buffer::~nsHtml5UTF16Buffer()
> {
>   MOZ_COUNT_DTOR(nsHtml5UTF16Buffer);
>+  NS_ASSERTION(mHasWrappedBuffer,
>+      "Forgot to let go of wrapped buffer. Double-free ahead!");
Indentation.



>   delete[] buffer;
> }
> 
>+bool
>+nsHtml5UTF16Buffer::Unwrap()
>+{
>+#ifdef DEBUG
>+  NS_PRECONDITION(mHasWrappedBuffer, "Trying to unwrap a non-wrapped buffer.");
>+  mHasWrappedBuffer = false;
This is strange. Debug build might get different behavior than opt build.
Isn't the NS_PRECONDITION enough?


>+
>+    /**
>+     * Lets go of a wrapped buffer possibly copying data into a heap-allocated
>+     * buffer owned by this object.
>+     * @return false if there was an allocation failure and true otherwise
>+     */
>+    bool Unwrap();
I wonder if Unwrap just use StringBuffer->AddRef, if the string is using stringbuffers...


(I'm running out of battery power... will continue soon)
(Assignee)

Comment 10

6 years ago
(In reply to Olli Pettay [:smaug] from comment #9)
> >+    bool                        mBroken;
> This is initialized to false somewhere, right?

nsHtml5TreeOpExecutor has zeroing operator new.

> >+bool
> >+nsHtml5UTF16Buffer::Unwrap()
> >+{
> >+#ifdef DEBUG
> >+  NS_PRECONDITION(mHasWrappedBuffer, "Trying to unwrap a non-wrapped buffer.");
> >+  mHasWrappedBuffer = false;
> This is strange. Debug build might get different behavior than opt build.
> Isn't the NS_PRECONDITION enough?

mHasWrappedBuffer doesn't exist in opt builds. It's only used for asserting.

> >+
> >+    /**
> >+     * Lets go of a wrapped buffer possibly copying data into a heap-allocated
> >+     * buffer owned by this object.
> >+     * @return false if there was an allocation failure and true otherwise
> >+     */
> >+    bool Unwrap();
> I wonder if Unwrap just use StringBuffer->AddRef, if the string is using
> stringbuffers...

Do you mean the comment isn't clear enough?

I noticed that the memcpy in Unwrap() copies from the wrong offset (would be nice to push this to try!).
(Assignee)

Comment 11

6 years ago
Created attachment 562745 [details] [diff] [review]
Deal better with large strings passed to the HTML parser, with fixed memcpy offset
Attachment #562733 - Attachment is obsolete: true
Attachment #562733 - Flags: review?(Olli.Pettay)
Attachment #562745 - Flags: review?(Olli.Pettay)
Comment on attachment 562745 [details] [diff] [review]
Deal better with large strings passed to the HTML parser, with fixed memcpy offset

>@@ -290,22 +297,19 @@ nsHtml5Parser::Parse(const nsAString& aS
> 
>   NS_ASSERTION(!(mStreamParser && !aKey),
>                "Got a null key in a non-script-created parser");
> 
>   if (aSourceBuffer.IsEmpty()) {
>     return NS_OK;
>   }
> 
>+  // MUST call Unwrap() on this object before returning!
>   nsRefPtr<nsHtml5UTF16Buffer> buffer =
>-    new nsHtml5UTF16Buffer(aSourceBuffer.Length());
>-  memcpy(buffer->getBuffer(),
>-         aSourceBuffer.BeginReading(),
>-         aSourceBuffer.Length() * sizeof(PRUnichar));
>-  buffer->setEnd(aSourceBuffer.Length());
>+    new nsHtml5UTF16Buffer(aSourceBuffer);
> 
>   // The buffer is inserted to the stream here in case it won't be parsed
>   // to completion.
>   // The script is identified by aKey. If there's nothing in the buffer
>   // chain for that key, we'll insert at the head of the queue.
>   // When the script leaves something in the queue, a zero-length
>   // key-holder "buffer" is inserted in the queue. If the same script
>   // leaves something in the chain again, it will be inserted immediately
>@@ -426,16 +430,20 @@ nsHtml5Parser::Parse(const nsAString& aS
>     }
>     buffer->setStart(originalStart);
> 
>     mDocWriteSpeculativeTreeBuilder->Flush();
>     mDocWriteSpeculativeTreeBuilder->DropHandles();
>     mExecutor->FlushSpeculativeLoads();
>   }
> 
>+  if (!buffer->Unwrap()) {
>+    mExecutor->MarkAsBroken();
>+    return NS_ERROR_OUT_OF_MEMORY;
>+  }
This looks quite error prone. If someone forgets to call Unwrap, the buffer->buffer may point to
random memory. 
Could you perhaps add Clone() method to nsHtml5UTF16Buffer?
Normally the buffer would be used as a stack variable without unwrapping, but if heap object is needed, one
should call Clone() which unwraps. Would that work? 


>+nsHtml5UTF16Buffer::Unwrap()
>+{
>+#ifdef DEBUG
>+  NS_PRECONDITION(mHasWrappedBuffer, "Trying to unwrap a non-wrapped buffer.");
>+  mHasWrappedBuffer = false;
>+#endif
Ah, mHasWrappedBuffer is DEBUG only variable
(Assignee)

Comment 13

6 years ago
> Could you perhaps add Clone() method to nsHtml5UTF16Buffer?
> Normally the buffer would be used as a stack variable without unwrapping,
> but if heap object is needed, one
> should call Clone() which unwraps. Would that work? 

I could do that, but then I'd like to fix bug 573078 at the same time to avoid having to support the case where this bug is fixed but bug 573078 isn't. (That is, it would be easier to not have an intermediate state that support three kinds of buffers: wrapped, owned but infallible and owned but fallible. I'd rather go straight to wrapped & owned but fallible.) Are you OK with having to land bug 573078 together with this one?
(Assignee)

Comment 14

6 years ago
Created attachment 563040 [details] [diff] [review]
Make all nsHtml5UTF16Buffer allocations in the HTML parser fallible

This patch addresses the review comments and also fixes bug 573078 in order to avoid a weird intermediate state between fixing this bug and fixing bug 573078.

This patch is mostly untested in order to avoid having the patch show up on try. :-( I don't know what the right procedure of testing patches of this size for confidential bugs is.
Attachment #562745 - Attachment is obsolete: true
Attachment #562745 - Flags: review?(Olli.Pettay)
Attachment #563040 - Flags: review?(Olli.Pettay)
(Assignee)

Updated

6 years ago
Attachment #563040 - Attachment description: Make all buffer allocations in the HTML parser fallible → Make all nsHtml5UTF16Buffer allocations in the HTML parser fallible
Comment on attachment 563040 [details] [diff] [review]
Make all nsHtml5UTF16Buffer allocations in the HTML parser fallible


>+    bool                        mBroken;
This is initialized to false somewhere, right?
Attachment #563040 - Flags: review?(Olli.Pettay) → review+
(Assignee)

Comment 16

6 years ago
dveditz, is it OK to land this at this point of the cycle and to push to try shortly before landing? If not, what's the right procedure?

(In reply to Olli Pettay [:smaug] from comment #15)
> >+    bool                        mBroken;
> This is initialized to false somewhere, right?

Yes, by a zeroing operator new.
(Reporter)

Comment 17

6 years ago
Fyi, a majority of tests (1.9.2, beta, aurora, nightly|mac 10.5/10.6, linux, windows xp/7) with the urls

http://stuff.rebro.org/test.html
http://eaea.sirdarckcat.net/firefox-memoryexhaust1.html
http://xgold-team-en-or.xooit.fr/index.php

from bug 659920 result in oom aborts now that bug 652438 is fixed however other stacks are still produced. nsHtml5Parser::Parse is the most common and reproducible signature. The remaining signatures appear to be randomly reproducible and may be due to memory corruption.

branch  os_name    cpu/build_cpu signature
1.9.2   Linux      x86/x86       dosprintf
1.9.2   Linux      x86_64/x86_64 NS_CopyUnicodeToNative
1.9.2   Windows NT x86/x86       NS_LogAddRef_P
1.9.2   Windows NT x86/x86       nsCycleCollectingAutoRefCnt::get()
beta    Linux      x86_64/x86_64 utf16_to_isolatin1
aurora  Linux      x86/x86       dosprintf
aurora  Linux      x86_64/x86_64 nsCString::get
nightly Linux      x86/x86       libm-2.13.so@0xefff
nightly Linux      x86_64/x86_64 @0x0 | PR_GetThreadPrivate
I think landing this should be fine...
(Assignee)

Comment 19

6 years ago
Created attachment 567040 [details] [diff] [review]
Minor additional tweaks to debug code

Tryserver testing revealed a couple of errors in debug code that I failed to catch in local smoketests. This patch fixes those.
Attachment #567040 - Flags: review?(Olli.Pettay)
Attachment #567040 - Flags: review?(Olli.Pettay) → review+
(Assignee)

Comment 20

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/daef044609ce
Whiteboard: [sg:investigate] → [sg:investigate][inbound]
Target Milestone: --- → mozilla10
(Assignee)

Comment 21

6 years ago
https://hg.mozilla.org/mozilla-central/rev/daef044609ce
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
(Assignee)

Comment 22

6 years ago
This caused bug 696651, which is interesting. Something is wrong about the document.write() changes landed here. :-(
Blocks: 696651
(Assignee)

Comment 23

4 years ago
Do we have a reason to keep this hidden?

Updated

2 years ago
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.