Closed Bug 389605 Opened 14 years ago Closed 14 years ago

Assertion with empty-destructuring-let and try..catch

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set
critical

Tracking

()

VERIFIED FIXED

People

(Reporter: jruderman, Assigned: igor)

References

Details

(Keywords: assertion, testcase)

Attachments

(2 files, 4 obsolete files)

js> let ([] = function() { }) { try { throw 1; } catch(x) { } }
Assertion failure: OBJ_GET_CLASS(cx, obj)->flags & JSCLASS_HAS_PRIVATE, at jsapi.c:2777


Variations trigger other assertions:

Assertion failure: OBJ_GET_CLASS(cx, obj) == &js_BlockClass, at jsinterp.c:5948

Assertion failure: (3 + 1) < (obj)->map->freeslot, at jsobj.c:1986


Doesn't crash opt.
Triggers a different assertion now:

js> let ([] = function() { }) { try { throw 1; } catch(x) { } }
Assertion failure: (size_t) (blocksp - fp->spbase) <= script->depth, at jsinterp.c:6515

Still doesn't crash opt.
Assignee: general → igor
The reason for the bug is that let ([] = xxx) introduces a let block with 0 depth yet the stack unwind code relies on the stack depth to unwind the scope chain.

As such the interpreter, when transferring the control to the catch block would skip the let block scope and sets the scope chain to the call object for the function. Yet JSOP_LEAVESCOPE would still be executed and tries to unwind the already put object.
Group: security
Flags: blocking1.9?
One way to fix this is to disable empty destructuring patterns so the bug never happens. But this non-compatible and is no-go, right?

Another way is to add an artificial property like "" to such let blocks so OBJ_BLOCK_COUNT is always non-zero.
Yet another possibility for a fix is to record the depth of scope/block chains into the try note and to unwind the scope precisely to that depth. It should make the unwinding much more robust. 
Yes, we need to allow empty destructuring patterns per ES4 as proposed. Recording the depth sounds best, robustness trumps minimizing space overhead.

