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

Status: RESOLVED FIXED

(Core :: XSLT, defect)

Description

[Paul Nickerson]

Attachment: testcase

Stack overflow in an xsl transform using the key function.

Comment 1

[Paul Nickerson]

Attachment: stack

Comment 2

[Paul Nickerson]

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.

Comment 3

[Peter Van der Beken [:peterv]]

Attachment: v1

This seems to catch the recursion. Still need to add a crashtest.

Comment 4

[Peter Van der Beken [:peterv]]

Attachment: v1

Comment 5

[Peter Van der Beken [:peterv]]

Attachment: v2

Branch would get this patch without the change to xslt.properties and we'll just report an unknown error there.

Comment 6

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

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.

Comment 7

[Peter Van der Beken [:peterv]]

(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 8

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

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.

Comment 9

[Peter Van der Beken [:peterv]]

(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 ;-).

Comment 10

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

Peter: Does this still apply? Should we try to land it?

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d858d1b52b6b

Comment 12

[Peter Van der Beken [:peterv]]

(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.

Comment 13

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/d858d1b52b6b