Last Comment Bug 358594 - "Assertion failure: vlength > n" calling uneval(this) (involves __proto__ and serialization using sharps?)
: "Assertion failure: vlength > n" calling uneval(this) (involves __proto__ and...
Status: VERIFIED FIXED
[sg:moderate]
: crash, fixed1.8.0.15, testcase, verified1.8.1.5
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- critical (vote)
: mozilla1.9alpha5
Assigned To: Brian Crowder
:
Mentors:
: 379630 (view as bug list)
Depends on: 356083
Blocks: jsfunfuzz 380933
  Show dependency treegraph
 
Reported: 2006-10-29 03:22 PST by Jesse Ruderman
Modified: 2008-03-08 05:41 PST (History)
11 users (show)
dveditz: blocking1.8.1.5+
dveditz: wanted1.8.1.x+
asac: blocking1.8.0.next+
dveditz: wanted1.8.0.x+
bob: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
use new "needOldStyleGetterSetter" logic (3.54 KB, patch)
2007-05-09 09:13 PDT, Brian Crowder
no flags Details | Diff | Review
implementation v2 (7.21 KB, patch)
2007-05-15 14:43 PDT, Brian Crowder
no flags Details | Diff | Review
implementation v2a (7.25 KB, patch)
2007-05-15 14:46 PDT, Brian Crowder
igor: review+
Details | Diff | Review
MOZILLA_1_8_BRANCH patch, roll-up (13.75 KB, patch)
2007-05-23 16:03 PDT, Brian Crowder
mrbkap: review+
dveditz: approval1.8.1.5+
Details | Diff | Review
comparison of before|after MOZILLA_1_8_BRANCH rollup patch (3.94 KB, text/plain)
2007-05-25 17:05 PDT, Bob Clary [:bc:]
no flags Details
js1_7/regress/regress-358594.js (2.40 KB, text/plain)
2007-05-27 18:03 PDT, Bob Clary [:bc:]
no flags Details
js1_5/extensions/regress-358594-01.js (2.40 KB, text/plain)
2007-06-15 11:50 PDT, Bob Clary [:bc:]
no flags Details
js1_5/extensions/regress-358594-02.js (2.16 KB, text/plain)
2007-06-15 11:51 PDT, Bob Clary [:bc:]
no flags Details
js1_5/extensions/regress-358594-03.js (2.40 KB, text/plain)
2007-06-15 11:52 PDT, Bob Clary [:bc:]
no flags Details
js1_5/extensions/regress-358594-04.js (2.16 KB, text/plain)
2007-06-15 11:53 PDT, Bob Clary [:bc:]
no flags Details
js1_5/extensions/regress-358594-05.js (2.41 KB, text/plain)
2007-06-15 11:53 PDT, Bob Clary [:bc:]
no flags Details
js1_5/extensions/regress-358594-06.js (2.16 KB, text/plain)
2007-06-15 11:54 PDT, Bob Clary [:bc:]
no flags Details
difference between 1.8.1 without patch and with patch (7.45 KB, text/plain)
2007-06-23 13:03 PDT, Bob Clary [:bc:]
no flags Details
roll-up for 1.8.0 (14.39 KB, patch)
2007-07-30 06:45 PDT, Alexander Sack
igor: review-
Details | Diff | Review
roll-up for 1.8.0 (with comments) (14.04 KB, patch)
2007-09-26 07:12 PDT, Alexander Sack
igor: review+
asac: approval1.8.0.next+
Details | Diff | Review

Description Jesse Ruderman 2006-10-29 03:22:19 PST
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 2007-01-20 16:43:11 PST
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 2007-02-05 01:18:13 PST
This bug is getting in the way of my testing :(
Comment 3 Jesse Ruderman 2007-04-05 14:04:04 PDT
Maybe sharped functions just need to use the "x getter: " syntax rather than the "get x" syntax.
Comment 4 Igor Bukanov 2007-05-03 11:27:31 PDT
*** Bug 379630 has been marked as a duplicate of this bug. ***
Comment 5 Brendan Eich [:brendan] 2007-05-08 13:10:15 PDT
(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 2007-05-09 09:13:26 PDT
Created attachment 264243 [details] [diff] [review]
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 2007-05-09 10:05:17 PDT
> 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 2007-05-10 10:05:45 PDT
Comment on attachment 264243 [details] [diff] [review]
use new "needOldStyleGetterSetter" logic

Moving review to Igor
Comment 9 Igor Bukanov 2007-05-10 10:20:15 PDT
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 2007-05-10 10:22:56 PDT
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 2007-05-11 08:50:15 PDT
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 2007-05-15 14:43:51 PDT
Created attachment 264918 [details] [diff] [review]
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 2007-05-15 14:46:37 PDT
Created attachment 264920 [details] [diff] [review]
implementation v2a

A minor tweak for OLD_GETTER_SETTER users
Comment 14 Brian Crowder 2007-05-15 15:42:59 PDT
jsobj.c: 3.335
Comment 15 Daniel Veditz [:dveditz] 2007-05-21 12:35:26 PDT
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 2007-05-21 12:37:33 PDT
I think this is a relatively harmless (ie., crash, but nothing more) UMR on the branches.
Comment 17 Jesse Ruderman 2007-05-21 17:02:18 PDT
Does it make decisions based on the UMR that might reveal sensitive information in Firefox's address space.
Comment 18 Blake Kaplan (:mrbkap) (please use needinfo!) 2007-05-21 17:10:07 PDT
Ah, there might be a privacy problem, then, since this string can be printed and inspected for sensitive information.
Comment 19 Brian Crowder 2007-05-23 16:03:00 PDT
Created attachment 265857 [details] [diff] [review]
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 2007-05-23 16:29:22 PDT
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 2007-05-23 16:34:33 PDT
I already filed bug 380831 for that.
Comment 22 Brian Crowder 2007-05-23 16:38:22 PDT
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 2007-05-23 16:42:12 PDT
(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 2007-05-23 16:44:34 PDT
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:] 2007-05-25 17:05:08 PDT
Created attachment 266144 [details]
comparison of before|after MOZILLA_1_8_BRANCH rollup patch

There are some changes in decompilation that aren't cool.
Comment 26 Brian Crowder 2007-05-25 17:54:45 PDT
Thanks, Bob.  I will address these and upload another attempt in a day or so.
Comment 27 Bob Clary [:bc:] 2007-05-27 18:03:11 PDT
Created attachment 266313 [details]
js1_7/regress/regress-358594.js

This doesn't include any of the decompiler tests.
Comment 28 Brian Crowder 2007-06-04 13:25:29 PDT
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 2007-06-05 12:10:50 PDT
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:] 2007-06-05 16:52:02 PDT
I'll get to it this evening on linux at the least.
Comment 31 Bob Clary [:bc:] 2007-06-05 17:52:30 PDT
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 2007-06-05 23:05:21 PDT
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:] 2007-06-08 17:02:43 PDT
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:] 2007-06-15 11:48:47 PDT
Comment on attachment 266313 [details]
js1_7/regress/regress-358594.js

