XSLT crashes Firefox 5 and Firefox 7 (but not Firefox 4)

RESOLVED FIXED in Firefox 6

Status

()

--
critical
RESOLVED FIXED
7 years ago
3 years ago

People

(Reporter: travisemmitt, Assigned: peterv)

Tracking

(Blocks: 1 bug, {crash, regression, reproducible})

Trunk
mozilla7
crash, regression, reproducible
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox5 affected, firefox6+ fixed, firefox7+ fixed, status2.0 unaffected, status1.9.2 unaffected, status1.9.1 unaffected)

Details

(Whiteboard: [sg:critical?][qa-], crash signature, URL)

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

7 years ago
User-Agent:       Mozilla/5.0 (Windows NT 5.1; rv:5.0) Gecko/20100101 Firefox/5.0
Build Identifier: Mozilla/5.0 (Windows NT 5.1; rv:5.0) Gecko/20100101 Firefox/5.0

Some (but not all) XSLT crashes Firefox 5 on Windows 7 and XP.  This crash did not happen on Firefox 4 or earlier.

The crash happens on local XSLT files as well.

Reproducible: Always

Steps to Reproduce:
1. Load the page indicated in the url


Actual Results:  
Loading the XSLT at the cited URL crashes Firefox 5 but not 4 and earlier.

Expected Results:  
Firefox 5 should not crash, because Firefox 4 did not crash (and neither do IE or Chrome).

I haven't tried methodically whittling down the XSLT to find out whether it is a particular function or extension which is causing the crash.  If that is something that would be useful, I can do it; otherwise, I am hoping other people report similar problems.

Comment 1

7 years ago
Reproduced:
Mozilla/5.0 (X11; Linux x86_64; rv:7.0a1) Gecko/20110626 Firefox/7.0a1


0 	libc-2.13.so 	libc-2.13.so@0x32405 	
1 	libc-2.13.so 	libc-2.13.so@0x3567f 	
2 	libxul.so 	nsTArray<txOutputTransaction*, nsTArrayDefaultAllocator>::~nsTArray 	nsTArray.h:374
3 	ld-2.13.so 	ld-2.13.so@0x55f 	
4 	libc-2.13.so 	libc-2.13.so@0x6b9f 	
5 	libm-2.13.so 	libm-2.13.so@0x281fff 	
6 	ld-2.13.so 	ld-2.13.so@0xd711 	
7 	libstdc++.so.6.0.16 	libstdc++.so.6.0.16@0xbee4c 	
8 	libpthread-2.13.so 	libpthread-2.13.so@0xe11c 	
9 	libstdc++.so.6.0.16 	libstdc++.so.6.0.16@0xbcff5 	
10 	libstdc++.so.6.0.16 	libstdc++.so.6.0.16@0xbd022 	
11 	libstdc++.so.6.0.16 	libstdc++.so.6.0.16@0xbda3e 	
12 	libxul.so 	txResultBuffer::flushToHandler 	content/xslt/src/xslt/txBufferingHandler.cpp:477
13 	libxul.so 	txCopyOf::execute 	content/xslt/src/xslt/txInstructions.cpp:432
14 	libxul.so 	txXSLTProcessor::execute 	content/xslt/src/xslt/txXSLTProcessor.cpp:104
15 	libxul.so 	txMozillaXSLTProcessor::TransformToDoc 	content/xslt/src/xslt/txMozillaXSLTProcessor.cpp:676
16 	libxul.so 	nsTransformBlockerEvent::Run 	content/xslt/src/xslt/txMozillaXSLTProcessor.cpp:558
17 	libxul.so 	nsThread::ProcessNextEvent 	xpcom/threads/nsThread.cpp:617
18 	libxul.so 	NS_ProcessNextEvent_P 	obj-firefox/xpcom/build/nsThreadUtils.cpp:245
19 	libxul.so 	mozilla::ipc::MessagePump::Run 	ipc/glue/MessagePump.cpp:110
20 	libxul.so 	MessageLoop::Run 	ipc/chromium/src/base/message_loop.cc:218
21 	libxul.so 	nsBaseAppShell::Run 	widget/src/xpwidgets/nsBaseAppShell.cpp:189
22 	libxul.so 	nsAppStartup::Run 	toolkit/components/startup/nsAppStartup.cpp:222
23 	libxul.so 	XRE_main 	toolkit/xre/nsAppRunner.cpp:3573
24 	firefox 	main 	browser/app/nsBrowserApp.cpp:198
25 	libc-2.13.so 	libc-2.13.so@0x1eeac 	
26 	firefox 	firefox@0x193f
Crash Signature: [@ libc-2.13.so@0x32405 ]
Keywords: crash
OS: Windows 7 → All
Hardware: x86 → All
Version: unspecified → Trunk

