Last Comment Bug 485286 - XSLT should heap allocate all evalContexts
: XSLT should heap allocate all evalContexts
Status: VERIFIED FIXED
[sg:critical]
: 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) (please use needinfo!)
:
Mentors:
Depends on: CVE-2009-1169
Blocks:
  Show dependency treegraph
 
Reported: 2009-03-25 19:23 PDT by Blake Kaplan (:mrbkap) (please use needinfo!)
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+
dveditz: blocking1.8.1.next+
caillon: blocking1.8.0.next+
mrbkap: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


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

Description Blake Kaplan (:mrbkap) (please use needinfo!) 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) 2009-03-25 19:29:05 PDT
Created attachment 369416 [details]
testcase
Comment 2 Jonas Sicking (:sicking) 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) (please use needinfo!) 2009-03-26 00:11:05 PDT
Created attachment 369449 [details] [diff] [review]
Fix

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]
Fix

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]
Fix

Approved for 1.9.0.8 and 1.9.0.9. a=ss

Please land on the 1.9.0 branch ASAP. We also need to get this on the relbranch for 1.9.0.8.
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) (please use needinfo!) 2009-03-26 00:40:24 PDT
http://hg.mozilla.org/mozilla-central/rev/85bd18f6b652 and http://hg.mozilla.org/releases/mozilla-1.9.1/rev/4552d789ad8f

Also:

content/xslt/src/xslt/txKeyFunctionCall.cpp: 1.42 -> 1.43 (1.9.0) and 1.41.30.1 -> 1.41.30.2 (relbranch)
Comment 8 Blake Kaplan (:mrbkap) (please use needinfo!) 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) (please use needinfo!) 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
http://people.mozilla.com/~ss/485286-shh/testcase.xml
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:1.9.0.8) Gecko/2009032609 Firefox/3.0.8 (.NET CLR 3.5.30729)
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.0.8) Gecko/2009032608 Firefox/3.0.8
Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.8) 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 1.8.1.22. a=ss for release-drivers.
Comment 14 Blake Kaplan (:mrbkap) (please use needinfo!) 2009-03-26 19:43:48 PDT
/cvsroot/mozilla/extensions/transformiix/source/xslt/functions/Attic/txKeyFunctionCall.cpp,v  <--  txKeyFunctionCall.cpp
new revision: 1.33.28.2; previous revision: 1.33.28.1
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) (please use needinfo!) 2009-03-30 11:47:56 PDT
Created attachment 370042 [details] [diff] [review]
Crashtest
Comment 19 Blake Kaplan (:mrbkap) (please use needinfo!) 2009-03-30 12:03:48 PDT
http://hg.mozilla.org/mozilla-central/rev/f02ddd383a4b and http://hg.mozilla.org/releases/mozilla-1.9.1/rev/cb01d655a1b1 and the 1.9.0 branch.
Comment 20 Al Billings [:abillings] 2009-04-02 14:55:10 PDT
Verified for 1.9.0.9 (Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.9pre) 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 1.8.1.22 using testcase in comment #10.

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