Closed Bug 637214 Opened 9 years ago Closed 9 years ago

RECURSION_LEVEL assertion with SVG textPath

Categories

(Core :: SVG, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla2.0
Tracking Status
blocking2.0 --- final+
status1.9.2 --- wontfix
status1.9.1 --- wontfix

People

(Reporter: jruderman, Assigned: sicking)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, testcase, Whiteboard: [sg:critical?][hardblocker][ETA: 3/2])

Attachments

(8 files, 5 obsolete files)

587 bytes, image/svg+xml
Details
6.39 KB, text/plain
Details
2.78 KB, patch
sicking
: review+
Details | Diff | Splinter Review
2.12 KB, patch
Details | Diff | Splinter Review
3.53 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
1.35 KB, patch
Details | Diff | Splinter Review
6.29 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
1.70 KB, patch
Details | Diff | Splinter Review
Attached image testcase
About 60% of the time, this testcase triggers:

###!!! ASSERTION: op == PL_DHASH_LOOKUP || RECURSION_LEVEL(table) == 0: 'op == PL_DHASH_LOOKUP || RECURSION_LEVEL(table) == 0', file pldhash.c, line 612

Sometimes it also triggers:

###!!! ASSERTION: Failed to track content!: 'aOldElement == p->mElement', file content/base/src/nsReferencedElement.cpp, line 257

The testcase is based on layout/reftests/svg/dynamic-textPath-01.svg.
Attached file stack traces
The "also triggers" is bug 612736.
Oh, it is? It's a different assertion.
The relevant documentation:

330    * This gets fired when the element that an id refers to changes.
331    * This fires at difficult times. It is generally not safe to do anything
332    * which could modify the DOM in any way. Use
333    * nsContentUtils::AddScriptRunner.
334    * @return PR_TRUE to keep the callback in the callback set, PR_FALSE
335    * to remove it.
336    */
337   typedef PRBool (* IDTargetObserver)(Element* aOldElement,
338                                       Element* aNewelement, void* aData);

nsSVGIDRenderingObserver::SourceReference::ElementChanged is totally not doing what that comment says to do.
Oh, and this is probably exploitable, since you can modify the hashtable as it's being enumerated over, and then all bets are off.

I assume this is a problem for 1.9.2; not sure about 1.9.1.
Group: core-security
blocking1.9.2: --- → ?
blocking2.0: --- → ?
On the other hand, nsReferencedElement::Observe _does_ use a script runner, but we're apparently not under a scriptblocker here?  I wonder why not...
blocking2.0: ? → final+
Whiteboard: [sg:critical][hardblocker]
dholbert, can you own this, or suggest another owner if you're not the right guy here?
Assignee: nobody → dholbert
Yeah, I can.  I'm at a SVG WG F2F right now, but I'll try to debug this in more detail later today.
Attached patch Patch (v1) (obsolete) — Splinter Review
Assignee: dholbert → ehsan
Status: NEW → ASSIGNED
Attachment #515775 - Flags: review?(bzbarsky)
Whiteboard: [sg:critical][hardblocker] → [sg:critical][hardblocker][has patch][needs review bz]
Comment on attachment 515775 [details] [diff] [review]
Patch (v1)

Please put the blocker in RemoveFromIdTable after the early returns.

You don't need the blocker in LookupImageElement.  In fact, that should be using GetEntry, not PutEntry!