Comment 2

7 years ago
Last good nightly: 2011-04-08 First bad nightly: 2011-04-09

Pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=27d6a4a5e20f&tochange=7d90da136b2c
Keywords: regression

Comment 3

7 years ago
bp-26a25d8c-953c-44d2-a39f-146542110626
[@ linux-gate.so@0x425 ] 

bp-650154ac-8a4d-4b10-984a-820e32110626
[@ linux-gate.so@0x425 ] 

bp-03f57bff-3748-4304-9c77-007d42110626
[@ linux-gate.so@0x425 ]

bp-2e307c77-cbec-465d-89aa-5d6b82110626
[@ linux-gate.so@0x425 ] 

bp-2bd4ebd1-eabe-4047-a868-4c7362110626
[@ libc-2.13.so@0x32405 ]
Crash Signature: [@ libc-2.13.so@0x32405 ] → [@ libc-2.13.so@0x32405 ][@ linux-gate.so@0x425 ]

Comment 4

7 years ago
0 	linux-gate.so 	linux-gate.so@0x425 	
1 	libc-2.13.so 	libc-2.13.so@0x2abb0 	
2 	libc-2.13.so 	libc-2.13.so@0x155ff3 	
3 	libc-2.13.so 	libc-2.13.so@0x2dfd1 	
4 	ld-2.13.so 	ld-2.13.so@0x9ba1 	
5 	libstdc++.so.6.0.16 	libstdc++.so.6.0.16@0xab4b 	
6 	ld-2.13.so 	ld-2.13.so@0x1da73 	
7 	libc-2.13.so 	libc-2.13.so@0xbf863 	
8 	libstdc++.so.6.0.16 	libstdc++.so.6.0.16@0x24440 	
9 	libstdc++.so.6.0.16 	libstdc++.so.6.0.16@0xc1607 	
10 	libc-2.13.so 	libc-2.13.so@0x15657f

Comment 5

7 years ago
The first bad revision is:
changeset:   67678:a6822a5df633
user:        Peter Van der Beken <peterv@propagandism.org>
date:        Thu Dec 02 11:12:27 2010 -0500
summary:     Fix for bug 603844 (Leak txUnknownHandler+ with transformToDocument(textnode)). r=sicking.

http://hg.mozilla.org/mozilla-central/rev/a6822a5df633

Updated

7 years ago
Component: File Handling → XSLT
Product: Firefox → Core
QA Contact: file.handling → xslt

Updated

7 years ago
Status: UNCONFIRMED → NEW
Depends on: 603844
Ever confirmed: true
Keywords: reproducible
Summary: XSLT crashes Firefox 5 (but not 4) → XSLT crashes Firefox 5 and Firefox 7 (but not Firefox 4)

Comment 6

7 years ago
Travis, are you able to post some related Report IDs from about:crashes, like I've done in comment 3? The important thing is "bp-" strings. My IDs are from Linux so it would be interesting to also see what they look like with Windows.

Comment 8