js1_7 not required.
Comment 35 Bob Clary [:bc:] 2007-06-15 11:50:48 PDT
Created attachment 268516 [details]
js1_5/extensions/regress-358594-01.js
Comment 36 Bob Clary [:bc:] 2007-06-15 11:51:40 PDT
Created attachment 268517 [details]
js1_5/extensions/regress-358594-02.js
Comment 37 Bob Clary [:bc:] 2007-06-15 11:52:24 PDT
Created attachment 268518 [details]
js1_5/extensions/regress-358594-03.js
Comment 38 Bob Clary [:bc:] 2007-06-15 11:53:11 PDT
Created attachment 268519 [details]
js1_5/extensions/regress-358594-04.js
Comment 39 Bob Clary [:bc:] 2007-06-15 11:53:53 PDT
Created attachment 268520 [details]
js1_5/extensions/regress-358594-05.js
Comment 40 Bob Clary [:bc:] 2007-06-15 11:54:31 PDT
Created attachment 268521 [details]
js1_5/extensions/regress-358594-06.js
Comment 41 Bob Clary [:bc:] 2007-06-23 13:03:28 PDT
Created attachment 269533 [details]
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] 2007-06-25 10:47:53 PDT
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 2007-07-09 08:55:48 PDT
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] 2007-07-16 11:49:34 PDT
bc, could you help verifying this fix on the latest 2.0.0.5 rc builds?
Comment 45 Bob Clary [:bc:] 2007-07-17 08:52:28 PDT
verified fixed 1.8, 1.9.0 windows, linux, macppc with 7/16 opt/debug shell/browser.
Comment 46 Alexander Sack 2007-07-30 06:45:48 PDT
Created attachment 274461 [details] [diff] [review]
roll-up for 1.8.0
Comment 47 Igor Bukanov 2007-07-31 07:46:06 PDT
(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 2007-09-19 03:01:16 PDT
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:] 2007-09-21 07:40:18 PDT
/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 2007-09-26 07:12:22 PDT
Created attachment 282402 [details] [diff] [review]
 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 2007-09-26 10:08:13 PDT
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 2007-11-28 05:19:14 PST
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 2008-02-28 05:17:45 PST
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] (use needinfo?) 2008-03-08 05:41:48 PST
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

Note You need to log in before you can comment on or make changes to this bug.