Closed
Bug 687744
Opened 13 years ago
Closed 13 years ago
Crash [@ nsHtml5Parser::Parse(const nsAString_internal &, void *, const nsACString_internal & aContentType={...}, int, nsDTDMode) ] | [@ mozalloc.dll@0x193d ]
Categories
(Core :: DOM: HTML Parser, defect)
Tracking
()
RESOLVED
FIXED
mozilla10
People
(Reporter: bc, Assigned: hsivonen)
References
()
Details
(Keywords: crash, Whiteboard: [sg:investigate][inbound])
Crash Data
Attachments
(2 files, 2 obsolete files)
55.27 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
2.09 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
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•13 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•13 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•13 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•13 years ago
|
||
Another data point is that the vm where I tested this only has 2G of ram.
Comment 5•13 years ago
|
||
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•13 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•13 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•13 years ago
|
||
I'll develop a fix with the assumption that the string passed to nsHtml5Parser::Parse is too large.
Assignee | ||
Comment 8•13 years ago
|
||
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.
Comment 9•13 years ago
|
||
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•13 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•13 years ago
|
||
Attachment #562733 -
Attachment is obsolete: true
Attachment #562733 -
Flags: review?(Olli.Pettay)
Attachment #562745 -
Flags: review?(Olli.Pettay)
Comment 12•13 years ago
|
||
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•13 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•13 years ago
|
||
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•13 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 15•13 years ago
|
||
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•13 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•13 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
Comment 18•13 years ago
|
||
I think landing this should be fine...
Assignee | ||
Comment 19•13 years ago
|
||
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)
Updated•13 years ago
|
Attachment #567040 -
Flags: review?(Olli.Pettay) → review+
Assignee | ||
Comment 20•13 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/daef044609ce
Whiteboard: [sg:investigate] → [sg:investigate][inbound]
Target Milestone: --- → mozilla10
Assignee | ||
Comment 21•13 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/daef044609ce
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Assignee | ||
Comment 22•13 years ago
|
||
This caused bug 696651, which is interesting. Something is wrong about the document.write() changes landed here. :-(
Blocks: 696651
Assignee | ||
Comment 23•11 years ago
|
||
Do we have a reason to keep this hidden?
Updated•9 years ago
|
Group: core-security → core-security-release
Updated•9 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•