460090 - Firefox crashes (segfault) on attempting to view XSL Transform (xml file with linked xsl) [ txMozillaXSLTProcessor::TransformToDoc ]

Status: RESOLVED DUPLICATE of bug 485217

(Core :: XSLT, defect, P1)

Description

[Mike Rooney]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.0.3) Gecko/2008101315 Ubuntu/8.10 (intrepid) Firefox/3.0.3
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.0.3) Gecko/2008101315 Ubuntu/8.10 (intrepid) Firefox/3.0.3

Opened from downstream https://bugs.launchpad.net/ubuntu/+source/firefox-3.0/+bug/253641.

Attempting to view http://launchpadlibrarian.net/16454122/test.xml causes a segfault, confirmed on multiple systems and multiple versions of Firefox. If the XSL is not linked, there is no problem, so it appears to be caused by the XSL.

Reproducible: Always

Steps to Reproduce:
1.Visit http://launchpadlibrarian.net/16454122/test.xml
Actual Results:  
Segfault.

Expected Results:  
No segfault. The XML should be displayed, applying the XSLT, or at least giving an error that it cannot be transformed.

Comment 1

[Mardeg]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b2pre) Gecko/20081015 Minefield/3.1b2pre
Confirmed the crash. Please change the product to "Core", component to "XSLT"
Report should appear at http://crash-stats.mozilla.com/report/index/d62fe3c7-9aec-11dd-a839-0013211cbf8a

Comment 2

[Thomas K. (:tom)]

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b2pre) Gecko/20081014 Firefox/3.0.3pre

http://crash-stats.mozilla.com/report/index/95c38b5e-9af0-11dd-94b2-001cc45a2ce4

Comment 3

[Dave Garrett]

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.0.3) Gecko/2008092416 Firefox/3.0.3
bp-fa156a2e-9af6-11dd-97b8-001a4bd43e5c

Comment 4

[Ria Klaassen (not reading all bugmail)]

Tested with Windows XP, I see this crash also in older versions like Firefox 1.

Comment 5

[Mark Omohundro]

It could be a problem with this in the xsl file:
 <xsl:for-each select="key('kat', @id)">
or with the related namespace possibly.

Try changing or removing this and see if it runs on FF 3.0.3.
Hope this helps.

Comment 6

[Mark Omohundro]

a minor item(?) worth mentioning is UTF-8 encoding doesn't display the extended characters correctly (in my tests anyway). but ISO-8859-1 does as coded below....  why, does anybody know?  but not only in firefox -- other browsers do this as well -- so maybe some/many of us have the wrong idea of what chars UTF-8 supports??  or is there some other reason.  anybody?  just fyi, for what it is worth. hope it helps someone.

sample xml file that displays chars properly:

<?xml version="1.0" encoding="ISO-8859-1"?>
<sample>
    <data>Bücher</data>
    <data>Verfügbar</data>
</sample>

Comment 7

[Martin]

Attachment: Reduced testcase

Looks to be a forwards-compatible processing issue, specifically with a key that uses a function with an unprefixed name that is not in the language.

Comment 8

[Martin]

Attachment: Fix and a couple of crash tests

Problem is evalContext is stack-allocated but in the failure case remains referenced in the txExecutionState object, whose destructor later tries to delete it.

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 350567 [details] [diff] [review]
Fix and a couple of crash tests

Could you write the test by doing

use="count(2)"

instead. That is sure to crash during evaluation rather than earlier.

And thanks for the patch!

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

If you attach a patch with that fixed i'll mark it approved for 1.9.1. Preferably a mq patch for easy landing.

Comment 11

[Martin]

Attachment: .hg/patches/bug460090

> Could you write the test by doing
>
> use="count(2)"
>
> instead. That is sure to crash during evaluation rather than earlier.

Done, though I don't quite understand why, or how Bug 143291 might affect things.

> If you attach a patch with that fixed i'll mark it approved for 1.9.1.
> Preferably a mq patch for easy landing.

Please feel free to add +a191 to the commit message and push to mozilla-central for me.

Comment 12

[Martin]

...or assign the bug to me and I'll bother someone else to commit it.

Comment 13

[Martin]

Comment on attachment 352626 [details] [diff] [review]
.hg/patches/bug460090

This bug seems to have fallen through the cracks, not sure what bugzilla incantation is the right one to get it noticed again, so asking for review of changed patch.

Comment 14

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 352626 [details] [diff] [review]
.hg/patches/bug460090

This patch seems to be missing one of the tests from the previous patch. Was that intentional?

Comment 15

[Martin]

> Could you write the test by doing
>
> use="count(2)"
>
> instead. That is sure to crash during evaluation rather than earlier.

I took the singlar 'test' and the lack of mention of the extention function case to mean that you only wanted the forwards-compatible processing version.

By my reading of the spec, the original tests were valid, but I was trying to address your review without understanding what that comment meant.

Please edit whichever patch however you want if you feel the need.

Comment 16

[Boris Zbarsky [:bzbarsky]]

So now that bug 485217 is fixed, is this fixed too?

Comment 17

[Daniel Veditz [:dveditz]]

ugh, too bad we missed this. Duping forward because we checked in using the other bug.

Comment 18

[Martin]

There's not much point giving r+ and assigning to me *after* a seperate bug has been filed and fixed for an exploit using this.

Given this result, the time quibbling over tests that will never now be in tree seems a little pointless.

Comment 19

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Indeed. I'm sorry i forgot about this bug until it was too late. The other discussions about a crash in XSLT made me remember this bug. Wasn't until afterwords that I realized it was the same bug even.

For what it's worth, it wasn't quibbling about the missing testcase. I intended to add that back myself.