Closed Bug 667315 Opened 13 years ago Closed 13 years ago
XSLT crashes Firefox 5 and Firefox 7 (but not Firefox 4)
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.
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 ]
OS: Windows 7 → All
Hardware: x86 → All
Version: unspecified → Trunk
Last good nightly: 2011-04-08 First bad nightly: 2011-04-09 Pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=27d6a4a5e20f&tochange=7d90da136b2c
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 ]
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
The first bad revision is: changeset: 67678:a6822a5df633 user: Peter Van der Beken <email@example.com> 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
Component: File Handling → XSLT
Product: Firefox → Core
QA Contact: file.handling → xslt
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.
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 ]
Peter, can you look at this?
Assignee: nobody → peterv
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)
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+
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.
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.
Okay, thanks for the clarification!
Grmbl, I accidently checked in the testcases. http://hg.mozilla.org/mozilla-central/rev/84f72e1d1fa2
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.
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+
(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-]
You need to log in before you can comment on or make changes to this bug.