358594 - "Assertion failure: vlength > n" calling uneval(this) (involves __proto__ and serialization using sharps?)

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jesse Ruderman]

js> function f() { } f.__proto__ = this; this.m setter = f; uneval(this);
Assertion failure: vlength > n, at jsobj.c:938

The code is trying to remove "(function " and ")" from something that turns out to be a sharp variable.  Security sensitive because it looks like the code might jump past the end of a string.

   /*
    * Remove '(function ' from the beginning of valstr and ')' from the
    * end so that we can put "get" in front of the function definition.
    */
   if (gsop[j] && VALUE_IS_FUNCTION(cx, val[j])) {
       size_t n = strlen(js_function_str) + 2;
       JS_ASSERT(vlength > n);
       vchars += n;
       vlength -= n + 1;
   }


Strange behavior in opt:

js> function f() { } f.__proto__ = this; this.m setter = f; uneval(this);
({f:#1={prototype:{}}, set m #1#})

js> function f() { } f.hhhhhhhhh = this; this.m setter = f; uneval(this);
#1={f:#2=function f() {}, set m #2#}

Comment 1

[Jesse Ruderman]

Btw, the sharp stuff I mentioned at the end of comment 0 doesn't play well with the "set" syntax when trying to eval again.

js> a = {}; h = function() { }; a.b getter = h; a.c getter = h; print(uneval(a)); eval(uneval(a));
({get b #1=() {}, get c #1#})
typein:23: SyntaxError: missing ( before formal parameters:
typein:23: ({get b #1=() {}, get c #1#})
typein:23: ........^

(I didn't test this before because I thought sharp variables were output-only!)

Comment 2

[Jesse Ruderman]

This bug is getting in the way of my testing :(

Comment 3

[Jesse Ruderman]

Maybe sharped functions just need to use the "x getter: " syntax rather than the "get x" syntax.

Comment 5

[Brendan Eich [:brendan]]

(In reply to comment #3)
> Maybe sharped functions just need to use the "x getter: " syntax rather than
> the "get x" syntax.

Yes, that's the ticket. Brian, can you detect a # at the front of the value string and switch to this syntax?

/be

Comment 6

[Brian Crowder]

Attachment: use new "needOldStyleGetterSetter" logic

There must be a decompiler version of this same bug, though what it is isn't obvious to me.  This uses the logic added in bug 356083 with some refactoring to handle the value side of expressions (previously only calculated needOldStyleGetterSetter for the propid).

Comment 7

[Jesse Ruderman]

> There must be a decompiler version of this same bug, though what it is
> isn't obvious to me.

The decompiler seems to get it right: it collapses some uses of "getter" in object literals into the new "get" syntax, but leaves ones with sharps alone.

js> function() { return { x getter: function(){} } }
function () {
    return {get x() {}};
}

js> function() { return { x getter: #1=function(){} } } 
function () {
    return {x getter:#1=function () {}};
}

js> function() { return { x getter: #1# } }              
function () {
    return {x getter:#1#};
}

Comment 8

[Brian Crowder]

Comment on attachment 264243 [details] [diff] [review]
use new "needOldStyleGetterSetter" logic

Moving review to Igor

Comment 9

[Igor Bukanov]

Comment on attachment 264243 [details] [diff] [review]
use new "needOldStyleGetterSetter" logic

>                     val[valcnt] = (jsval) ((JSScopeProperty *)prop)->getter;
>-#ifdef OLD_GETTER_SETTER
>-                    gsop[valcnt] =
>+                    gsopold[valcnt] =
>                         ATOM_TO_STRING(cx->runtime->atomState.getterAtom);
>-#else
>-                    gsop[valcnt] = needOldStyleGetterSetter
>-                        ? ATOM_TO_STRING(cx->runtime->atomState.getterAtom)
>-                        : ATOM_TO_STRING(cx->runtime->atomState.getAtom);
>-#endif

Why the patch removes OLD_GETTER_SETTER ifdefs?

Comment 10

[Brian Crowder]

Comment on attachment 264243 [details] [diff] [review]
use new "needOldStyleGetterSetter" logic

 #ifndef OLD_GETTER_SETTER
+            if (vchars[0] == '#')
+#endif
+                needOldStyleGetterSetter = JS_TRUE;
+

We use that ifdef to make this decision later, so the ifdefs above are just  old untidiness from the last time I worked in this code.

Comment 11

[Igor Bukanov]

Comment on attachment 264243 [details] [diff] [review]
use new "needOldStyleGetterSetter" logic

> #ifndef OLD_GETTER_SETTER
>+            if (vchars[0] == '#')
>+#endif
>+                needOldStyleGetterSetter = JS_TRUE;
>+
>+            if (needOldStyleGetterSetter)
>+                gsop[j] = gsopold[j];
>+
>+#ifndef OLD_GETTER_SETTER
>             /*
>              * Remove '(function ' from the beginning of valstr and ')' from the
>              * end so that we can put "get" in front of the function definition.
>              */


That if wrappend into #ifndef makes code hard to follow. I suggest to move it into the following +#ifndef OLD_GETTER_SETTER block and add a copy of "gsop[j] = gsopold[j];" as #else code.

r+ with that fixed.

Comment 12

[Brian Crowder]

Attachment: implementation v2

This addresses Igor's comments, some bugs I encountered in testing this patch and cleans up an old preprocessor-guarded section of code, which really is no longer necessary.  People defining OLD_GETTER_SETTER will suffer some bloat, but the code is tidier now.

Comment 13

[Brian Crowder]

Attachment: implementation v2a

A minor tweak for OLD_GETTER_SETTER users

Comment 14

[Brian Crowder]

jsobj.c: 3.335

Comment 15

[Daniel Veditz [:dveditz]]

on the 1.8 branch I hit the original assertion using a debug xpcshell but not when I try to run the code in the browser. What's the potential security impact for Mozilla clients from this flaw? Do we need to land this on the 1.8 branch?

Comment 16

[Brian Crowder]

I think this is a relatively harmless (ie., crash, but nothing more) UMR on the branches.

Comment 17

[Jesse Ruderman]

Does it make decisions based on the UMR that might reveal sensitive information in Firefox's address space.

Comment 18

[Blake Kaplan (:mrbkap) (inactive)]

Ah, there might be a privacy problem, then, since this string can be printed and inspected for sensitive information.

Comment 19

[Brian Crowder]

Attachment: MOZILLA_1_8_BRANCH patch, roll-up

This should pick up fixes from bug 358594, bug 381303, bug 356083
(pre-requisite for 358594), bug 381211 (another bug introduced here,
fixed in bug 367629), and bug 380933.  I think if we're going to
do a branch patch here, we should get all of these.  If that is
undesirable, I will try to back out a few of them.  That proposal
is more painful than using this, though.

(not ready for review yet, still needs more testing)

Comment 20

[Brian Crowder]

The bug in comment #1 is still not fixed here (on trunk or by my patch roll-up).  Jesse, would you mind opening another bug for that?

Comment 21

[Jesse Ruderman]

I already filed bug 380831 for that.

Comment 22

[Brian Crowder]

Comment on attachment 265857 [details] [diff] [review]
MOZILLA_1_8_BRANCH patch, roll-up

This roll-up catches brokenness from all the testcases in this and the bugs mentioned in comment #19, except as noted in comment #20.  I haven't had time to run the full JS test suite on it.

Comment 23

[Brian Crowder]

(In reply to comment #21)
> I already filed bug 380831 for that.

Which explains why I thought I'd fixed that.

Comment 24

[Brian Crowder]

Comment on attachment 265857 [details] [diff] [review]
MOZILLA_1_8_BRANCH patch, roll-up

Do we want 1.8.0.x for this, too?  Will wait for both approval and test-suite blessing to land this.

Comment 25

[Bob Clary [:bc] (inactive)]

Attachment: comparison of before|after MOZILLA_1_8_BRANCH rollup patch

There are some changes in decompilation that aren't cool.

Comment 26

[Brian Crowder]

Thanks, Bob.  I will address these and upload another attempt in a day or so.

Comment 27

[Bob Clary [:bc] (inactive)]

Attachment: js1_7/regress/regress-358594.js

This doesn't include any of the decompiler tests.

Comment 28

[Brian Crowder]

I'm not worried about the two performance bugs here; 101964 is only showing a delta because it's value is different.  The orders of magnitude are the same.  There's no reason this test should affect the performance of sort() (bug 99120), either.  Not sure why there's a delta there, but it seems like perhaps just a change in the test environment or something (maybe the file moved?)

I'll have a patch to address the others shortly.

Comment 29

[Brian Crowder]

It looks like the rest of these are addressed in the patch for bug 355736.  That bug should be nominated for branch approval if we care, otherwise we should probably just land this.  Bob, can I get you to try the test-suite again when you get a chance?  If nothing has changed, I think we can land this...  I have a sneaking suspicion a few more tweaks have happened since I did the roll-up.

Comment 30

[Bob Clary [:bc] (inactive)]

I'll get to it this evening on linux at the least.

Comment 31

[Bob Clary [:bc] (inactive)]

crowder, do you just want the vanilla trunk tested? From what time period? I already have vanilla 1.8, trunk tests running on all three platforms with builds from this morning. Is that sufficient for your needs?

Comment 32

[Brian Crowder]

I want a 1.8 engine, but run against the trunk testsuite (if they differ) to make sure that this patch doesn't regress anything and that it improves the decompilation/obj_toSource situation.  I'm not worried about '' quoted keywords, though; as I mentioned that should be handled in another patch.  The patch for bug 355736 applies cleanly on 1.8

Comment 33

[Bob Clary [:bc] (inactive)]

crowder, I've been trying to get a good result for this on 1.8 with the patch but am having problems for which I can not conclusively blame this patch. I'll keep trying and see if I can get a definite answer for you tonight.

Comment 34

[Bob Clary [:bc] (inactive)]

Comment on attachment 266313 [details]
js1_7/regress/regress-358594.js

js1_7 not required.

Comment 35

[Bob Clary [:bc] (inactive)]

Attachment: js1_5/extensions/regress-358594-01.js

Comment 36

[Bob Clary [:bc] (inactive)]

Attachment: js1_5/extensions/regress-358594-02.js

Comment 37

[Bob Clary [:bc] (inactive)]

Attachment: js1_5/extensions/regress-358594-03.js

Comment 38

[Bob Clary [:bc] (inactive)]

Attachment: js1_5/extensions/regress-358594-04.js

Comment 39

[Bob Clary [:bc] (inactive)]

Attachment: js1_5/extensions/regress-358594-05.js

Comment 40

[Bob Clary [:bc] (inactive)]

Attachment: js1_5/extensions/regress-358594-06.js

Comment 41

[Bob Clary [:bc] (inactive)]

Attachment: difference between 1.8.1 without patch and with patch

These differences are all due to (from what I can tell) bugs which are fixed on the trunk but not branch. /me stamps approval fwiw

Comment 42

[Daniel Veditz [:dveditz]]

Comment on attachment 265857 [details] [diff] [review]
MOZILLA_1_8_BRANCH patch, roll-up

approved for 1.8.1.5, a=dveditz for release-drivers

Comment 43

[Brian Crowder]

Checking in jsobj.c;
/cvsroot/mozilla/js/src/jsobj.c,v  <--  jsobj.c
new revision: 3.208.2.52; previous revision: 3.208.2.51
done
Checking in jsopcode.c;
/cvsroot/mozilla/js/src/jsopcode.c,v  <--  jsopcode.c
new revision: 3.89.2.72; previous revision: 3.89.2.71
done

Comment 44

[juan becerra [:juanb]]

bc, could you help verifying this fix on the latest 2.0.0.5 rc builds?

Comment 45

[Bob Clary [:bc] (inactive)]

verified fixed 1.8, 1.9.0 windows, linux, macppc with 7/16 opt/debug shell/browser.

Comment 46

[Alexander Sack]

Attachment: roll-up for 1.8.0

Comment 47

[Igor Bukanov]

(In reply to comment #46)
> Created an attachment (id=274461) [details]
> roll-up for 1.8.0
> 

Could you add a patch using the same cvs diff -u -p -8 options as the patch from comment 19 and also add a plain diff between patches to simplify the review?

Comment 48

[Igor Bukanov]

Comment on attachment 274461 [details] [diff] [review]
roll-up for 1.8.0

Sorry for a late review, I forgot about it.

>Index: mozilla/js/src/jsopcode.c
>===================================================================
>--- mozilla.orig/js/src/jsopcode.c	2007-07-16 16:36:40.000000000 +0200
>+++ mozilla/js/src/jsopcode.c	2007-07-18 12:22:30.000000000 +0200
>@@ -61,16 +61,17 @@
...
>                 if (lastop == JSOP_GETTER || lastop == JSOP_SETTER) {
>                     rval += strlen(js_function_str) + 1;
>-                    todo = Sprint(&ss->sprinter, "%s%s%s %s%.*s",
>-                                  lval,
>-                                  (lval[1] != '\0') ? ", " : "",
>-                                  (lastop == JSOP_GETTER)
>-                                  ? js_get_str : js_set_str,
>-                                  xval,
>-                                  strlen(rval) - 1,
>-                                  rval);
>+                    if (!atom || !ATOM_IS_STRING(atom) ||
>+                        !ATOM_IS_IDENTIFIER(atom) ||
>+                        !!ATOM_KEYWORD(js_AtomizeChars(cx,
>+                                                     ATOM_TO_STRING(atom),
>+                                                     sizeof(ATOM_TO_STRING(atom)),
>+                                                     0))||

No need to re-atomize the atom here meaning that
  atom == js_AtomizeChars(cx, JSSTRING_CHARS(ATOM_TO_STRING(atom)), JSSTRING_LENGTH(ATOM_TO_STRING(atom)), 0).

In any case 
  js_AtomizeChars(cx, ATOM_TO_STRING(atom), sizeof(ATOM_TO_STRING(atom)), 0)
is bogus and should generate at least warnings on the wrong pointer type of js_AtomizeChars argumnet.

Now given that ATOM_IS_IDENTIFIER is:

  !ATOM_KEYWORD(atom) && js_IsIdentifier(ATOM_TO_STRING(atom))

Then !ATOM_IS_IDENTIFIER(atom) is

  ATOM_KEYWORD(atom) || !js_IsIdentifier(ATOM_TO_STRING(atom))

meaning that !!ATOM_KEYWORD() can be omitted.

>Index: mozilla/js/src/jsobj.c
>===================================================================
>@@ -806,91 +808,111 @@
>     /*
>      * We have four local roots for cooked and raw value GC safety.  Hoist the
>      * "argv + 2" out of the loop using the val local, which refers to the raw
>      * (unconverted, "uncooked") values.
>      */
>     val = argv + 2;
> 
>     for (i = 0, length = ida->length; i < length; i++) {
>+        JSBool idIsLexicalIdentifier, needOldStyleGetterSetter;
>+        char *atomstrchars;
>+
>         /* Get strings for id and value and GC-root them via argv. */
>         id = ida->vector[i];
> 
> #if JS_HAS_GETTER_SETTER
>-
>         ok = OBJ_LOOKUP_PROPERTY(cx, obj, id, &obj2, &prop);
>         if (!ok)
>             goto error;
>+#endif
>+
>+        /*
>+         * Convert id to a jsval and then to a string.  Decide early whether we
>+         * prefer get/set or old getter/setter syntax.
>+         */
>+        atom = JSID_IS_ATOM(id) ? JSID_TO_ATOM(id) : NULL;
>+        idstr = js_ValueToString(cx, ID_TO_VALUE(id));
>+        if (!idstr) {
>+            ok = JS_FALSE;
>+            OBJ_DROP_PROPERTY(cx, obj2, prop);
>+            goto error;
>+        }
>+        *rval = STRING_TO_JSVAL(idstr);         /* local root */
>+        idIsLexicalIdentifier = js_IsIdentifier(idstr);
>+
>+        atomstrchars = ATOM_TO_STRING(atom);
>+        needOldStyleGetterSetter = 
>+            !idIsLexicalIdentifier ||
>+            ATOM_KEYWORD(js_AtomizeChars(cx,
>+                                     atomstrchars,
>+                                     sizeof(atomstrchars),
>+                                     0)) != TOK_EOF;
>+

Again, use ATOM_KEYWORD(atom) here.

Comment 49

[Bob Clary [:bc] (inactive)]

/cvsroot/mozilla/js/tests/js1_5/extensions/regress-358594-01.js,v  <--  regress-358594-01.js
initial revision: 1.1

/cvsroot/mozilla/js/tests/js1_5/extensions/regress-358594-02.js,v  <--  regress-358594-02.js
initial revision: 1.1

/cvsroot/mozilla/js/tests/js1_5/extensions/regress-358594-03.js,v  <--  regress-358594-03.js
initial revision: 1.1

/cvsroot/mozilla/js/tests/js1_5/extensions/regress-358594-04.js,v  <--  regress-358594-04.js
initial revision: 1.1

/cvsroot/mozilla/js/tests/js1_5/extensions/regress-358594-05.js,v  <--  regress-358594-05.js
initial revision: 1.1

/cvsroot/mozilla/js/tests/js1_5/extensions/regress-358594-06.js,v  <--  regress-358594-06.js
initial revision: 1.1

Comment 50

[Alexander Sack]

Attachment: roll-up for 1.8.0 (with comments)

updated according to comment #48

this time using:
  cvs diff -u -p -8 jsobj.c jsopcode.c

Comment 51

[Brian Crowder]

Comment on attachment 282402 [details] [diff] [review]
 roll-up for 1.8.0 (with comments)

Igor did the last review, so setting the ? to him again.

Comment 52

[Igor Bukanov]

Comment on attachment 282402 [details] [diff] [review]
 roll-up for 1.8.0 (with comments)

Sorry for a late review, I missed the request 2 months ago.

Comment 53

[Alexander Sack]

Comment on attachment 282402 [details] [diff] [review]
 roll-up for 1.8.0 (with comments)

a=asac for 1.8.0.15

Comment 54

[Reed Loden [:reed]]

MOZILLA_1_8_0_BRANCH:

Checking in js/src/jsobj.c;
/cvsroot/mozilla/js/src/jsobj.c,v  <--  jsobj.c
new revision: 3.208.2.12.2.28; previous revision: 3.208.2.12.2.27
done
Checking in js/src/jsopcode.c;
/cvsroot/mozilla/js/src/jsopcode.c,v  <--  jsopcode.c
new revision: 3.89.2.8.2.12; previous revision: 3.89.2.8.2.11
done