351102 - try/catch-guard/finally bytecode still GC-unsafe

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brendan Eich [:brendan]]

The fix for bug 350312, with followup fix from bug 350837, is not enough.  Consider:

function f() {
    try {
        throw new Error('bad');
    } catch (e if (e = null, gc(), false)) {
    } catch (e) {
        // e is dangling now
    }
}

Comment 1

[Igor Bukanov]

I had to see it. I suspected something was fishy with [throwing], but ignored that :(

Comment 2

[Brendan Eich [:brendan]]

Safest way is to stack the exception under the block frame for the catch scope, but it is not compile-time invariant IIRC.  Have to psych myself up to re-read the code...

/be

Comment 3

[Igor Bukanov]

Here is another proof that using cx->exception as an exception storage is not a good idea:

function test()
{
        var a = null;
        try {   
                a();
        } catch (e) {
        }
        return false;
}

try {   
        throw 1;
} catch (e if test()) {
} catch (e if e == 1) {
        print("GOOD");
} catch (e) {
        print("BAD: "+e);
}

This prints currently:

BAD: TypeError: a is not a function

I think Forth got it right when it uses 2 independent stacks, one for expressions and one for control flow...

Comment 4

[Jesse Ruderman]

The fuzzer in bug 349611 found this crash:

js> f = function() { try { d.d.d } catch([] if gc()) { } catch(y) { print(y) } }; f(); f();        
before 9232, after 9232, break 01008000
Bus error

Is that the same bug?

Comment 5

[Jesse Ruderman]

The code in comment 4 does not crash with WAY_TOO_MUCH_GC!

Comment 6

[Jesse Ruderman]

Even simpler:

js> try { @foo } catch([] if gc()) { }
before 36928, after 36928, break 01008000

Thread 0 Crashed:
0   <<00000000>> 	0x00000000 0 + 0
1   js 	0x0004be9c js_ReportUncaughtException + 290 (jsexn.c:1271)
2   js 	0x0001a167 JS_ExecuteScript + 113 (jsapi.c:4214)
3   js 	0x000029d2 Process + 912 (js.c:268)
4   js 	0x00003354 ProcessArgs + 1910 (js.c:494)
5   js 	0x00007efd main + 612 (js.c:3159)
6   js 	0x000024e6 _start + 216
7   js 	0x0000240d start + 41

js> try { d.d.d } catch([] if gc()) { }
before 27696, after 27696, break 01008000
Assertion failure: 3 < (obj)->map->freeslot, at jsexn.c:370

Comment 7

[Igor Bukanov]

One do not even need destructing assignmnet to trigger the bug:

js> try { null.a } catch(e if (e = null, gc())) { } 
before 27696, after 27696, break 08a77000
Assertion failure: 3 < (obj)->map->freeslot, at jsexn.c:370

Comment 8

[Igor Bukanov]

Attachment: Fix v1

This is an untested patch to show the main idea behind the fix.

As the test cases for this bug demonstrates, one can not use cx->exception as a temporary storage for the pending exception while executing the destructing assignment/checking the guards for 2 reasons:

1. This is GC unsafe since cx->exception is no longer marked as throwing and will be GC-ed if the guard can unroot it.

2. The destructing assignment through element getters or the guard can run arbitrary code that can throw and catch other exceptions overriding the original cx->exception. Here is an example demonstrating that:

~/m/trunk/mozilla/js/src $ cat ~/s/y.js
var obj = { get a() {
	try {
		throw 1;
	} catch (e) {
	}
	return false;
}};

try {
   throw obj;
} catch ({a: a} if a) {
   throw "Unreachable";
} catch (e) {
   if (e !== obj)
     throw "Unexpected exception: "+uneval(e);
}
~/m/trunk/mozilla/js/src $ ./Linux_All_DBG.OBJ/js ~/s/y.js
uncaught exception: Unexpected exception: 1

To fix this the patch adds an extra dup/pop code for any catch with the guard to preserve the original exception. That required to change jsop_throwing to pop the exception and store it in cx->exception.

With this gosub/retsub no longer needs to push/restore cx->throwing state and rethrowing after the failed guard is simplified.

Comment 9

[Igor Bukanov]

Attachment: Fix v2

Compared with the previous version, this fixes the decompiler to ignore the extra [dup] for the guard, simplify the rethrowing after the guards code just to [throw] and bumps the xdr version as the patch changes the semantic of the bytecode.

Comment 10

[Igor Bukanov]

Attachment: Fix v3

In the new version I restored pushing/popping 2 stack slots in [gosub]/[retsub] to  ensure that finally handler begins with a consistent stack budget. Without it any [setsp] executed inside finally that is invoked during normal non-throwing execution will create a hole on the stack and bring into the root set a potentially already GC-ed value.

Comment 11

[Igor Bukanov]

Attachment: Fix v4

The new version cleanups [exception] decompilation as that bytecode is only generated as a part of catch code.
 
The test suite shows no regression with the patch.

Comment 12

[Brendan Eich [:brendan]]

Comment on attachment 258901 [details] [diff] [review]
Fix v4

>? throwing
>Index: jsemit.c
>===================================================================
>RCS file: /cvsroot/mozilla/js/src/jsemit.c,v
>retrieving revision 3.240
>diff -U7 -p -r3.240 jsemit.c
>--- jsemit.c	11 Mar 2007 19:25:58 -0000	3.240
>+++ jsemit.c	17 Mar 2007 13:39:11 -0000
>@@ -4830,20 +4830,23 @@ js_EmitTree(JSContext *cx, JSCodeGenerat
>             jsint count = 0;    /* previous catch block's population */
> 
>             catchStart = end;
> 
>             /*
>              * The emitted code for a catch block looks like:
>              *
>+             * [ trowing ]                         only if 2nd+ catch block

Typo: throwing

>+             * Rethrow the exception delegating executing of finally if any
>+             * to the exception handler.

Comma before "delegating"

>+             * We emit a [setsp][gosub] sequence for running finally code
>+             * while letting an uncaught exception pass thrown from within
>+             * the try in a try-finally.  The [gosub] and [retsub] opcodes
>+             * will take care of stacking and rethrowing any exception pending

The "pass thrown" adjacent words seem ungrammatical or just hard to read. Reword?

>+++ jsgc.c	17 Mar 2007 13:39:12 -0000
>@@ -2960,16 +2960,21 @@ restart:
>         /* Cleanup temporary "dormant" linkage. */
>         if (acx->fp)
>             acx->fp->dormantNext = NULL;
> 
>         /* Mark other roots-by-definition in acx. */
>         GC_MARK(cx, acx->globalObject, "global object");
>         MarkWeakRoots(cx, &acx->weakRoots);
>-        if (acx->throwing && JSVAL_IS_GCTHING(acx->exception))
>-            GC_MARK(cx, JSVAL_TO_GCTHING(acx->exception), "exception");
>+        if (acx->throwing) {
>+            if (JSVAL_IS_GCTHING(acx->exception))
>+                GC_MARK(cx, JSVAL_TO_GCTHING(acx->exception), "exception");
>+        } else {
>+            /* Avoid leaving GC-ed junk in the exception object. */
>+            acx->exception = JSVAL_NULL;

Nit: "exception value" not "exception object".

>+++ jsinterp.c	17 Mar 2007 13:39:14 -0000
>@@ -5360,15 +5360,20 @@ interrupt:
>             i = PTRDIFF(pc, script->main, jsbytecode) + JSOP_GOSUB_LENGTH;
>             len = GET_JUMP_OFFSET(pc);
>             PUSH(INT_TO_JSVAL(i));
>           END_VARLEN_CASE
> 
>           BEGIN_CASE(JSOP_GOSUBX)
>             JS_ASSERT(cx->exception != JSVAL_HOLE);
>-            lval = cx->throwing ? cx->exception : JSVAL_HOLE;
>+            if (!cx->throwing) {
>+                lval = JSVAL_HOLE;
>+            } else {
>+                lval = cx->exception;
>+                cx->throwing = JS_FALSE;
>+            }
>             PUSH(lval);
>             i = PTRDIFF(pc, script->main, jsbytecode) + JSOP_GOSUBX_LENGTH;
>             len = GET_JUMPX_OFFSET(pc);
>             PUSH(INT_TO_JSVAL(i));
>           END_VARLEN_CASE

Where's the corresponding change for JSOP_GOSUB?

>               case JSOP_EXCEPTION:
>-                /*
>-                 * The only other JSOP_EXCEPTION cases occur as part of a code
>-                 * sequence that follows a SRC_CATCH-annotated JSOP_ENTERBLOCK
>-                 * or that precedes a SRC_HIDDEN-annotated JSOP_POP emitted
>-                 * when returning from within a finally clause.
>-                 */
>-                sn = js_GetSrcNote(jp->script, pc);
>-                LOCAL_ASSERT(sn && SN_TYPE(sn) == SRC_HIDDEN);
>-                todo = -2;
>+                /* The catch decompiler handles it itself. */
>+                LOCAL_ASSERT(JS_FALSE);

Nit: "handles this op itself."

>@@ -2381,15 +2374,25 @@ Decompile(SprintStack *ss, jsbytecode *p
>                     jp->indent -= 4;
>                     js_printf(CLEAR_MAYBE_BRACE(jp), "\t} catch (");
> 
>                     pc2 = pc;
>                     pc += oplen;
>                     LOCAL_ASSERT(*pc == JSOP_EXCEPTION);
>                     pc += JSOP_EXCEPTION_LENGTH;
>-
>+                    if (*pc == JSOP_DUP) {
>+                        sn2 = js_GetSrcNote(jp->script, pc);
>+                        if (sn2) {
>+                            /*
>+                             * dup pushing the exception object to use after

Nit: "A dup that pushes the exception object ...."

/be

Comment 13

[Igor Bukanov]

(In reply to comment #12)
> Where's the corresponding change for JSOP_GOSUB?

The current code in the interpreter contains:

          BEGIN_CASE(JSOP_GOSUB)
            JS_ASSERT(cx->exception != JSVAL_HOLE);
            if (!cx->throwing) {
                lval = JSVAL_HOLE;
            } else {
                lval = cx->exception;
                cx->throwing = JS_FALSE;
            }
            PUSH(lval);
            i = PTRDIFF(pc, script->main, jsbytecode) + JSOP_GOSUB_LENGTH;
            len = GET_JUMP_OFFSET(pc);
            PUSH(INT_TO_JSVAL(i));
          END_VARLEN_CASE

          BEGIN_CASE(JSOP_GOSUBX)
            JS_ASSERT(cx->exception != JSVAL_HOLE);
            lval = cx->throwing ? cx->exception : JSVAL_HOLE;
            PUSH(lval);
            i = PTRDIFF(pc, script->main, jsbytecode) + JSOP_GOSUBX_LENGTH;
            len = GET_JUMPX_OFFSET(pc);
            PUSH(INT_TO_JSVAL(i));
          END_VARLEN_CASE

That is, compared with JSOP_GOSUB, its X-version does not clear cx->throwing flag. The patch just fixes that.

Comment 14

[Igor Bukanov]

Attachment: Fix v4b

Here is the previous patch with the nits addressed.

Comment 15

[Brendan Eich [:brendan]]

Comment on attachment 259064 [details] [diff] [review]
Fix v4b

Ah, I should have looked. Didn't think we missed that back in the rush to patch the branch, but we did.

r=me, thanks.

/be

Comment 16

[Brendan Eich [:brendan]]

Comment on attachment 259064 [details] [diff] [review]
Fix v4b

I recall writing a testcase generator -- does it have complete coverage? If so, this patch or a backported version of it should go into the next branch releases.

/be

Comment 17

[Bob Clary [:bc] (inactive)]

(In reply to comment #16)
I believe you are referring to js1_5/extensions/regress-350312-0[23].js.

Comment 18

[Igor Bukanov]

I committed the patch from comment 14 to the trunk:

Checking in jsemit.c;
/cvsroot/mozilla/js/src/jsemit.c,v  <--  jsemit.c
new revision: 3.242; previous revision: 3.241
done
Checking in jsgc.c;
/cvsroot/mozilla/js/src/jsgc.c,v  <--  jsgc.c
new revision: 3.207; previous revision: 3.206
done
Checking in jsinterp.c;
/cvsroot/mozilla/js/src/jsinterp.c,v  <--  jsinterp.c
new revision: 3.341; previous revision: 3.340
done
Checking in jsopcode.c;
/cvsroot/mozilla/js/src/jsopcode.c,v  <--  jsopcode.c
new revision: 3.218; previous revision: 3.217
done
Checking in jsopcode.tbl;
/cvsroot/mozilla/js/src/jsopcode.tbl,v  <--  jsopcode.tbl
new revision: 3.90; previous revision: 3.89
done
Checking in jsxdrapi.h;
/cvsroot/mozilla/js/src/jsxdrapi.h,v  <--  jsxdrapi.h
new revision: 1.28; previous revision: 1.27
done

Comment 19

[Igor Bukanov]

(In reply to comment #16)
> (From update of attachment 259064 [details] [diff] [review])
> I recall writing a testcase generator -- does it have complete coverage? If so,
> this patch or a backported version of it should go into the next branch
> releases.

If a backport for 1.8.0 branch would require too much efforts, I suggest just apply the jsgc.c part of the patch there which replaces cx->exception with JSVAL_NULL after GC if not throwing. This is enough to ensure GC-safty.

Comment 20

[Igor Bukanov]

(In reply to comment #17)
> I believe you are referring to js1_5/extensions/regress-350312-0[23].js.
> 

Note that definitely does not include the test case from comment 8.

Comment 21

[Igor Bukanov]

Fixing proper regression dependancies

Comment 22

[Bob Clary [:bc] (inactive)]

(In reply to comment #6)

try { try { @foo } catch([] if gc()) { } } catch(ex) {}
try { try { d.d.d } catch([] if gc()) { } } catch(ex) {}

each give (with or without setting version(170) btw)

Assertion failure: !fp->blockChain || OBJ_GET_PARENT(cx, obj) == fp->blockChain, at jsinterp.c:5837

which is "somewhat" similar to Bug 361566 and Bug 355832 except it doesn't require js1_7. Note the unwrapped original versions no longer assert though.

Comment 23

[Bob Clary [:bc] (inactive)]

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

Comment 24

[Bob Clary [:bc] (inactive)]

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

Comment 25

[Bob Clary [:bc] (inactive)]

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

Comment 26

[Bob Clary [:bc] (inactive)]

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

Comment 27

[Bob Clary [:bc] (inactive)]

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

Comment 28

[Bob Clary [:bc] (inactive)]

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

Comment 29

[Bob Clary [:bc] (inactive)]

Attachment: js1_5/extensions/regress-351102-07.js

Comment 30

[Igor Bukanov]

(In reply to comment #22)
> (In reply to comment #6)
> 
> try { try { @foo } catch([] if gc()) { } } catch(ex) {}
> try { try { d.d.d } catch([] if gc()) { } } catch(ex) {}
> 
> each give (with or without setting version(170) btw)
> 
> Assertion failure: !fp->blockChain || OBJ_GET_PARENT(cx, obj) ==
> fp->blockChain, at jsinterp.c:5837

This is unrelated to this GC-safty bug. Just replacing gc() with false gives the same error:
try { try { throw 1 } catch([] if false) { } } catch(ex) {}

Assertion failure: !fp->blockChain || OBJ_GET_PARENT(cx, obj) == fp->blockChain, at jsinterp.c:5853

Comment 31

[Jesse Ruderman]

Ok, I split that into bug 375695.

Comment 32

[Igor Bukanov]

Attachment: Fix for 1.8 branch including the regression fixes

The patch and the follow up regression fixes from bug 351102 applies to 1.8 branch as is with the exception of changes to bump xdrversion. For the branch I set to 10 to ensure the uniqueness as the patch induces format changes that does not correspond to anything on the trunk.

Comment 33

[Igor Bukanov]

Pinging: given the fast approaching freeze for the next releases I would like to get r=something for 1.8.1 patch to base 1.8.0 version on it.

Comment 34

[Brendan Eich [:brendan]]

Comment on attachment 260876 [details] [diff] [review]
Fix for 1.8 branch including the regression fixes

Sorry, lost track of the request -- looks right (branch vs. trunk diff with this applied would be helpful to confirm).

/be

Comment 35

[Bob Clary [:bc] (inactive)]

I've moved the -03,-04,-05,-07 tests to js1_7/extensions since they cause syntax errors in 1.8.0. -04, -05 still show the assert in bug 375695. verifying on trunk though.

Comment 36

[Igor Bukanov]

Attachment: Fix v4b + regression fixes

This is for references purposes the trubk patch + trunk fixes to simplify compare with versions for branches.

Comment 37

[Igor Bukanov]

Attachment: diff between trunk and 1.8 versions of the patch

I did not include this diff initially as patch applied cleanly with the exception of XDR version code. But for the references here is the diff.

Now I need to backporty to 1.8.0 where the real differences lives.

Comment 38

[Brendan Eich [:brendan]]

Thanks -- looking fwd to the 1.8.0 diff of diffs -- one trick I use is to grep out context jitter (lines without + or - in the input diff).

/be

Comment 39

[Igor Bukanov]

Attachment: just fixing GC hazard for 1.8.0

Given that just a single hunk from trunk/1.8.1 patches applies to 1.8.0 and a prototype re-implementation of the patch on 1.8.0 is rather non-trivial and non-minimal, I suggest to just fix just GC-hazard for 1.8.0 via nulling cx->exception after GC.

Comment 40

[Brendan Eich [:brendan]]

Comment on attachment 261470 [details] [diff] [review]
just fixing GC hazard for 1.8.0

Good idea -- we are almost at end of life for 1.8.0 so r=me.

/be

Comment 41

[Daniel Veditz [:dveditz]]

Comment on attachment 261470 [details] [diff] [review]
just fixing GC hazard for 1.8.0

approved for 1.8.0.12, a=dveditz for release-drivers

Comment 42

[Daniel Veditz [:dveditz]]

Comment on attachment 260876 [details] [diff] [review]
Fix for 1.8 branch including the regression fixes

approved for 1.8.1.4, a=dveditz for release-drivers

Comment 43

[Igor Bukanov]

I committed the patch from comment 32 to MOZILLA_1_8_BRANCH:

Checking in jsemit.c;
/cvsroot/mozilla/js/src/jsemit.c,v  <--  jsemit.c
new revision: 3.128.2.71; previous revision: 3.128.2.70
done
Checking in jsgc.c;
/cvsroot/mozilla/js/src/jsgc.c,v  <--  jsgc.c
new revision: 3.104.2.33; previous revision: 3.104.2.32
done
Checking in jsinterp.c;
/cvsroot/mozilla/js/src/jsinterp.c,v  <--  jsinterp.c
new revision: 3.181.2.88; previous revision: 3.181.2.87
done
Checking in jsopcode.c;
/cvsroot/mozilla/js/src/jsopcode.c,v  <--  jsopcode.c
new revision: 3.89.2.71; previous revision: 3.89.2.70
done
Checking in jsopcode.tbl;
/cvsroot/mozilla/js/src/jsopcode.tbl,v  <--  jsopcode.tbl
new revision: 3.43.2.22; previous revision: 3.43.2.21
done
Checking in jsxdrapi.h;
/cvsroot/mozilla/js/src/jsxdrapi.h,v  <--  jsxdrapi.h
new revision: 1.14.58.11; previous revision: 1.14.58.10
done

Comment 44

[Igor Bukanov]

I committed the patch from comment 39 to MOZILLA_!_8_0_BRANCH:

Checking in jsgc.c;
/cvsroot/mozilla/js/src/jsgc.c,v  <--  jsgc.c
new revision: 3.104.2.3.2.19; previous revision: 3.104.2.3.2.18
done

Comment 45

[Bob Clary [:bc] (inactive)]

verified fixed windows, macppc, linux 1.8.0, 1.8.1 20070417 modulo asserts in bug 375695.

Comment 46

[Bob Clary [:bc] (inactive)]

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

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

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

Comment 47

[Bob Clary [:bc] (inactive)]

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

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

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

/cvsroot/mozilla/js/tests/js1_7/extensions/regress-351102-07.js,v  <--  regress-351102-07.js
initial revision: 1.1
done