305021 - crash in [@ nsSVGGradientFrame::checkURITarget] if gradient references itself

Status: VERIFIED FIXED

(Core :: SVG, defect)

Description

[arno renevier]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8b3) Gecko/20050712 Firefox/1.0+
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8b3) Gecko/20050712 Firefox/1.0+

crash if we try to use linearGradient (or radialGradient with xlink:href self
pointing. ex :
<linearGradient id="azerty" xlink:href="#azerty"/>

Reproducible: Always

Steps to Reproduce:
1. see testcase
2.
3.

Comment 1

[Stephen Donner [:stephend] Not actively reading bugmail]

Confirming: I see this with build 2005-08-16-12.  Stack as soon as I can get it.

Comment 2

[Rob Russell]

Attachment: Another example - not self-referential

Comment 3

[Rob Russell]

Funny, I just isolated the same bug 1 1/2 hours ago. I've seen it on Windows
2000 and XP with Deer Park Alpha 2. 

It happens with a radialGradient that has an attribute like
xlink:href="url(#g2)" - it doesn't have to reference itself. If the reference is
of the form xlink:href="#g2" it doesn't crash, but I don't think it does
anything useful either (I haven't seen examples of which form I should use for
an href).

Comment 4

[Scooter Morris]

Attachment: Fix self-referencing crash

OK, this fixes the simple case of a self-referencing gradient.	I suspect that
there is a more serious problem with reference loops, but I haven't
investigated that, yet.

Comment 5

[Scooter Morris]

(In reply to comment #3)
> Funny, I just isolated the same bug 1 1/2 hours ago. I've seen it on Windows
> 2000 and XP with Deer Park Alpha 2. 
> 
> It happens with a radialGradient that has an attribute like
> xlink:href="url(#g2)" - it doesn't have to reference itself. If the reference is
> of the form xlink:href="#g2" it doesn't crash, but I don't think it does
> anything useful either (I haven't seen examples of which form I should use for
> an href).

An href must be of the form "#id" to work -- the syntax "url(#id)" is only
supported in properties and attributes.

Your example would also not do anything interesting because you have not defined
any stops.

Comment 6

[Stephen Donner [:stephend] Not actively reading bugmail]

Scooter, can you request reviews?

Comment 7

[Scooter Morris]

(In reply to comment #6)
> Scooter, can you request reviews?

I would rather wait until I have a more complete solution -- i.e. to the
reference loop as opposed to the more simple self-reference problem.

Comment 8

[Scooter Morris]

Attachment: More complete patch

This patch not only protects against self-referencing gradients but also checks
for reference loops.

Comment 9

[tor]

Comment on attachment 194806 [details] [diff] [review]
More complete patch

> NS_IMETHODIMP
> nsSVGGradientFrame::DidModifySVGObservable(nsISVGValue* observable, 
>                                            nsISVGValue::modificationType aModType)
> {
>+  nsCOMPtr<nsISVGGradient> gradient = do_QueryInterface(observable);
>+  if (mNextGrad && aModType == nsISVGValue::mod_die && gradient) {
>+    // Yes, we need to handle this differently
>+    nsISVGGradient *grad;
>+    CallQueryInterface(mNextGrad, &grad);
>+    if (grad == gradient) {
>+      mNextGrad = nsnull;
>+    }
>+  }

nit - The "Yes..." comment seems to imply a question.  Add a comment?

Should we be removing the observer in the grad==gradient case?

> NS_IMETHODIMP
> nsSVGGradientFrame::GetStopOffset(PRInt32 aIndex, float *aOffset)
> {
>+  if (stopCount && !stopElement) {
>     *aOffset = nsnull;
>     return NS_ERROR_FAILURE;
>   }

Should be "*aOffset = 0.0f".

> PRBool 
> nsSVGGradientFrame::checkURITarget(nsIAtom *attr) {
>   // Get the Gradient
>   nsCAutoString aGradStr;
>   CopyUTF16toUTF8(mNextGradStr, aGradStr);
>   // Note that we are using *our* frame tree for this call, otherwise we're going to have
>   // to get the PresShell in each call
>   if (nsSVGUtils::GetReferencedFrame(&aNextGrad, aGradStr, mContent, GetPresContext()->PresShell()) == NS_OK) {
>+    // Are we self-referencing?
>+    if (aNextGrad == this) {
>+      // Yes, remove the reference and return an error
>+      NS_WARNING("Gradient references itself!");
>+      CopyUTF8toUTF16("", mNextGradStr);
>+      return PR_FALSE;
>+    }

Can we set the loop detection flag earlier and remove this extra test?

Comment 10

[Scooter Morris]

Attachment: Address tor's comments

Comment 11

[Scooter Morris]

(In reply to comment #10)
> Created an attachment (id=195043) [edit]
> Address tor's comments
> 

One comment I didn't address is the question about removing ourselves from the
gradient's observer list.  We don't do that in any of the cases because since
mod_die is set, we know that the frame is in the process of getting destroyed,
and we don't know exactly where we are in the process.  As part of the the
destruction, the observer list will go away anyways.

Comment 12

[tor]

Comment on attachment 195043 [details] [diff] [review]
Address tor's comments

> PRBool
> nsSVGGradientFrame::checkURITarget(void) {
>   nsIFrame *aNextGrad;
>+  mLoopFlag = PR_TRUE; // Set our loop detection flag

>     mNextGrad = (nsSVGGradientFrame *)aNextGrad;
>+    if (mNextGrad->mLoopFlag) {
>+      // Yes, remove the reference and return an error
>+      NS_WARNING("Gradient loop detected!");
>+      CopyUTF8toUTF16("", mNextGradStr);
>+      mNextGrad = nsnull;
>+      return PR_FALSE;
>+    }

Do we need to zero the loop flag in this early return?	With that, r=tor.

Comment 13

[Scooter Morris]

Attachment: Make sure all returns clear loop flag

Same as before, but make sure all possible returns from checkURI reset the loop
flag.

Comment 14

[Scooter Morris]

Checked into trunk

Comment 15

[Stephen Donner [:stephend] Not actively reading bugmail]

(In reply to comment #14)
> Checked into trunk

If (since?) there's no more work here, shouldn't this be marked FIXED?  Only
asking because I run SVG-enabled builds and would be happy to mark it VERIFIED
once it's resolved...

Comment 16

[Stephen Donner [:stephend] Not actively reading bugmail]

I opened bug 307742 for a crash loading the same SVG file
http://ffsearchplugins.free.fr/bugzilla/SvgGradientCrash.svg in this bug, but
with a different stack trace.

Comment 17

[Scooter Morris]

Checked into branch:
Checking in layout/svg/base/src/nsSVGGradientFrame.cpp;
/cvsroot/mozilla/layout/svg/base/src/nsSVGGradientFrame.cpp,v  <-- 
nsSVGGradientFrame.cpp
new revision: 1.14.2.1; previous revision: 1.14
done

Comment 18

[Scooter Morris]

*** Bug 307742 has been marked as a duplicate of this bug. ***

Comment 19

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED using SeaMonkey 1.1a

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20050917 Mozilla/1.0

Comment 20

[Jay Patel [:jay]]

v.fixed on branch with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b5)
Gecko/20050928 Firefox/1.4 and Talkback data (only 1 crash with signature from
9/8 build)