r=me with those changes, especially if you add some comments before the blockers about making sure that id listeners are not notified synchronously while adding/removing the entry.
Attachment #515775 - Flags: review?(bzbarsky) → review+
(In reply to comment #10)
> You don't need the blocker in LookupImageElement.  In fact, that should be
> using GetEntry, not PutEntry!

Should I make this change too?  If yes, I'd rather file a new bug for that, since that can't be considered as part of this bug...
Attached patch For check-inSplinter Review
Attachment #515775 - Attachment is obsolete: true
Attached patch TestsSplinter Review
Keywords: checkin-needed
Whiteboard: [sg:critical][hardblocker][has patch][needs review bz] → [sg:critical][hardblocker][has patch][needs landing]
I'm assuming this has explicit approval though I'm not sure what the tree rules are currently. An a+ would be nice to have.

Also, is the tests patch part of this to checkin?
> Should I make this change too?  If yes, I'd rather file a new bug for that

Please, and yes.
We should figure out the 1.9.2 story here before landing the tests.
Comment on attachment 515863 [details] [diff] [review]
For check-in

The script blocker in remove should come after the "no entry" early return, no?

And again, you don't need the script blocker in LookupImageElement...
(In reply to comment #14)
> I'm assuming this has explicit approval though I'm not sure what the tree rules
> are currently. An a+ would be nice to have.

It is a hardblocker so it doesn't need explicit approval.  I'll take care of landing this later today.

> Also, is the tests patch part of this to checkin?

Not until it's been fixed and we've shipped a release on 1.9.2.
Keywords: checkin-needed
(In reply to comment #17)
> Comment on attachment 515863 [details] [diff] [review]
> For check-in
> 
> The script blocker in remove should come after the "no entry" early return, no?

Yep, will move it further down.

> And again, you don't need the script blocker in LookupImageElement...

Hmm, actually I was thinking about leaving that in until we switch to use GetEntry there, and take it out in the same patch.  Does that sound good?
Whiteboard: [sg:critical][hardblocker][has patch][needs landing] → [sg:critical][hardblocker][has patch][needs landing][ETA:3/1]
OK, that sounds fine.
Comment on attachment 515863 [details] [diff] [review]
For check-in

Just in casee there's any doubt...
Attachment #515863 - Flags: approval2.0+
http://hg.mozilla.org/mozilla-central/rev/7ed9b3bc059e
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Whiteboard: [sg:critical][hardblocker][has patch][needs landing][ETA:3/1] → [sg:critical][hardblocker]
Target Milestone: --- → mozilla2.0
I don't understand why the scriptblocker was added to AddToIdTable/RemoveFromIdTable. Surely it should be added to whatever's calling those things? I mean we don't want scripts to run during the call to AddToIdTable from nsStyledElement::ParseAttribute, do we?

I'm also not sure why this is a hardblocker. This is a non-public fuzz-test, why hold the release for it?
(In reply to comment #15)
> > Should I make this change too?  If yes, I'd rather file a new bug for that
> 
> Please, and yes.

Filed bug 637807 with a patch!
(In reply to comment #23)
> I don't understand why the scriptblocker was added to
> AddToIdTable/RemoveFromIdTable. Surely it should be added to whatever's calling
> those things? I mean we don't want scripts to run during the call to
> AddToIdTable from nsStyledElement::ParseAttribute, do we?

That's what this patch does, isn't it?  We basically should block the ID hashtable listeners during those two calls.

> I'm also not sure why this is a hardblocker. This is a non-public fuzz-test,
> why hold the release for it?

I wasn't involved in the decision, so I don't know why we marked this as hardblocker.
I think roc's point is that the script blocker going out of scope can run scripts.  Bringing us back to my question about why we're not already in a scriptblocker here.

Looks like we don't set it up until SetAttrAndNotify, which is after ParseAttribute.
Yeah, the scriptblockers in AddToIdTable and RemoveFromIdTable should be replaced with assertions that we're already in a scriptblocker, and then the callers that trigger those assertions should be fixed.
Comment on attachment 515863 [details] [diff] [review]
For check-in

Agreed, this doesn't look correct to me.
Attachment #515863 - Flags: review-
Backed out: http://hg.mozilla.org/mozilla-central/rev/7b4075059940
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: [sg:critical][hardblocker] → [sg:critical][hardblocker][needs new patch]
Whats the plan here? This is the last bug keeping us from shipping an RC, basically.
Ehsan, are you going to continue working on this or do we need to pull in
someone else?
(In reply to comment #31)
> Ehsan, are you going to continue working on this or do we need to pull in
> someone else?

Yes, I'm waiting for a build to finish right now.  I will start working on this as soon as it's done.
By the way, I think the final patch here would need a try server run, so I'm not sure if it can land before tomorrow.
Whiteboard: [sg:critical][hardblocker][needs new patch] → [sg:critical][hardblocker][needs new patch][ETA: 3/2]
Personally I don't think this should be a hardblocker.
Even assuming this is exploitable, I don't think all known exploitable bugs should be hardblockers. While it would be nice to ship with zero known exploitable bugs, that's really a cosmetic statistic since we're sure to find a new exploitable bug pretty soon after we ship.
All reproducible sg:crits are hardblockers.
shaver just pointed out that we have to probably do a fast respin after 4.0 anyway due to pwn2own coming up. While we usually block on all reproducible sg-crits, I think we should consider unblocking on this one and spinning the RC build right now.
Attached patch Alternative patch (obsolete) — Splinter Review
Attachment #516081 - Flags: review?(bzbarsky)
Attached patch Assertions that roc suggested (obsolete) — Splinter Review
Attachment #516084 - Flags: review?(jonas)
(In reply to comment #36)
> All reproducible sg:crits are hardblockers.

OK, I think that's a bad policy.
Sicking is doing the heavy lifting here, just making it official.
Assignee: ehsan → jonas
Basically I think an sg:critical bug should only be a hardblocker if we would do a firedrill security update if we found that bug on a branch. Otherwise we're saying that sg:critical bugs suddenly become less important after they've been shipped in a release, which doesn't sound right. But let's proceed with the current policy for now...
(In reply to comment #42)
> Basically I think an sg:critical bug should only be a hardblocker if we would
> do a firedrill security update if we found that bug on a branch. Otherwise
> we're saying that sg:critical bugs suddenly become less important after they've
> been shipped in a release, which doesn't sound right. But let's proceed with
> the current policy for now...

This might be worth bringing up on dev.platform/planning...
Comment on attachment 515863 [details] [diff] [review]
For check-in

Unfortunately I think we'll have to go with this solution for now.
Attachment #515863 - Flags: review- → review+
Attachment #516081 - Attachment is obsolete: true
Attachment #516081 - Flags: review?(bzbarsky)
Attachment #516107 - Flags: review?(bzbarsky)
Here's the approach I took:

I audited all callers of AddToIdTable/RemoveFromIdTable. Most of them already have scriptblockers, the two exceptions were:

1) XUL-code
But we disabled XUL for the web!

2) nsXMLElement::NodeInfoChange
This function is called in two places
 2a) nsNodeUtils::CloneAndAdopt
     This code is called in all sorts of places. However since this doesn't
     insert the cloned nodes into a document, we're fine!
 2b) nsGenericElement::SetPrefix
     Fixed by adding a scriptblocker
Comment on attachment 516107 [details] [diff] [review]
Alternative patch v2

Fix the comment in MozSetImageElement to reflect reality and maybe switch to a scriptblocker in SetPrefix, and r=me
Attachment #516107 - Flags: review?(bzbarsky) → review+
Oh, and we should fix the XUL thing too, it's just lower priority.
Attachment #516084 - Attachment is obsolete: true
Attachment #516084 - Flags: review?(jonas)
Whiteboard: [sg:critical][hardblocker][needs new patch][ETA: 3/2] → [sg:critical][hardblocker][has patch][ETA: 3/2]
I am watching the patch on try. I can land it tonight if it goes green.
Whiteboard: [sg:critical][hardblocker][has patch][ETA: 3/2] → [sg:critical][hardblocker][has patch][ETA: 3/1]
Checked in. Thanks for all the help folks!

http://hg.mozilla.org/mozilla-central/rev/af9c212658df
http://hg.mozilla.org/mozilla-central/rev/65db1504a456
http://hg.mozilla.org/mozilla-central/rev/ed5db6019d75
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Backed out Ehsan's asserts, they set the tree orange. Please tryserver and file a new bug if necessary.

http://hg.mozilla.org/mozilla-central/rev/18c62a838be0
I had to remove another assert, this time a crash test was failing. The stacks should tell the story. Please re-file and patch whats left.

http://hg.mozilla.org/mozilla-central/rev/919f15d71153
Attached patch Additional needed fixes (obsolete) — Splinter Review
I totally forgot that we don't have scriptblockers while overridden UnsetAttr implementations are running. This of course makes the assertions fire like mad and leaves this bug still open.

The XUL changes are only there to quiet the assertions. Need to look into how to fix those more correctly, but that can wait for a separate bug.
Attachment #516194 - Flags: review?(bzbarsky)
Whiteboard: [sg:critical][hardblocker][has patch][ETA: 3/1] → [sg:critical][hardblocker][has patch][needs review][ETA: 3/2]
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch Additional needed fixes v1.5 (obsolete) — Splinter Review
This gets all needed ::UnsetAttr implementations.
Attachment #516194 - Attachment is obsolete: true
Attachment #516194 - Flags: review?(bzbarsky)
Attachment #516202 - Flags: review?(bzbarsky)
bz, can you please check the tryserver results and if you + the patch can you please land it on m-c first thing in the morning?
Using non-removable blockers in nsXULElement causes assertions in the mutation-events code, even if they are removed when we actually fire the event. So use "real" removable scriptblockers instead.
Attachment #516202 - Attachment is obsolete: true
Attachment #516202 - Flags: review?(bzbarsky)
Attachment #516223 - Flags: review?(bzbarsky)
This just adds back the assertions that were backed out to fix the orange.
Comment on attachment 516223 [details] [diff] [review]
Additional needed fixes v2

Hah!  I was going to ask for an auto removable script blocker before I opened this patch... ;)

r=me
Attachment #516223 - Flags: review?(bzbarsky) → review+
I pushed that patch as http://hg.mozilla.org/mozilla-central/rev/23cf0cedfd4a and the asserts.

Then I backed out the asserts, because they fire.  In particular, we have various BindToTree/UnbindFromTree callers that are not under scripblockers.  For example:

  nsGenericElement::RemoveFromIdTable [content/base/src/nsGenericElement.h:1046]
  nsStyledElement::UnbindFromTree [content/base/src/nsStyledElement.cpp:241]
  AnonymousContentDestroyer::Run [content/base/src/nsContentUtils.cpp:4197]
  nsContentUtils::RemoveScriptBlocker [content/base/src/nsContentUtils.cpp:4729]
  nsAutoScriptBlocker::~nsAutoScriptBlocker [nsContentUtils.h:1896]
  DocumentViewerImpl::DestroyPresShell [layout/base/nsDocumentViewer.cpp:4330]

Jonas, do you think it's better to add script blockers in the Unbind/Bind impls involved, in the nsGenericElement function at the top of the stack there, or in places that do the bind/unbind?  The latter might be difficult to catch without just adding asserts to bind/unbind about being in a script blocker.
jonas: test case is thoroughly fixed, will do more work in a follow-up bug post FF4. bz concurs
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
I'll file a followup bug on relanding and fixing the assertions.
Whiteboard: [sg:critical][hardblocker][has patch][needs review][ETA: 3/2] → [sg:critical][hardblocker][ETA: 3/2]
Flags: in-testsuite? → in-testsuite+
#1 - Can we get a rollup patch for branch?
#2 - Does this affect 1.9.1?
#3 - Is there more work to be done for branch due to remote XUL being enabled there (as comment 46 suggests)
Re: #2 yes, I think it does as 1.9.1 has live ID tracking.
I rewrote the id handling significantly for FF4. It's very possible that prior to that we didn't crash at all, or crash in exploitable ways.
Depends on: 640652
(In reply to comment #64)
> I'll file a followup bug on relanding and fixing the assertions.

Jonas: Did that happen?

fwiw the crashtests don't crash the 1.9.1 or 1.9.2 branches.

Hard to tell what the final state of the code is with all the various checkins and backouts, especially not sure what the end results of comment 62 were. However it does look like these patches apply to the branches (minus the bits about nsDocument::MozSetImageElement which is new).
Whiteboard: [sg:critical][hardblocker][ETA: 3/2] → [sg:critical?][hardblocker][ETA: 3/2]
(In reply to comment #68)
> (In reply to comment #64)
> > I'll file a followup bug on relanding and fixing the assertions.
> 
> Jonas: Did that happen?

Filed bug 641106.

> fwiw the crashtests don't crash the 1.9.1 or 1.9.2 branches.
> 
> Hard to tell what the final state of the code is with all the various checkins
> and backouts, especially not sure what the end results of comment 62 were.
> However it does look like these patches apply to the branches (minus the bits
> about nsDocument::MozSetImageElement which is new).

Given that the testcase doesn't crash old branches, and the fact that this bug has caused regressions (bug 640652), I would say that we should not land this on old branches. At least not unless we get a stronger indication that this does indeed crash old branches.
OK, setting branches to "wontfix" for now, but that can change if we get evidence this bug does actually affect them.
blocking1.9.2: ? → ---
Group: core-security
Depends on: 1352389
You need to log in before you can comment on or make changes to this bug.