Last Comment Bug 528300 - Crash [@ txExecutionState::~txExecutionState]
: Crash [@ txExecutionState::~txExecutionState]
: crash, testcase, verified1.9.0.18, verified1.9.1
Product: Core
Classification: Components
Component: XSLT (show other bugs)
: Trunk
: x86 Windows XP
: -- critical (vote)
: mozilla1.9.2
Assigned To: Peter Van der Beken [:peterv]
: 528488 (view as bug list)
Depends on:
Blocks: 528561
  Show dependency treegraph
Reported: 2009-11-12 12:48 PST by Paul Nickerson
Modified: 2011-06-13 10:01 PDT (History)
8 users (show)
mbeltzner: blocking1.9.2+
dveditz: blocking1.9.0.18+
dveditz: wanted1.9.0.x+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

testcase (don't load from bugzilla) (464 bytes, text/xml)
2009-11-12 12:48 PST, Paul Nickerson
no flags Details
stack (1.44 KB, text/plain)
2009-11-12 12:49 PST, Paul Nickerson
no flags Details
v1 (713 bytes, patch)
2009-11-14 12:03 PST, Peter Van der Beken [:peterv]
no flags Details | Diff | Review
v1 (6.44 KB, patch)
2009-11-14 12:10 PST, Peter Van der Beken [:peterv]
no flags Details | Diff | Review
v1.1 (8.36 KB, patch)
2009-11-15 12:48 PST, Peter Van der Beken [:peterv]
jonas: review+
pvnick: review+
Details | Diff | Review
v1.2 (8.60 KB, patch)
2009-11-22 10:24 PST, Peter Van der Beken [:peterv]
jonas: review+
dveditz: approval1.9.1.8+
dveditz: approval1.9.0.18+
Details | Diff | Review
1.8.0 version (7.87 KB, patch)
2010-01-27 05:14 PST, Martin Stránský
no flags Details | Diff | Review

Description Paul Nickerson 2009-11-12 12:48:08 PST
Created attachment 412043 [details]
testcase (don't load from bugzilla)

Load the testcase and refresh if it doesn't crash right away. EIP is sometimes different, especially when extra fluff is added to the transform and to the original document, so IMO this is exploitable.
Comment 1 Paul Nickerson 2009-11-12 12:49:34 PST
Created attachment 412044 [details]
Comment 2 Paul Nickerson 2009-11-12 12:53:00 PST
WFM unless I save the file locally then open it.
Comment 3 Peter Van der Beken [:peterv] 2009-11-14 12:03:04 PST
Created attachment 412409 [details] [diff] [review]

Still need to add the crashtest.
Comment 4 Peter Van der Beken [:peterv] 2009-11-14 12:10:02 PST
Created attachment 412413 [details] [diff] [review]
Comment 5 Peter Van der Beken [:peterv] 2009-11-15 12:38:19 PST
*** Bug 528488 has been marked as a duplicate of this bug. ***
Comment 6 Peter Van der Beken [:peterv] 2009-11-15 12:39:26 PST
I'll also add the crashtest from bug 528488.
Comment 7 Peter Van der Beken [:peterv] 2009-11-15 12:48:47 PST
Created attachment 412490 [details] [diff] [review]
Comment 8 Paul Nickerson 2009-11-16 13:42:24 PST
Comment on attachment 412490 [details] [diff] [review]

I don't crash on this testcase or the one in bug 528488 with the patch applied.
Comment 9 Jonas Sicking (:sicking) 2009-11-16 13:49:10 PST
Comment on attachment 412490 [details] [diff] [review]

So why exactly are we crashing? Is the global-variable code ending up in a bad state or something?

While this patch seems like the right thing to do, I'm wondering if we could/should secure more code.

r=me anyhow
Comment 10 Peter Van der Beken [:peterv] 2009-11-17 11:15:40 PST
We bail out early because of an error when trying to get the variable in txExecutionState::getVariable. We don't pop the result handler from the stack. We have code to deal with that, not deleting the unknown handler if an error happens (since the result handler stack will delete it). But evaluateToNumber eats the error return and so we do delete the unknown handler, and then the stacks tries to do that again.
Comment 11 Jonas Sicking (:sicking) 2009-11-17 11:55:00 PST
Ah, and we can't pop these as we go up the callstack because it was push by a txInstruction and so we can't pop these guys off as we unwind.

Would be nice to get this more stable, but I can't think of a way to do so for now.
Comment 12 Peter Van der Beken [:peterv] 2009-11-19 07:51:50 PST
Need this crash fix on branch (not yet landed on trunk, need blocking or approval first).
Comment 13 Peter Van der Beken [:peterv] 2009-11-22 10:24:00 PST
Created attachment 413940 [details] [diff] [review]

Turns out the testcase for bug 528488 exposed a small leak in txExecutionState::getVariable. This is what I landed.
Comment 14 Peter Van der Beken [:peterv] 2009-11-22 10:25:07 PST

I'll nominate for other branches too.
Comment 15 Jonas Sicking (:sicking) 2009-11-22 11:55:31 PST
Comment on attachment 413940 [details] [diff] [review]

Would be nice if you could diff with -u8

Comment 16 Peter Van der Beken [:peterv] 2009-11-27 09:41:16 PST
Comment on attachment 413940 [details] [diff] [review]

XSLT crash fix.
Comment 17 Daniel Veditz [:dveditz] 2009-12-02 15:38:29 PST
Comment on attachment 413940 [details] [diff] [review]

Approved for and, a=dveditz for release-drivers
Comment 18 Peter Van der Beken [:peterv] 2009-12-03 06:37:16 PST
Comment 19 Al Billings [:abillings] 2010-01-15 17:02:42 PST
Verified fixed in 1.9.1 with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv: Gecko/20100115 Shiretoko/3.5.8pre (.NET CLR 3.5.30729). Attached testcase crashes in but just shows "Error during XSLT transformation: XSLT Stylesheet (possibly) contains a recursion." in the 1.9.1 current nightly.
Comment 20 Martin Stránský 2010-01-27 05:14:10 PST
Created attachment 423778 [details] [diff] [review]
1.8.0 version

Affects 1.8.0
Comment 21 Al Billings [:abillings] 2010-02-05 14:55:58 PST
Verified for (Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv: Gecko/2010020215 GranParadiso/3.0.18pre (.NET CLR 3.5.30729) with attached testcase. Results are the same as the verification with a crash in but an XSLT error in

Note You need to log in before you can comment on or make changes to this bug.