7 years ago
0 	ntdll.dll 	KiFastSystemCallRet 	
1 	ntdll.dll 	ZwWaitForSingleObject 	
2 	kernel32.dll 	WaitForSingleObjectEx 	
3 	kernel32.dll 	WaitForSingleObject 	
4 	xul.dll 	google_breakpad::ExceptionHandler::WriteMinidumpOnHandlerThread 	toolkit/crashreporter/google-breakpad/src/client/windows/handler/exception_handler.cc:763
5 	xul.dll 	google_breakpad::ExceptionHandler::HandlePureVirtualCall 	toolkit/crashreporter/google-breakpad/src/client/windows/handler/exception_handler.cc:707
6 	mozcrt19.dll 	_purecall 	obj-firefox/memory/jemalloc/crtsrc/purevirt.c:47
7 	xul.dll 	flushTransaction 	content/xslt/src/xslt/txBufferingHandler.cpp:477
8 	xul.dll 	txResultBuffer::flushToHandler 	content/xslt/src/xslt/txBufferingHandler.cpp:503
9 	xul.dll 	txResultTreeFragment::flushToHandler 	content/xslt/src/xslt/txRtfHandler.cpp:89
10 	xul.dll 	txCopyOf::execute 	content/xslt/src/xslt/txInstructions.cpp:432
11 	xul.dll 	txXSLTProcessor::execute 	content/xslt/src/xslt/txXSLTProcessor.cpp:104
12 	xul.dll 	txMozillaXSLTProcessor::TransformToDoc 	content/xslt/src/xslt/txMozillaXSLTProcessor.cpp:715
13 	xul.dll 	nsTransformBlockerEvent::Run 	content/xslt/src/xslt/txMozillaXSLTProcessor.cpp:595
14 	xul.dll 	nsThread::ProcessNextEvent 	xpcom/threads/nsThread.cpp:618
15 	xul.dll 	TimerThread::RemoveTimer 	xpcom/threads/TimerThread.cpp:417
16 	xul.dll 	MessageLoop::RunInternal 	ipc/chromium/src/base/message_loop.cc:219
17 	xul.dll 	MessageLoop::RunHandler 	ipc/chromium/src/base/message_loop.cc:202
18 	mozcrt19.dll 	_VEC_memzero 	
19 	xul.dll 	xul.dll@0x37057f 	
20 	firefox.exe 	firefox.exe@0x1bb7 	
21 	kernel32.dll 	GetCodePageFileInfo 	
22 	kernel32.dll 	BaseProcessStart 	
23 	firefox.exe 	firefox.exe@0x186f
Crash Signature: [@ libc-2.13.so@0x32405 ][@ linux-gate.so@0x425 ] → [@ libc-2.13.so@0x32405 ][@ linux-gate.so@0x425 ][@ _purecall | flushTransaction ]
Blocks: 603844
tracking-firefox6: --- → ?
tracking-firefox7: --- → ?
No longer depends on: 603844
Peter, can you look at this?
Assignee: nobody → peterv

Updated

7 years ago
Blocks: 532972
(Assignee)

Comment 13

7 years ago
Created attachment 542900 [details] [diff] [review]
v1

This needs the txUnknownHandler to flush while we're already flushing a different buffer to the result handler (flushing the rtf buffer for example). Apparently we didn't have a test for that.

This patch avoids the |delete this|'s that we have in the txUnknownHandler. Instead we forward the remaining hooks to the new result handler from the txUnknownHandler and keep the txUnknownHandler alive until the execution state dies. We clear the buffer so this shouldn't give too much bloat.
Attachment #542900 - Flags: review?(jonas)
(Assignee)

Comment 14

7 years ago
Created attachment 542903 [details] [diff] [review]
v1 (diff -w)
Comment on attachment 542900 [details] [diff] [review]
v1

Review of attachment 542900 [details] [diff] [review]:
-----------------------------------------------------------------

Please also add tests to not only ensure that we don't crash, but that we also output the correct result. For example that if the variable starts with <html> that we go into HTML mode etc. You can just use XSLTProcessor.transformToDocument to do that.

r=me with that

::: content/xslt/src/xslt/txUnknownHandler.cpp
@@ +107,5 @@
> +        // leak mEs->mResultHandler in createHandlerAndFlush and we'll crash on
> +        // the last line (delete this).
> +        NS_ASSERTION(mEs->mResultHandler == this,
> +                     "We're leaking mEs->mResultHandler and are going to "
> +                     "crash.");

The crash part sounds like it might no longer be true. At least that part of the comment is wrong. Maybe just say that we might crash since we're in unknown territory.
Attachment #542900 - Flags: review?(jonas) → review+

Updated

7 years ago
Group: core-security

Updated

7 years ago
tracking-firefox6: ? → -
tracking-firefox7: ? → +
XSLT is an area of recent interest amongst security researchers. If this instance was found on a live web server isn't it likely to be rediscovered? How common is the condition that causes the crash?