/be
Brendan, Igor, should this block 1.9?
This looks like a null-ptr dereference, so I would normally suggest not blocking.  I think this bug (and Igor's proposed fix, specifically) might represent a -class- of similar bugs, though, so it may be worth blocking on...
-'ing based on Comment #7.
Flags: blocking1.9? → blocking1.9-
Attached patch v1 (obsolete) — Splinter Review
This is an untested patch that just makes sure that the let block always adds at least one slot to the stack.

The alternative approach of recording scope chain depth in try notes requires much more code and would be more difficult to backport to 1.8.
Attached patch v2 (obsolete) — Splinter Review
Testing v1 has shown that adding a synthetic property to the let block at the emit time is too late. Any property that contributes to the OBJ_BLOCK_COUNT must be added right after the code parsed the let variables to prevent pre-synthetic OBJ_BLOCK_COUNT entering calculations.

This is what that v2 does. I also removed changes to jsopcode.c as they are not as the decompiler ignores any synthetic let property not referenced by the bytecode.
Attachment #314091 - Attachment is obsolete: true
Attachment #314151 - Flags: review?(brendan)
Comment on attachment 314151 [details] [diff] [review]
v2

>+            JS_ASSERT(vp > regs.sp);
>             JS_ASSERT(vp <= fp->spbase + script->depth);

The added assertion reads slightly better to me if reordered to regs.sp < vp -- number line order, and it matches:

>             while (regs.sp < vp) {
>                 STORE_OPND(0, JSVAL_VOID);
>                 regs.sp++;
>             }
[snip]
>+#if JS_HAS_DESTRUCTURING
>+/*
>+ * The catch/finally handler implementation in the interpreter assumes that
>+ * any operation that introduces a new scope (like a "let" or "with" block)
>+ * increases the stack depth. This way it is possible to restore the scope

Comma after "This way".

>+ * chain based on stack depth of the handler alone. A let block with an empty
>+ * destructuring pattern like in

s/like in/such as/

>+ *
>+ *   let [] = 1;
>+ *
>+ * would violate it as the there would be no let locals to store on the stack.

s/it /this assumption, /

>+ * To workaround it we add an empty property to such blocks so

s/workaround it/satisfy it/

>+ * OBJ_BLOCK_COUNT(cx, blockObj), that gives the number of slots, would be
>+ * always positive.
>+ */
>+static JSBool
>+MakeNonEmptyLet(JSContext *cx, JSTreeContext *tc)

s/Make/Ensure/g

because of this test at the top of the function:

>+{
>+    if (0 != OBJ_BLOCK_COUNT(cx, tc->blockChain))

Nit: generally we avoid constant on the left for != and ==.

>+        return JS_TRUE;
>+
>+    return js_DefineNativeProperty(
>+               cx, tc->blockChain,
>+               ATOM_TO_JSID(cx->runtime->atomState.emptyAtom),
>+               JSVAL_VOID, NULL, NULL, JSPROP_PERMANENT | JSPROP_READONLY,
>+               SPROP_HAS_SHORTID, 0, NULL);

Neat trick. Could it be indented more conventionally?

    return js_DefineNativeProperty(cx, tc->blockChain,
                                   ATOM_TO_JSID(cx->runtime->atomState
                                                .emptyAtom),
                                   JSVAL_VOID, NULL, NULL,
                                   JSPROP_PERMANENT | JSPROP_READONLY,
                                   SPROP_HAS_SHORTID, 0, NULL);

Thanks for fixing this. The first time 'round, I had to find a different way of handling the empty destructuring special case, after missing it when implementing for js1.7 from a draft es4 proposal.

/be
Attachment #314151 - Flags: review?(brendan) → review+
Attached patch v3 (obsolete) — Splinter Review
The previous version of the patch adds an empty synthetic property when putting the let blocks. v3 fixes this and addresses the nits. Here is a delta from v2:

--- /home/igor/m/ff/mozilla/quilt.LBI6Dv/js/src/jsinterp.c	2008-04-08 16:54:37.000000000 +0200
+++ js/src/jsinterp.c	2008-04-08 09:33:54.000000000 +0200
@@ -6485,3 +6485,3 @@ interrupt:
             vp = regs.sp + OBJ_BLOCK_COUNT(cx, obj);
-            JS_ASSERT(vp > regs.sp);
+            JS_ASSERT(regs.sp < vp);
             JS_ASSERT(vp <= fp->spbase + script->depth);
--- /home/igor/m/ff/mozilla/quilt.LBI6Dv/js/src/jsparse.c	2008-04-08 16:54:37.000000000 +0200
+++ js/src/jsparse.c	2008-04-08 09:42:42.000000000 +0200
@@ -1664,3 +1664,3 @@ BindLet(JSContext *cx, BindData *data, J
  * any operation that introduces a new scope (like a "let" or "with" block)
- * increases the stack depth. This way it is possible to restore the scope
+ * increases the stack depth. This way, it is possible to restore the scope
  * chain based on stack depth of the handler alone. A let block with an empty
@@ -1670,4 +1670,4 @@ BindLet(JSContext *cx, BindData *data, J
  *
- * would violate it as the there would be no let locals to store on the stack.
- * To workaround it we add an empty property to such blocks so
+ * would violate this assumption as the there would be no let locals to store
+ * on the stack. To satisfy it we add an empty property to such blocks so
  * OBJ_BLOCK_COUNT(cx, blockObj), that gives the number of slots, would be
@@ -1676,4 +1676,6 @@ BindLet(JSContext *cx, BindData *data, J
 static JSBool
-MakeNonEmptyLet(JSContext *cx, JSTreeContext *tc)
+EnsureNonEmptyLet(JSContext *cx, JSTreeContext *tc)
 {
+    jsid id;
+    
     if (0 != OBJ_BLOCK_COUNT(cx, tc->blockChain))
@@ -1681,7 +1683,7 @@ MakeNonEmptyLet(JSContext *cx, JSTreeCon
 
-    return js_DefineNativeProperty(
-               cx, tc->blockChain,
-               ATOM_TO_JSID(cx->runtime->atomState.emptyAtom),
-               JSVAL_VOID, NULL, NULL, JSPROP_PERMANENT | JSPROP_READONLY,
-               SPROP_HAS_SHORTID, 0, NULL);
+    id = ATOM_TO_JSID(cx->runtime->atomState.emptyAtom);
+    return js_DefineNativeProperty(cx, tc->blockChain, id,
+                                   JSVAL_VOID, NULL, NULL,
+                                   JSPROP_PERMANENT | JSPROP_READONLY,
+                                   SPROP_HAS_SHORTID, 0, NULL);
 }
@@ -3002,3 +3004,3 @@ Statement(JSContext *cx, JSTokenStream *
                     pn3 = DestructuringExpr(cx, &data, tc, tt);
-                    if (!pn3 || !MakeNonEmptyLet(cx, tc))
+                    if (!pn3 || !EnsureNonEmptyLet(cx, tc))
                         return NULL;
@@ -3620,3 +3622,3 @@ Variables(JSContext *cx, JSTokenStream *
 #if JS_HAS_DESTRUCTURING
-    if (let && !MakeNonEmptyLet(cx, tc))
+    if (let && !EnsureNonEmptyLet(cx, tc))
         return NULL;
--- /home/igor/m/ff/mozilla/quilt.LBI6Dv/js/src/jsobj.c	2008-04-03 00:26:14.000000000 +0200
+++ js/src/jsobj.c	2008-04-08 09:42:32.000000000 +0200
@@ -1976,2 +1976,8 @@ js_PutBlockObject(JSContext *cx, JSBool 
                 continue;
+            if (sprop->id == ATOM_TO_JSID(cx->runtime->atomState.emptyAtom)) {
+                /* See comments before EnsureNonEmptyLet from jsparse.c. */
+                JS_ASSERT(OBJ_BLOCK_COUNT(cx, obj) == 1);
+                JS_ASSERT(sprop->shortid == 0);
+                continue;
+            }
             slot = depth + (uintN) sprop->shortid;
Attachment #314151 - Attachment is obsolete: true
Attachment #314340 - Flags: review?(brendan)
Comment on attachment 314340 [details] [diff] [review]
v3

>+    if (0 != OBJ_BLOCK_COUNT(cx, tc->blockChain))

The 0 != expr form still goes against prevailing style and makes me wince (memories of Windows(tm) programmers phearing assignment operator by typo, but in this case the right operand is not an lvalue).

r=me again, esp. if you can stand to pick this nit.

We should take this for 1.9 to avoid regressing from JS1.7 / Firefox 2.

/be
Attachment #314340 - Flags: review?(brendan)
Attachment #314340 - Flags: review+
Attachment #314340 - Flags: approval1.9?
Attached patch v3bSplinter Review
The new version of the patch addresses the last nit:

@@ -1680,3 +1680,3 @@ EnsureNonEmptyLet(JSContext *cx, JSTreeC
 
-    if (0 != OBJ_BLOCK_COUNT(cx, tc->blockChain))
+    if (OBJ_BLOCK_COUNT(cx, tc->blockChain) != 0)
         return JS_TRUE;
Attachment #314340 - Attachment is obsolete: true
Attachment #314702 - Flags: review+
Attachment #314702 - Flags: approval1.9?
Attachment #314340 - Flags: approval1.9?
Blocks: 427798
Comment on attachment 314702 [details] [diff] [review]
v3b

a1.9=beltzner
Attachment #314702 - Flags: approval1.9? → approval1.9+
I checked in the patch from the comment 14 to the trunk:

http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&branch=HEAD&cvsroot=%2Fcvsroot&date=explicit&mindate=1207842567&maxdate=1207842660&who=igor%25mir2.org
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Depends on: 428706
Depends on: 428708
Flags: in-testsuite+
Flags: in-litmus-
v 1.9.0
Status: RESOLVED → VERIFIED
Attached file js1_7/regress/regress-389605.js (obsolete) —
ignore syntax error on trunk due to bug 418051 fix.
Attachment #316405 - Attachment is obsolete: true
Attachment #316405 - Attachment is obsolete: false
Attachment #336387 - Attachment is obsolete: true
when this bug is opened, the test should be checked in.
Flags: in-testsuite+ → in-testsuite?
Group: core-security
You need to log in before you can comment on or make changes to this bug.