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

VERIFIED FIXED in mozilla1.9alpha5

Status

()

Core
JavaScript Engine
--
critical
VERIFIED FIXED
11 years ago
9 years ago

People

(Reporter: Jesse Ruderman, Assigned: Brian Crowder)

Tracking

(Blocks: 1 bug, 4 keywords)

Trunk
mozilla1.9alpha5
crash, fixed1.8.0.15, testcase, verified1.8.1.5
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.8.1.5 +
wanted1.8.1.x +
blocking1.8.0.next +
wanted1.8.0.x +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:moderate])

Attachments

(11 attachments, 4 obsolete attachments)

7.25 KB, patch
Igor Bukanov
: review+
Details | Diff | Splinter Review
13.75 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
3.94 KB, text/plain
Details
2.40 KB, text/plain
Details
2.16 KB, text/plain
Details
2.40 KB, text/plain
Details
2.16 KB, text/plain
Details
2.41 KB, text/plain
Details
2.16 KB, text/plain
Details
7.45 KB, text/plain
Details
14.04 KB, patch
Igor Bukanov
: review+
Alexander Sack
: approval1.8.0.next+
Details | Diff | Splinter Review
(Reporter)

Description

11 years ago
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#}
(Reporter)

Comment 1

11 years ago
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!)
(Reporter)

Comment 2

10 years ago
This bug is getting in the way of my testing :(
(Reporter)

Updated

10 years ago
Summary: "Assertion failure: vlength > n" calling uneval(this) → "Assertion failure: vlength > n" calling uneval(this) (involves __proto__ and serialization using sharps?)
(Assignee)

Updated

10 years ago
Assignee: general → crowder
(Reporter)

Comment 3

10 years ago
Maybe sharped functions just need to use the "x getter: " syntax rather than the "get x" syntax.
(Reporter)

Updated

10 years ago
Depends on: 356083

Updated

10 years ago
Duplicate of this bug: 379630
(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
(Assignee)

Comment 6

10 years ago
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).
Attachment #264243 - Flags: review?(brendan)
(Assignee)

Updated

10 years ago
Status: NEW → ASSIGNED
(Reporter)

Comment 7

10 years ago
> 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#};
}
(Assignee)

Comment 8

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

Moving review to Igor
Attachment #264243 - Flags: review?(brendan) → review?(igor)

Comment 9

10 years ago
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?
(Assignee)

Comment 10

10 years ago
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

10 years ago
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.
(Assignee)

Comment 12

10 years ago
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.
Attachment #264243 - Attachment is obsolete: true
Attachment #264918 - Flags: review?(igor)
Attachment #264243 - Flags: review?(igor)
(Assignee)

Comment 13

10 years ago
Created attachment 264920 [details] [diff] [review]
implementation v2a

A minor tweak for OLD_GETTER_SETTER users
Attachment #264918 - Attachment is obsolete: true
Attachment #264920 - Flags: review?(igor)
Attachment #264918 - Flags: review?(igor)

Updated

10 years ago
Attachment #264920 - Flags: review?(igor) → review+
(Assignee)

Comment 14

10 years ago
jsobj.c: 3.335
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED

Updated

10 years ago
Target Milestone: --- → mozilla1.9alpha5
(Reporter)

Updated

10 years ago
No longer blocks: 349611
(Reporter)

Updated

10 years ago
Blocks: 349611
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?
OS: Mac OS X → All
Hardware: Macintosh → All
(Assignee)

Comment 16

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

Comment 17

10 years ago
Does it make decisions based on the UMR that might reveal sensitive information in Firefox's address space.
Ah, there might be a privacy problem, then, since this string can be printed and inspected for sensitive information.
(Assignee)

Comment 19

10 years ago
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)
(Assignee)

Comment 20

10 years ago
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?
(Reporter)

Comment 21

10 years ago
I already filed bug 380831 for that.
(Assignee)

Comment 22

10 years ago
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.
Attachment #265857 - Flags: review?(mrbkap)
(Assignee)

