350629 - GeneratePrefix will return "xml" and "xmlns" prefixes

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jeff Walden [:Waldo]]

Per the XML namespaces spec, these prefixes are invalid (and with the moving around of code happening in bug 350442, will cause 'xmlns:xmlns="..."' or 'xmlns:xml="..."' in XML.toXMLString(), both of which are highly invalid).


Example:

js> var f = <foo/>
js> var x = new Namespace("http://foo/bar.xml");
js> var y = new Namespace("http://foo/bar.xmlns");
js> f.@x::attrXML = "fun";
fun
js> f.@y::attrXMLNS = "more fun";
more fun
js> f.toXMLString()
<foo xml:attrXML="fun" xmlns:attrXMLNS="more fun" xmlns:xml="http://foo/bar.xml" xmlns:xmlns="http://foo/bar.xmlns"/>


Sub-module owner, eh?  :-\

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Patch

This patch performs correctly on the tests in the forthcoming testcase, which I believe to be exhaustive for the purposes of this bug.  (Note that I was imprecise in the original report: the spec actually reserves *all* prefixes beginning with "xml", so "xmln", "xmlfoopy", etc. shouldn't be generated, either.)

Side note: should there be a JS_ASSERT(decls->length != 0) in GeneratePrefix?

Comment 2

[Jeff Walden [:Waldo]]

Attachment: Testcase

Comment 3

[Brendan Eich [:brendan]]

Comment on attachment 236201 [details] [diff] [review]
Patch

>Index: jsxml.c
>===================================================================
>RCS file: /cvsroot/mozilla/js/src/jsxml.c,v
>retrieving revision 3.122
>diff -p -u -8 -r3.122 jsxml.c
>--- jsxml.c	29 Aug 2006 21:10:54 -0000	3.122
>+++ jsxml.c	31 Aug 2006 08:28:28 -0000
>@@ -2508,30 +2508,52 @@ GeneratePrefix(JSContext *cx, JSString *
>      * ".../there.is.only.xul", "xbl" given ".../xbl", and "xbl2" given any
>      * likely URI of the form ".../xbl2/2005".
>      */
>     start = JSSTRING_CHARS(uri);
>     cp = end = start + JSSTRING_LENGTH(uri);
>     while (--cp > start) {
>         if (*cp == '.' || *cp == '/' || *cp == ':') {
>             ++cp;
>-            if (IsXMLName(cp, PTRDIFF(end, cp, jschar)))
>+            length = PTRDIFF(end, cp, jschar);
>+            if (IsXMLName(cp, length) &&
>+                !STARTS_WITH_XML(cp, length))

This fits on one line, so don't wrap after && -- if you had to, local style would brace the then clause even though it's a one-liner (if any part of the if condition, then, or else takes more than one line, including comments, brace all dependent statements).

>                 break;
>             end = --cp;
>         }
>     }
>     length = PTRDIFF(end, cp, jschar);
> 
>     /*
>+     * If the namespace consisted only of non-XML names or names which begin
>+     * case-insensitively with "xml", arbitrarily create a prefix consisting of
>+     * 'a's of size length (allow dp-calculating code to work with or without
>+     * this branch executing) plus the space for storing a hyphen and serial
>+     * number (avoid reallocation if a collision happens).

s/avoid/avoiding/

Thanks for this fix -- and yeah, asserting decls->length != 0 is fine (it can be proved by studying the code, in the absense of memory corruption, but assertions are good for documenting and for future sanity checkin).

/be

Comment 4

[Jeff Walden [:Waldo]]

Attachment: Fixes nits (and an unforeseen decls->length wrinkle)

Conveniently enough, adding the assertion demonstrated that it can be violated (!) (and even more conveniently, by a test in the testcase I posted here).  The root cause is |if (!ns->declared) continue;| code in XMLToXMLString, which in the mentioned example means that decls is empty when GeneratePrefix is called.


+        prefix = JS_NewStringCopyZ(cx, "a");
+        if (JSSTRING_LENGTH(prefix) == 0)
+            prefix = NULL;
+        return prefix;

Please tell me there's a function which accepts a const char* and returns a JSString* or NULL on failure (or suggest something stylistically prettier than this); I saw none in the JSAPI reference or in jsstr.h.

Comment 5

[Brendan Eich [:brendan]]

Comment on attachment 236586 [details] [diff] [review]
Fixes nits (and an unforeseen decls->length wrinkle)

>+    if (decls->length == 0) {
>+        prefix = JS_NewStringCopyZ(cx, "a");
>+        if (JSSTRING_LENGTH(prefix) == 0)
>+            prefix = NULL;
>+        return prefix;

I didn't understand your API question, but this is wrong:

1.  prefix will be null on OOM, so JSSTRING_LENGTH will crash.

2.  If no OOM, then prefix is guaranteed to refer to a JSString containing the jschar 'a' followed by an uncounted-by-length backstop 0, with length 1.

What are you trying to check here, exactly?

/be

Comment 6

[Brendan Eich [:brendan]]

Thanks for figuring out so much of E4X.  There's a long story about the declared flag, and the supposed "right" way to do ToXMLString, according to the unwritten intent of the spec... next time you are in town.

/be

Comment 7

[Jeff Walden [:Waldo]]

(In reply to comment #5)
> 1.  prefix will be null on OOM, so JSSTRING_LENGTH will crash.

Not according to <http://developer.mozilla.org/en/docs/JS_NewStringCopyZ> -- is this a bug in the JSAPI reference?  If it is, then I get the two-line prettiness I wanted:

>+    if (decls->length == 0)
>+        return JS_NewStringCopyZ(cx, "a");

Comment 8

[Brendan Eich [:brendan]]

(In reply to comment #7)
> is this a bug in the JSAPI reference?

Yeah (surprise surprise). That was never true. Confusion with JS_GetStringBytes? Not likely.

All JS_New* API entry points return null on OOM, period, end of story.

/be

Comment 9

[Jeff Walden [:Waldo]]

Attachment: Patch (final) (hopefully)

JS_NewStringCopyZ returns an empty string for args (cx, NULL); I think this is the source of the docs confusion.  Are we still trying to keep the docs in CVS up-to-date, or are they abandoned in favor of developer.m.o docs?  I'll file a bug to either update or remove them.

Comment 10

[Brendan Eich [:brendan]]

Comment on attachment 236719 [details] [diff] [review]
Patch (final) (hopefully)


>     /*
>+     * If the namespace consisted only of non-XML names or names which begin

s/which/that/ for defining clause linkage.

>+     * case-insensitively with "xml", arbitrarily create a prefix consisting of
>+     * 'a's of size length (allowing dp-calculating code to work with or without
>+     * this branch executing) plus the space for storing a hyphen and serial

"the" before "serial".

With nits picked, r=me.  Thanks,

/be

Comment 11

[Brendan Eich [:brendan]]

(In reply to comment #9)
> JS_NewStringCopyZ returns an empty string for args (cx, NULL); I think this is
> the source of the docs confusion.  Are we still trying to keep the docs in CVS
> up-to-date, or are they abandoned in favor of developer.m.o docs?  I'll file a
> bug to either update or remove them.

Abandoned if the http://www.mozilla.org/js/spidermonkey links all point to MDC.

You may cvs remove at will (leave a README pointing to MDC?).

/be

Comment 12

[Jeff Walden [:Waldo]]

Comment on attachment 236719 [details] [diff] [review]
Patch (final) (hopefully)

Patch checked in on trunk.

I think this would be a good correctness fix for 2.0 over 1.5 (where bug 350206 prevented this bug from ever happening).  In addition to fixing the behavior described in the bug summary, it prevents the calling of the |log10| function with an argument of 0, which according to the documentation I have results in undefined behavior (which works on Linux but very possibly doesn't on Windows, since the best docs I've found say log10(0) there returns infinity).

Comment 13

[Mike Schroepfer]

Brendan can you comment on risk/reward for this for 1.8 branch?

Comment 14

[Brendan Eich [:brendan]]

(In reply to comment #12)
> (From update of attachment 236719 [details] [diff] [review] [edit])
> Patch checked in on trunk.
> 
> I think this would be a good correctness fix for 2.0 over 1.5 (where bug 350206
> prevented this bug from ever happening).  In addition to fixing the behavior
> described in the bug summary, it prevents the calling of the |log10| function
> with an argument of 0, which according to the documentation I have results in
> undefined behavior (which works on Linux but very possibly doesn't on Windows,
> since the best docs I've found say log10(0) there returns infinity).

Infinity cast to (size_t) is undefined, may be a very large number, and that number is a term in a sum fed into malloc -- so this is an important fix.  It is low-risk both because of the local scope of the changes, and because it's E4X.  Even if we don't care about fixing it for E4X users, we should fix it to reduce risk of malloc size wrapping exploit.

/be

Comment 15

[Mike Schroepfer]

Comment on attachment 236719 [details] [diff] [review]
Patch (final) (hopefully)

a=schrep for drivers based on risk assessment from comment 16.

Comment 16

[Jeff Walden [:Waldo]]

Fixed on branch.

Comment 17

[Bob Clary [:bc] (inactive)]

Jeff, please use one of the TEST* functions for e4x tests. reportCompare is not defined in the e4x/shell.js or e4x/browser.js. But thanks for the full test!

Checking in regress-350629.js;
/cvsroot/mozilla/js/tests/e4x/Regress/regress-350629.js,v  <--  regress-350629.js
initial revision: 1.1

Comment 18

[Bob Clary [:bc] (inactive)]

verified fixed 1.8 1.9 20060909 windows/mac*/linux