Closed Bug 527558 Opened 13 years ago Closed 7 years ago

Crash [@ xul.dll!nsDependentCSubstring::nsDependentCSubstring]

Categories

(Core :: XSLT, defect)

x86
All
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed
status1.9.1 --- wanted

People

(Reporter: pvnick, Assigned: peterv)

References

Details

(Keywords: crash, testcase, Whiteboard: [sg:dos stack exhaustion])

Crash Data

Attachments

(3 files, 2 obsolete files)

Attached file testcase
Stack overflow in an xsl transform using the key function.
Attached file stack
This bug is hit very quickly, so I had to disable key() testing in the fuzzer. No rush, but I can't test key() anymore until this is fixed.
Assignee: nobody → peterv
Status: NEW → ASSIGNED
Attached patch v1 (obsolete) — Splinter Review
This seems to catch the recursion. Still need to add a crashtest.
Attached patch v1 (obsolete) — Splinter Review
Attachment #412410 - Attachment is obsolete: true
Attached patch v2Splinter Review
Branch would get this patch without the change to xslt.properties and we'll just report an unknown error there.
Attachment #412412 - Attachment is obsolete: true
Attachment #412476 - Flags: review?(jonas)
Whiteboard: [sg:dos stack exhaustion]
Aren't use-expressions allowed to reference keys? As long as that doesn't happen in a circular fashion.

I wonder if we should simply do what the js-engine does and have a catch-all recursion detector based on measuring the amount of C++ stack used.
(In reply to comment #6)
> Aren't use-expressions allowed to reference keys? As long as that doesn't
> happen in a circular fashion.

Nope: http://www.w3.org/1999/11/REC-xslt-19991116-errata/#E13.
Comment on attachment 412476 [details] [diff] [review]
v2

Why the ignoreError stuff? Couldn't technically XSLT2 allow keys as long as they aren't used circularly? We still create an ErrorExpr, so no risk of out of control recursions.

Nit: I would prefer if 'allowed' was called something more descriptive? Like exprTypeAllowed or some such?

r=me either way.
(In reply to comment #8)
> (From update of attachment 412476 [details] [diff] [review])
> Why the ignoreError stuff? Couldn't technically XSLT2 allow keys as long as
> they aren't used circularly?

I really don't see how that would fit into 1 of the 6 bullet points in http://www.w3.org/TR/xslt#forwards. And for patterns we don't create ErrorExprs, so use and match would be treated differently?

> Nit: I would prefer if 'allowed' was called something more descriptive? Like
> exprTypeAllowed or some such?

Yeah, I forgot to tell you I'm looking for better names and suggestions are welcome ;-).
Keywords: crash
OS: Windows XP → All
Keywords: testcase
Crash Signature: [@ xul.dll!nsDependentCSubstring::nsDependentCSubstring]
Peter: Does this still apply? Should we try to land it?
(In reply to Jonas Sicking (:sicking) from comment #10)
> Peter: Does this still apply? Should we try to land it?

Needed some updating, thanks for reminding me.
https://hg.mozilla.org/mozilla-central/rev/d858d1b52b6b
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.