Comment 23

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

Which explains why I thought I'd fixed that.

Updated

10 years ago
Attachment #265857 - Flags: review?(mrbkap) → review+
(Assignee)

Comment 24

10 years ago
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.
Attachment #265857 - Flags: approval1.8.1.5?

Comment 25

10 years ago
Created attachment 266144 [details]
comparison of before|after MOZILLA_1_8_BRANCH rollup patch

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

Comment 26

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

Comment 27

10 years ago
Created attachment 266313 [details]
js1_7/regress/regress-358594.js

This doesn't include any of the decompiler tests.

Updated

10 years ago
Flags: in-testsuite+
Flags: wanted1.8.1.x+
Flags: wanted1.8.0.x+
Flags: blocking1.8.1.5?
Whiteboard: [sg:moderate]
(Assignee)

Comment 28

10 years ago
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.
(Assignee)

Comment 29

10 years ago
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

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

Comment 31

10 years ago
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?
(Assignee)

Comment 32

10 years ago
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

10 years ago
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.
Flags: blocking1.8.1.5? → blocking1.8.1.5+

Comment 34

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

js1_7 not required.
Attachment #266313 - Attachment is obsolete: true

Comment 35

10 years ago
Created attachment 268516 [details]
js1_5/extensions/regress-358594-01.js

Comment 36

10 years ago
Created attachment 268517 [details]
js1_5/extensions/regress-358594-02.js

Comment 37

10 years ago
Created attachment 268518 [details]
js1_5/extensions/regress-358594-03.js

Comment 38

10 years ago
Created attachment 268519 [details]
js1_5/extensions/regress-358594-04.js

Comment 39

10 years ago
Created attachment 268520 [details]
js1_5/extensions/regress-358594-05.js

Comment 40

10 years ago
Created attachment 268521 [details]
js1_5/extensions/regress-358594-06.js

Comment 41

10 years ago
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 on attachment 265857 [details] [diff] [review]
MOZILLA_1_8_BRANCH patch, roll-up

approved for 1.8.1.5, a=dveditz for release-drivers
Attachment #265857 - Flags: approval1.8.1.5? → approval1.8.1.5+
(Assignee)

Updated

10 years ago
Blocks: 380933
(Assignee)

Comment 43

10 years ago
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
Keywords: fixed1.8.1.5
bc, could you help verifying this fix on the latest 2.0.0.5 rc builds?

Comment 45

10 years ago
verified fixed 1.8, 1.9.0 windows, linux, macppc with 7/16 opt/debug shell/browser.
Status: RESOLVED → VERIFIED
Keywords: fixed1.8.1.5 → verified1.8.1.5

Comment 46

10 years ago
Created attachment 274461 [details] [diff] [review]
roll-up for 1.8.0

Updated

10 years ago
Attachment #274461 - Flags: approval1.8.0.13?

Updated

10 years ago
Attachment #274461 - Flags: review?(igor)

Comment 47

10 years ago
(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?

Attachment #274461 - Flags: approval1.8.0.13? → approval1.8.0.14?
Group: security

Comment 48

10 years ago
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.
Attachment #274461 - Flags: review?(igor) → review-

Comment 49

10 years ago
/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

10 years ago
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
Attachment #274461 - Attachment is obsolete: true
Attachment #282402 - Flags: review?
Attachment #274461 - Flags: approval1.8.0.14?
(Assignee)

Comment 51

10 years ago
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.
Attachment #282402 - Flags: review? → review?(igor)

Comment 52

10 years ago
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.
Attachment #282402 - Flags: review?(igor) → review+

Updated

9 years ago
Attachment #282402 - Flags: approval1.8.0.15?

Comment 53

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

a=asac for 1.8.0.15
Attachment #282402 - Flags: approval1.8.0.15? → approval1.8.0.15+

Updated

9 years ago
Flags: blocking1.8.0.15+
Keywords: checkin-needed
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
Keywords: checkin-needed → fixed1.8.0.15
You need to log in before you can comment on or make changes to this bug.