It would be much better to get this fix into Firefox 6 unless this is a risky patch.
status1.9.1: --- → unaffected
status1.9.2: --- → unaffected
status2.0: --- → unaffected
status-firefox5: --- → affected
status-firefox6: --- → affected
status-firefox7: --- → affected
tracking-firefox6: - → +
Whiteboard: [sg:critical?]
(Reporter)

Comment 17

7 years ago
The crash happens on my local copy of the file as well, so I don't want anyone thinking the web server configuration has anything to do with it.  Like I said, if you guys need help reducing the XSLT to the simplest possible code that creates a crash, I can do that.  I suspect the crash has something to do with either an XSLT extension or perhaps my use of the document() function, although it might be something else entirely.

Did you guys find the cause of the problem?

As for priority, all I can say is that I personally won't be upgrading to Firefox 5 until the crash is fixed, because I use Firefox to test my XML-based sites before rendering them into XHTML and pushing live.  Until I can safely use Firefox 5, I won't be able to recommend it to others, and might have to start recommending Chrome or IE9.  So if your goal is to maximize Firefox users, my recommendation is to fix the crash in Firefox 5 so that long-time developers like myself can continue to use (and promote) Firefox.
There's a reviewed fix attached to the bug, so yes, the problem has been located.
Travis, the suggestion in comment 16 to fix this for Firefox 6 means to fix it for the scheduled security update to Firefox 5 (scheduled for August 16).  Fixing it before that would involve doing a special release just for this fix, and while this is definitely a problem it's not clear that it's serious enough for that.  Typically we would only do that for serious compatibility regressions or security bugs being actively exploited.
(Reporter)

Comment 20

7 years ago
Okay, thanks for the clarification!
(Assignee)

Comment 21

7 years ago
Grmbl, I accidently checked in the testcases.

http://hg.mozilla.org/mozilla-central/rev/84f72e1d1fa2
Status: NEW → RESOLVED
Last Resolved: 7 years ago
status-firefox7: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla7
(Assignee)

Comment 22

7 years ago
Comment on attachment 542900 [details] [diff] [review]
v1

This is a fairly safe patch, we delay deleting an object while there might be callers that have a reference to it (and forward all uses of that object to the object that replaced it). We used to delete it right away and callers potentially used a dangling pointer. We should take this for Firefox 6.
Attachment #542900 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 23

7 years ago
Created attachment 543814 [details] [diff] [review]
v1
Attachment #542900 - Attachment is obsolete: true
Attachment #542903 - Attachment is obsolete: true
Attachment #543814 - Flags: review+
Attachment #543814 - Flags: approval-mozilla-aurora?
Attachment #542900 - Flags: approval-mozilla-aurora?

Comment 24

7 years ago
Comment on attachment 543814 [details] [diff] [review]
v1

this is already in Aurora 7 but we'd like it in Beta 7. Setting approval-mozilla-beta+
Attachment #543814 - Flags: approval-mozilla-aurora? → approval-mozilla-beta+
(Assignee)

Comment 25

7 years ago
(In reply to comment #24)
> this is already in Aurora 7 but we'd like it in Beta 7. Setting
> approval-mozilla-beta+

I don't understand, did you mean Beta 6?
qa+ for fix verification in Firefox 7
Whiteboard: [sg:critical?] → [sg:critical?][qa+]
I wasn't able to reproduce this problem on versions version 5.0.1 on XP, nor 5.0.1, 6.0.2, 7.09rc, nightly on Linux using the information on comment #0, so I am not able to verify it.
I notice that the URL of the testcase of the bug is behind authorization now. There is no file attached to the bug.

QA cannot verify.

Since the checked in testcase is passing, I'm going to assume it is good but QA cannot check manually.
Based on recent comments and my own failed attempt, QA cannot verify this fix due to an inaccessible testcase. Please provide a working testcase or simply mark VERIFIED if the checked-in test is enough.
Whiteboard: [sg:critical?][qa+] → [sg:critical?][qa-]
Group: core-security
Keywords: testcase-wanted
Keywords: testcase-wanted
You need to log in before you can comment on or make changes to this bug.