Beginning on October 25th, 2016, Persona will no longer be an option for authentication on BMO. For more details see Persona Deprecated.
Last Comment Bug 485286 - XSLT should heap allocate all evalContexts
: XSLT should heap allocate all evalContexts
: fixed-seamonkey1.1.16, verified1.8.1.22, verified1.9.0.8, verified1.9.0.9, verified1.9.1
Product: Core
Classification: Components
Component: XSLT (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla1.9.2a1
Assigned To: Blake Kaplan (:mrbkap)
: Andrew Overholt [:overholt]
Depends on: CVE-2009-1169
  Show dependency treegraph
Reported: 2009-03-25 19:23 PDT by Blake Kaplan (:mrbkap)
Modified: 2009-06-04 14:32 PDT (History)
17 users (show)
mbeltzner: blocking1.9.1+
samuel.sidler+old: blocking1.9.0.8+
samuel.sidler+old: blocking1.9.0.9+
dveditz: wanted1.9.0.x+
mrbkap: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Proposed fix (1.76 KB, patch)
2009-03-25 19:23 PDT, Blake Kaplan (:mrbkap)
jonas: review+
jonas: superreview+
Details | Diff | Splinter Review
testcase (360 bytes, application/xml)
2009-03-25 19:29 PDT, Jonas Sicking (:sicking) No longer reading bugmail consistently
no flags Details
Fix (2.04 KB, patch)
2009-03-26 00:11 PDT, Blake Kaplan (:mrbkap)
peterv: review+
mrbkap: superreview+
samuel.sidler+old: approval1.9.0.8+
samuel.sidler+old: approval1.9.0.9+
Details | Diff | Splinter Review
For the 1.8 branch (1.86 KB, patch)
2009-03-26 01:01 PDT, Blake Kaplan (:mrbkap)
peterv: review+
Details | Diff | Splinter Review
Crashtest (899 bytes, patch)
2009-03-30 11:47 PDT, Blake Kaplan (:mrbkap)
no flags Details | Diff | Splinter Review

Description Blake Kaplan (:mrbkap) 2009-03-25 19:23:26 PDT
Created attachment 369413 [details] [diff] [review]
Proposed fix

bug 485217 fixed one case where we fail to pop a stack-allocated evalContext in an error condition. However, with the patch in that bug, it's possible to re-enter the interpreter and push a heap-allocated eval context (via a txPushNewContext instruction). If, after we handle such an instruction, we hit an error, then we'll leave that eval context on the stack and, when we retreat back into the code fixed in bug 485217, we'll pop the *wrong* context, leaving the same error as before.

The patch I'm attaching basically backs out the patch in bug 485217 per sicking's request to make the error handling consistent (you never pop someone else's context, but all contexts are heap allocated, so we can safely clean them up at the end).
Comment 1 Jonas Sicking (:sicking) No longer reading bugmail consistently 2009-03-25 19:29:05 PDT
Created attachment 369416 [details]
Comment 2 Jonas Sicking (:sicking) No longer reading bugmail consistently 2009-03-25 19:49:45 PDT
Forgot to make one comment. We should check for out-of-memory when allocating the context. Whoever checks this in it'd be great to fix that, but it's not really very critical.

I also look through the code and couldn't find any other cases where this pattern occurs.
Comment 3 Blake Kaplan (:mrbkap) 2009-03-26 00:11:05 PDT
Created attachment 369449 [details] [diff] [review]

This is ready to be checked in.
Comment 4 Peter Van der Beken [:peterv] 2009-03-26 00:12:22 PDT
Comment on attachment 369449 [details] [diff] [review]

Ok, that makes sense. I've also looked for
other cases and couldn't see anything wrong.
Comment 5 Samuel Sidler (old account; do not CC) 2009-03-26 00:17:28 PDT
Comment on attachment 369449 [details] [diff] [review]

Approved for and a=ss

Please land on the 1.9.0 branch ASAP. We also need to get this on the relbranch for
Comment 6 Samuel Sidler (old account; do not CC) 2009-03-26 00:23:13 PDT
Blocking all around. I can't set 1.9.1 blocking, but it blocks that too. Blake will make sure this applies to 1.8 (or attach a 1.8 patch) tomorrow.

1.8.1/1.8.0 Linux distro drivers: We're taking this patch with our firedrill. It's a more complete fix for bug 485217 and we don't want to re-0-day ourselves.
Comment 7 Blake Kaplan (:mrbkap) 2009-03-26 00:40:24 PDT and


content/xslt/src/xslt/txKeyFunctionCall.cpp: 1.42 -> 1.43 (1.9.0) and -> (relbranch)
Comment 8 Blake Kaplan (:mrbkap) 2009-03-26 01:01:27 PDT
Created attachment 369457 [details] [diff] [review]
For the 1.8 branch

Peter, mind sanity checking the backport? Thanks. Hopefully, this will be the last we hear of this bug.
Comment 9 Blake Kaplan (:mrbkap) 2009-03-26 02:18:01 PDT
Note: sicking's testcase relies on bug 353364, so will need tweaking to test the 1.9.0 and 1.8 branches.
Comment 10 Daniel Veditz [:dveditz] 2009-03-26 02:54:38 PDT
fwiw I tested that Blake's initial patch fixed the testcase in bug 485217 and did not fix the testcase here, and that this patch fixes both testcases. Tested both 1.9.1 and 1.9.0 builds (the tweaking in comment 9 is replacing href="" with an explicit full URI to wherever you've put that file).

For now Sam has put an appropriately tweaked copy at
Comment 11 Alexander Sack 2009-03-26 06:02:14 PDT
i can confirm that the 1.8 branch patch makes the testcase not-crash for me on 1.8.1. Thanks!
Comment 12 juan becerra [:juanb] 2009-03-26 19:09:51 PDT
Verified fixed on the following builds using the testcase in comment #10:
Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv: Gecko/2009032609 Firefox/3.0.8 (.NET CLR 3.5.30729)
Mozilla/5.0 (X11; U; Linux i686; en-US; rv: Gecko/2009032608 Firefox/3.0.8
Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv: Gecko/2009032608 Firefox/3.0.8

Fx307 on Mac, Linux and Windows crashed. Fx308 does not crash on this testcase.
Comment 13 Samuel Sidler (old account; do not CC) 2009-03-26 19:40:26 PDT
Comment on attachment 369457 [details] [diff] [review]
For the 1.8 branch

Approved for a=ss for release-drivers.
Comment 14 Blake Kaplan (:mrbkap) 2009-03-26 19:43:48 PDT
/cvsroot/mozilla/extensions/transformiix/source/xslt/functions/Attic/txKeyFunctionCall.cpp,v  <--  txKeyFunctionCall.cpp
new revision:; previous revision:
Comment 15 Alexander Sack 2009-03-27 04:51:18 PDT
Comment on attachment 369457 [details] [diff] [review]
For the 1.8 branch

a=asac for 1.8.0

verified that this fixes the testcase on 1.8.0 too
Comment 16 Henrik Skupin (:whimboo) 2009-03-30 02:57:43 PDT
Verified on trunk and 1.9.1 with builds on OS X and Windows:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090329 Minefield/3.6a1pre ID:20090329031037

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b4pre) Gecko/20090329 Shiretoko/3.5b4pre ID:20090329030931

Would be nice to have this testcase as an automated test.
Comment 17 Samuel Sidler (old account; do not CC) 2009-03-30 07:29:15 PDT
(In reply to comment #16)
> Would be nice to have this testcase as an automated test.

Blake, can you get an automated test for this landed in the various places?
Comment 18 Blake Kaplan (:mrbkap) 2009-03-30 11:47:56 PDT
Created attachment 370042 [details] [diff] [review]
Comment 20 Al Billings [:abillings] 2009-04-02 14:55:10 PDT
Verified for (Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv: Gecko/2009040204 GranParadiso/3.0.9pre) with testcase in comment 10. Blake's crashtest also passes on tinderbox.
Comment 21 Al Billings [:abillings] 2009-06-04 14:32:58 PDT
Verified for using testcase in comment #10.

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