XSLT should heap allocate all evalContexts

VERIFIED FIXED in mozilla1.9.2a1

Status

()

Core
XSLT
VERIFIED FIXED
9 years ago
8 years ago

People

(Reporter: mrbkap, Assigned: mrbkap)

Tracking

(5 keywords)

Trunk
mozilla1.9.2a1
fixed-seamonkey1.1.16, verified1.8.1.22, verified1.9.0.8, verified1.9.0.9, verified1.9.1
Points:
---
Bug Flags:
blocking1.9.1 +
blocking1.9.0.8 +
blocking1.9.0.9 +
wanted1.9.0.x +
blocking1.8.1.next +
blocking1.8.0.next +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical])

Attachments

(4 attachments, 1 obsolete attachment)

360 bytes, application/xml
Details
Fix
2.04 KB, patch
peterv
: review+
mrbkap
: superreview+
Samuel Sidler (old account; do not CC)
: approval1.9.0.8+
Samuel Sidler (old account; do not CC)
: approval1.9.0.9+
Details | Diff | Splinter Review
1.86 KB, patch
peterv
: review+
Samuel Sidler (old account; do not CC)
: approval1.8.1.next+
Alexander Sack
: approval1.8.0.next+
Details | Diff | Splinter Review
899 bytes, patch
Details | Diff | Splinter Review
(Assignee)

Description

9 years ago
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).
Attachment #369413 - Flags: superreview?(jonas)
Attachment #369413 - Flags: review?(peterv)
Attachment #369413 - Flags: review?(jonas)
Flags: wanted1.9.0.x?
Flags: blocking1.9.1?
Flags: blocking1.9.0.9?
Flags: blocking1.8.1.next?
Flags: blocking1.8.0.next?
OS: Mac OS X → All
Hardware: x86 → All
Whiteboard: [sg:critical prevention]
Created attachment 369416 [details]
testcase
Attachment #369413 - Flags: superreview?(jonas)
Attachment #369413 - Flags: superreview+
Attachment #369413 - Flags: review?(jonas)
Attachment #369413 - Flags: review+
Flags: blocking1.9.0.8?
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.
(Assignee)

Comment 3

9 years ago
Created attachment 369449 [details] [diff] [review]
Fix

This is ready to be checked in.
Attachment #369413 - Attachment is obsolete: true
Attachment #369449 - Flags: superreview+
Attachment #369449 - Flags: review?(peterv)
Attachment #369413 - Flags: review?(peterv)
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.
Attachment #369449 - Flags: review?(peterv) → review+
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.
Attachment #369449 - Flags: approval1.9.0.9+
Attachment #369449 - Flags: approval1.9.0.8+
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.
Flags: wanted1.9.0.x?
Flags: blocking1.9.0.9?
Flags: blocking1.9.0.9+
Flags: blocking1.9.0.8?
Flags: blocking1.9.0.8+
Whiteboard: [sg:critical prevention] → [sg:critical]
(Assignee)

Comment 7

9 years ago
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)
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Keywords: fixed1.9.0.8, fixed1.9.0.9, fixed1.9.1
Resolution: --- → FIXED
(Assignee)

Comment 8

9 years ago
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.
Attachment #369457 - Flags: review?(peterv)
Attachment #369457 - Flags: approval1.8.1.next?
Attachment #369457 - Flags: approval1.8.0.next?
Target Milestone: --- → mozilla1.9.2a1
Flags: wanted1.9.0.x+
Flags: blocking1.8.1.next?
Flags: blocking1.8.1.next+
Target Milestone: mozilla1.9.2a1 → ---
Target Milestone: --- → mozilla1.9.2a1
Flags: blocking1.8.0.next? → blocking1.8.0.next+
Attachment #369457 - Flags: review?(peterv) → review+
(Assignee)

Comment 9

9 years ago
Note: sicking's testcase relies on bug 353364, so will need tweaking to test the 1.9.0 and 1.8 branches.
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

9 years ago
i can confirm that the 1.8 branch patch makes the testcase not-crash for me on 1.8.1. Thanks!
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.
Keywords: fixed1.9.0.8 → verified1.9.0.8
Comment on attachment 369457 [details] [diff] [review]
For the 1.8 branch

Approved for 1.8.1.22. a=ss for release-drivers.
Attachment #369457 - Flags: approval1.8.1.next? → approval1.8.1.next+
(Assignee)

Comment 14

9 years ago
/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
Keywords: fixed1.8.1.22

Comment 15

9 years ago
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
Attachment #369457 - Flags: approval1.8.0.next? → approval1.8.0.next+
Flags: blocking1.9.1? → blocking1.9.1+
Group: core-security
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.
Status: RESOLVED → VERIFIED
Flags: in-testsuite?
Keywords: fixed1.9.1 → verified1.9.1
(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?
(Assignee)

Comment 18

9 years ago
Created attachment 370042 [details] [diff] [review]
Crashtest
(Assignee)

Comment 19

9 years ago
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.
Flags: in-testsuite? → in-testsuite+
Depends on: 485217
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.
Keywords: fixed1.9.0.9 → verified1.9.0.9

Updated

9 years ago
Keywords: fixed-seamonkey1.1.16
Verified for 1.8.1.22 using testcase in comment #10.
Keywords: fixed1.8.1.22 → verified1.8.1.22
You need to log in before you can comment on or make changes to this bug.