311157 - Comment-hiding compromise left E4X parsing/scanning inconsistent

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Brendan Eich [:brendan]]

Given the final state of affairs after bug 311071 was fixed, with XML element
and list literals unambiguous in all script tags (with or without e4x=1 or a new
MIME type), but XML comment and CDATA literals not supported because they're
ambiguous with HTML comment hiding hacks, there are some holes in the
parser/scanner state space.  For example,

x = <hi> <!-- duh -->
    there </hi>;

should work in a script tag, but it does not.  An assertion in jsparse.c fires
and a bad parse tree is constructed, leading to a crash.  The problem is that
the scanner collects the space before the <!-- and stops on the <, thinking it
will be tokenized as an XML token (start-tag opener, etc.).  But thanks to the
comment hiding hack support, it's skipped (characters to end of line are eaten).

That means the next token is not something to-do with <, but a TOK_NAME for
'there'.  The code in XMLElementContent in jsparse.c asserts otherwise, then
blithely makes a parse node assuming it got the expected type (one of TOK_XMLPI,
TOK_XMLCDATA, and TOK_XMLCOMMENT).  For TOK_NAME, this is safe, but not so for
other token types.

The straightforward fix is to toggle JSOPTION_XML when parsing XMLElementContent
(easy to do, restores the previous coupling between the TSF_XMLTEXTMODE scanner
subset and the comment-hiding hack, by enabling the latter when using the former
to scan text up to a < in a tag).

/be

Comment 1

[Brendan Eich [:brendan]]

Should fix for RC1, not a 1.8b5 stopper.  Potential for security exploit low but
non-zero given content-controlled parse tree type botch.

/be

Comment 2

[Brendan Eich [:brendan]]

Attachment: proposed fix

Straighforward refactoring to consolidate JSOPTION_XML toggling and avoid doing
it when recursing through XMLElementOrList or XMLElementContent.

/be

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 198546 [details] [diff] [review]
proposed fix

r=mrbkap

Comment 4

[Mike Shaver (:shaver emeritus)]

Comment on attachment 198546 [details] [diff] [review]
proposed fix

sr=shaver

Comment 5

[Brendan Eich [:brendan]]

Fixed on the trunk.

/be

Comment 6

[Brendan Eich [:brendan]]

Comment on attachment 198546 [details] [diff] [review]
proposed fix

This is a necessary fix to restore lost consistency between the scanner and the
parser.

/be

Comment 7

[Bob Clary [:bc] (inactive)]

RCS file: /cvsroot/mozilla/js/tests/e4x/Expressions/11.4.1-04.js,v
done
Checking in 11.4.1-04.js;
/cvsroot/mozilla/js/tests/e4x/Expressions/11.4.1-04.js,v  <--  11.4.1-04.js
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/js/tests/e4x/Expressions/11.4.1-05.js,v
done
Checking in 11.4.1-05.js;
/cvsroot/mozilla/js/tests/e4x/Expressions/11.4.1-05.js,v  <--  11.4.1-05.js
initial revision: 1.1
done

Comment 8

[Bob Clary [:bc] (inactive)]

Checking in regress-311157-01.js;
/cvsroot/mozilla/js/tests/js1_6/Regress/regress-311157-01.js,v  <-- 
regress-311157-01.js
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/js/tests/js1_6/Regress/regress-311157-02.js,v
done
Checking in regress-311157-02.js;
/cvsroot/mozilla/js/tests/js1_6/Regress/regress-311157-02.js,v  <-- 
regress-311157-02.js
initial revision: 1.1
done

Comment 9

[Brendan Eich [:brendan]]

Fixed on branch, thanks.

/be

Comment 10

[Yuh-Ruey Chen]

So lemme get this straight. All XML comments and CDATA tags are ignored when the
MIME type doesn't have e4x=1. Everything else works regardless of whether e4x=1.
Is that correct? Are there any cases in which XML comments are parsed without e4x=1?

Comment 11

[Brendan Eich [:brendan]]

(In reply to comment #10)
> So lemme get this straight. All XML comments and CDATA tags are ignored when the
> MIME type doesn't have e4x=1.

No, only top-level ones.

> Are there any cases in which XML comments are parsed without e4x=1?

Yes, if they're contained in a list or element.  Same for CDATA.

/be

Comment 12

[Bob Clary [:bc] (inactive)]

no crash firefox 1.5 rc2 winxp/linux

Comment 13

[Bob Clary [:bc] (inactive)]

verified fixed 1.9 windows/mac(ppc|tel)/linux 20060812