Closed Bug 351102 Opened 15 years ago Closed 15 years ago

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

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla1.8.1

People

(Reporter: brendan, Assigned: igor)

References

Details

(Keywords: verified1.8.0.12, verified1.8.1.4, Whiteboard: [sg:critical?])

Attachments

(11 files, 5 obsolete files)

2.44 KB, text/plain
Details
2.52 KB, text/plain
Details
2.45 KB, text/plain
Details
2.34 KB, text/plain
Details
2.34 KB, text/plain
Details
2.35 KB, text/plain
Details
2.53 KB, text/plain
Details
19.49 KB, patch
brendan
: review+
Details | Diff | Splinter Review
19.41 KB, patch
igor
: review+
Details | Diff | Splinter Review
3.55 KB, text/plain
Details
1.27 KB, patch
brendan
: review+
Details | Diff | Splinter Review
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
    }
}
I had to see it. I suspected something was fishy with [throwing], but ignored that :(
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
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...
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?
The code in comment 4 does not crash with WAY_TOO_MUCH_GC!
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
Flags: blocking1.9?
Whiteboard: [sg:critical?]
Assignee: general → igor
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
Status: NEW → ASSIGNED
Attached patch Fix v1 (obsolete) — Splinter Review
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.
Attached patch Fix v2 (obsolete) — Splinter Review
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.
Attachment #258877 - Attachment is obsolete: true
Attached patch Fix v3 (obsolete) — Splinter Review
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.
Attachment #258894 - Attachment is obsolete: true
Attached patch Fix v4 (obsolete) — Splinter Review
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.
Attachment #258898 - Attachment is obsolete: true
Attachment #258901 - Flags: review?(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
Blocks: js1.7src
(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.
Attached patch Fix v4b (obsolete) — Splinter Review
Here is the previous patch with the nits addressed.
Attachment #258901 - Attachment is obsolete: true
Attachment #259064 - Flags: review?(brendan)
Attachment #258901 - Flags: review?(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
Attachment #259064 - Flags: review?(brendan) → review+
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
(In reply to comment #16)
I believe you are referring to js1_5/extensions/regress-350312-0[23].js.
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
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
(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.
Flags: blocking1.8.1.4?
Flags: blocking1.8.0.12?
(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.
Flags: blocking1.8.1.4?
Flags: blocking1.8.1.4+
Flags: blocking1.8.0.12?
Flags: blocking1.8.0.12+
Depends on: 374589
Blocks: 374713
Fixing proper regression dependancies
No longer blocks: 374713
Depends on: 374713
(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.
Flags: in-testsuite+
Attachment #259859 - Attachment is patch: false
(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
Ok, I split that into bug 375695.
Flags: wanted1.8.1.x+
Flags: wanted1.8.0.x+
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.
Attachment #260876 - Flags: review?(brendan)
Attachment #260876 - Flags: approval1.8.1.4?
Whiteboard: [sg:critical?] → [sg:critical?] need r=brendan
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 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
Attachment #260876 - Flags: review?(brendan) → review+
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.

Status: RESOLVED → VERIFIED
This is for references purposes the trubk patch + trunk fixes to simplify compare with versions for branches.
Attachment #259064 - Attachment is obsolete: true
Attachment #261391 - Flags: review+
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.
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
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.
Attachment #261470 - Flags: review?(brendan)
Attachment #261470 - Flags: approval1.8.0.12?
Attachment #261470 - Attachment is patch: true
Attachment #261470 - Attachment mime type: text/x-patch → text/plain
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
Attachment #261470 - Flags: review?(brendan) → review+
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
Attachment #261470 - Flags: approval1.8.0.12? → approval1.8.0.12+
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
Attachment #260876 - Flags: approval1.8.1.4? → approval1.8.1.4+
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
Keywords: fixed1.8.1.4
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
Keywords: fixed1.8.0.12
Whiteboard: [sg:critical?] need r=brendan → [sg:critical?]
verified fixed windows, macppc, linux 1.8.0, 1.8.1 20070417 modulo asserts in bug 375695.
Group: security
/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
/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
You need to log in before you can comment on or make changes to this bug.