350312 - Accessing wrong stack slot with nested catch/finally

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Igor Bukanov]

Consider the following example for js shell:

var finally1, ex;

function gen()
{
        try {
                try {
                        throw 1;
                } catch (e) {
                        yield 1;
                } finally {
                        finally1 = true;
                }
        } catch (e) {
                ex = e;
        }
}

finally1 = false;
var iter = gen();
iter.next();
try {
        iter.throw(1);
} catch (e if e instanceof StopIteration) {
}

var passed = finally1 && ex === 1;
if (!passed) {
        print("Failed!");
        print("finally1="+finally1);
        print("ex="+uneval(ex));
}

Currently the shell prints when executing the example:

Failed!
finally1=true
ex=67853518

where ex=67853518 comes from the wrong value poped from the stack when finishing execution of inner finally.

Comment 1

[Igor Bukanov]

The bug has nothing to do with generators, good-old-nested try-catch is enough as the example bellow demonstrates:

var tmp;

function f()
{
        try {   
                try {   
                        throw 1;
                } catch (e) {
                        throw e;
                } finally {
                        tmp = true;
                }
        } catch (e) {
                return e;
        }
}

var ex = f();

var passed = ex === 1;
if (!passed) {
        print("Failed!");
        print("ex="+uneval(ex));
}

Currently it prints:
Failed!
ex=67853478

Comment 2

[Brendan Eich [:brendan]]

Regression from patch for bug 346494.  Patches for trunk and both 1.8x branches coming up fast.

/be

Comment 3

[Brendan Eich [:brendan]]

Attachment: trunk patch

Really, anyone among mrbkap, shaver, and I can vouch for this.  It's a conservative fix in that it budgets more than enough stack space for case 3 as mis-stated in the pre-patch revisions.  This will never cause a memory problem, and as Igor's test shows, it is necessary.  See the corrected case analysis in the comment for why.

Jesse, is your fuzzer generating throws in catches and finallys?

/be

Comment 4

[Brendan Eich [:brendan]]

Attachment: 1.8 branch patch

Comment 5

[Brendan Eich [:brendan]]

Attachment: 1.8.0 branch patch

This should go in today for 1.8.0.7.

/be

Comment 6

[Brendan Eich [:brendan]]

With the code generator's model stack too shallow, the final maxStackDepth was one too low, which led to a generating pc clobbering the saved exception that was on the stack in the first slot (it was explicitly thrown by the catch block, then stacked by JSOP_EXCEPTION as part of the rethrow sequence).

Thanks to Igor for finding this!

/be

Comment 7

[Mike Shaver (:shaver emeritus)]

Comment on attachment 235596 [details] [diff] [review]
trunk patch

r=shaver

Let's get this into the suite, too.

Comment 8

[Mike Shaver (:shaver emeritus)]

Comment on attachment 235597 [details] [diff] [review]
1.8 branch patch

r=shaver, including the error-checking ride-along. :)

Comment 9

[Mike Shaver (:shaver emeritus)]

Comment on attachment 235598 [details] [diff] [review]
1.8.0 branch patch

r=shaver, it's good for 1.8.0.7.

Comment 10

[Brendan Eich [:brendan]]

Fixed on trunk.

/be

Comment 11

[Mike Schroepfer]

Comment on attachment 235597 [details] [diff] [review]
1.8 branch patch

a=schrep/beltnzer for drivers.

Comment 12

[Daniel Veditz [:dveditz]]

Comment on attachment 235598 [details] [diff] [review]
1.8.0 branch patch

a=dveditz for 1.8.0

Comment 13

[Brendan Eich [:brendan]]

The 1.8.0 branch patch is good for 1.8.0.7 and does not expose a further bug, which lexically scoped catch variables (trunk, and soon on 1.8) do expose:

js> function f(x,y){try{throw x}catch(e){if(y)throw e}finally{try{throw 42}catch(e2){print(e2)}}}
js> f(2, 1)
42
uncaught exception: 2
js> f(2, 0)
42
42
42
42
etc. ad infinitum

The code generator cannot know the stack depth for the finally subroutine.  In the f(2, 0) case, the outer catch does not throw, so the finally runs at depth 1, not 2 -- but the code generator models the finally as starting at depth 2, so it emits a JSOP_GETLOCAL for e2 that has the wrong depth, and it JSOP_RETSUBs using the int-tagged exception value 42 off the stack, which lands on what looks like a JSOP_NOP (0) after a prior JSOP_SETSP, and starts ilooping.

Followup trunk patch and revised, full 1.8 branch patch, soon (I hope).

/be

Comment 14

[Brendan Eich [:brendan]]

Attachment: new approach, 1.8.0 branch patch

The stack depth at which a finally block starts must be an invariant that the code generator can compute.  Combine this with the fact that started all the revisiting of this seven+ year old code: a finally block may run while an uncaught exception is pending (try-finally, or try-catch(guard)-finally with no unguarded catch before the finally).  Therefore, the finally must be invoked with a bytecode that saves any pending exception, and its final return-from-finally opcode must throw that saved exception, if any.

This patch implements the above as directly as possible.  JSOP_GOSUB and JSOP_RETSUB now save and restore, respectively, exception state.  They therefore push and pop, respectively, two slots.  To compress cx->throwing and cx->exception the patch uses JSVAL_HOLE to mean !cx->throwing, and all other jsvals to mean cx->throwing with the jsval telling the value of the thrown exception.

I finally bit the bullet and removed the wrong stack imbalance (nuses and ndefs not both 0) for these opcodes.  This relieves the straight-line stack modeling code in the code generator from having to restore depth to cg->stackDepth.  It also removes JSOP_BACKPATCH_PUSH.

The try-catch(guard)-finally case requires a new opcode, JSOP_THROWING, because catch guards run in series, each picking up the pending exception *and clearing cx->throwing* with a JSOP_EXCEPTION opcode before the guard condition evaluation.  If the guard fails, control branches to the next catch, or to the rethrow sequence.  So after the first failing guard, cx->throwing will be false but cx->exception must still contain the pending exception.

If a guard throws, that new exception will clobber cx->{throwing,exception} and propagate -- no problem.  This always worked.

But what if a guard triggers a GC?  The GC will not scan cx->exception if !cx->throwing.  But the catch code binds the exception to the catch-var-named property of an Object on the scope chain, which saves us.  There is a GC hazard when popping that scope object (JSOP_LEAVEWITH) on guard failure, but it is not exploitable at present -- the only opcodes before another JSOP_EXCEPTION picks up the hanging-by-a-thread cx->exception are GC-safe (JSOP_SETSP, JSOP_THROWING [see below]).

And, if no guard succeeds and there is a finally block, the rethrow sequence of JSOP_SETSP; JSOP_GOSUB will not save the pending exception under JSOP_GOSUB, because cx->throwing was cleared by the first guard's JSOP_EXCEPTION.  Hence JSOP_THROWING, to restore cx->throwing right before the SETSP/GOSUB.

For try-catch(guard) without any finally, the rethrow sequence must once again use JSOP_EXCEPTION to push the pending (but !cx->throwing) exception, then JSOP_THROW to propagate it.

This all makes for fairly hairy case analysis, in part due to guards (a feature that was proposed for ECMA-262 Edition 3 but rejected), in part just because of the number of try/catch/finally combinations.  But guards hurt.

Trunk and branch patches soon, with yet another (unique) XUL_FASTLOAD_FILE_VERSION number change.

/be

Comment 15

[Brendan Eich [:brendan]]

(In reply to comment #14)
> But what if a guard triggers a GC?  The GC will not scan cx->exception if
> !cx->throwing.  But the catch code binds the exception to the catch-var-named
> property of an Object on the scope chain, which saves us.

This is true for the first guarded catch.

> There is a GC hazard
> when popping that scope object (JSOP_LEAVEWITH) on guard failure, but it is not
> exploitable at present -- the only opcodes before another JSOP_EXCEPTION picks
> up the hanging-by-a-thread cx->exception are GC-safe (JSOP_SETSP, JSOP_THROWING
> [see below]).

This is not true after the first guard fails.  Consider:

js> function f(x) {
    try {
        throw x;
    } catch (e if e === 42) {
        print("catch guard 1", e);
    } catch (e if e === 43) {
        print("catch guard 2", e);
    }
}
js> dis(f)
main:
00000:  try
00001:  getarg 0
00004:  throw
00005:  goto 96 (91)
00008:  setsp 0
00011:  nop
00012:  name "Object"
00015:  pushobj
00016:  newinit
00017:  exception

This pushes cx->exception and clears cx->throwing.

00018:  initcatchvar "e"
00021:  enterwith
00022:  name "e"
00025:  uint16 42
00028:  eq
00029:  ifeq 50 (21)

On first catch's guard failure, control transfers to bytecode 50.

00032:  name "print"
00035:  pushobj
00036:  string "catch guard 1"
00039:  name "e"
00042:  call 2
00045:  pop
00046:  leavewith
00047:  goto 96 (49)
00050:  leavewith

This leaves cx->exception unrooted.

00051:  nop
00052:  name "Object"

This may cause a last-ditch GC!

00055:  pushobj
00056:  newinit
00057:  exception
00058:  initcatchvar "e"
00061:  enterwith
00062:  name "e"
00065:  uint16 43
00068:  eq
00069:  ifeq 90 (21)
00072:  name "print"
00075:  pushobj
00076:  string "catch guard 2"
00079:  name "e"
00082:  call 2
00085:  pop
00086:  leavewith
00087:  goto 96 (9)
00090:  setsp 0
00093:  exception
00094:  throw
00095:  nop

This bug does not exist with lexically scoped catch variables, which are in on the trunk and going into the 1.8 branch.  It's a problem in the patch I've just attached for 1.8.0.  I'll work on it with shaver.

/be

Comment 16

[Igor Bukanov]

(In reply to comment #14)
> This patch implements the above as directly as possible.  JSOP_GOSUB and
> JSOP_RETSUB now save and restore, respectively, exception state.  They
> therefore push and pop, respectively, two slots.

Just my 2 cents: but why 2 slots? Boolean jsval has enough space to encode jumps directly AFAICS. Also what puzzled we even in Rhino's interpreter how gosub always followed either by goto or throw. Why not to make one command? But that is not for 1.8.0. 



  To compress cx->throwing and
> cx->exception the patch uses JSVAL_HOLE to mean !cx->throwing, and all other
> jsvals to mean cx->throwing with the jsval telling the value of the thrown
> exception.
> 
> I finally bit the bullet and removed the wrong stack imbalance (nuses and ndefs
> not both 0) for these opcodes.  This relieves the straight-line stack modeling
> code in the code generator from having to restore depth to cg->stackDepth.  It
> also removes JSOP_BACKPATCH_PUSH.
> 
> The try-catch(guard)-finally case requires a new opcode, JSOP_THROWING, because
> catch guards run in series, each picking up the pending exception *and clearing
> cx->throwing* with a JSOP_EXCEPTION opcode before the guard condition
> evaluation.  If the guard fails, control branches to the next catch, or to the
> rethrow sequence.  So after the first failing guard, cx->throwing will be false
> but cx->exception must still contain the pending exception.
> 
> If a guard throws, that new exception will clobber cx->{throwing,exception} and
> propagate -- no problem.  This always worked.
> 
> But what if a guard triggers a GC?  The GC will not scan cx->exception if
> !cx->throwing.  But the catch code binds the exception to the catch-var-named
> property of an Object on the scope chain, which saves us.  There is a GC hazard
> when popping that scope object (JSOP_LEAVEWITH) on guard failure, but it is not
> exploitable at present -- the only opcodes before another JSOP_EXCEPTION picks
> up the hanging-by-a-thread cx->exception are GC-safe (JSOP_SETSP, JSOP_THROWING
> [see below]).
> 
> And, if no guard succeeds and there is a finally block, the rethrow sequence of
> JSOP_SETSP; JSOP_GOSUB will not save the pending exception under JSOP_GOSUB,
> because cx->throwing was cleared by the first guard's JSOP_EXCEPTION.  Hence
> JSOP_THROWING, to restore cx->throwing right before the SETSP/GOSUB.
> 
> For try-catch(guard) without any finally, the rethrow sequence must once again
> use JSOP_EXCEPTION to push the pending (but !cx->throwing) exception, then
> JSOP_THROW to propagate it.
> 
> This all makes for fairly hairy case analysis, in part due to guards (a feature
> that was proposed for ECMA-262 Edition 3 but rejected), in part just because of
> the number of try/catch/finally combinations.  But guards hurt.
> 
> Trunk and branch patches soon, with yet another (unique)
> XUL_FASTLOAD_FILE_VERSION number change.
> 
> /be
> 

(In reply to comment #14)
> Created an attachment (id=235780) [edit]
> new approach, 1.8.0 branch patch
> 
> The stack depth at which a finally block starts must be an invariant that the
> code generator can compute.  Combine this with the fact that started all the
> revisiting of this seven+ year old code: a finally block may run while an
> uncaught exception is pending (try-finally, or try-catch(guard)-finally with no
> unguarded catch before the finally).  Therefore, the finally must be invoked
> with a bytecode that saves any pending exception, and its final
> return-from-finally opcode must throw that saved exception, if any.
> 
> This patch implements the above as directly as possible.  JSOP_GOSUB and
> JSOP_RETSUB now save and restore, respectively, exception state.  They
> therefore push and pop, respectively, two slots.  To compress cx->throwing and
> cx->exception the patch uses JSVAL_HOLE to mean !cx->throwing, and all other
> jsvals to mean cx->throwing with the jsval telling the value of the thrown
> exception.
> 
> I finally bit the bullet and removed the wrong stack imbalance (nuses and ndefs
> not both 0) for these opcodes.  This relieves the straight-line stack modeling
> code in the code generator from having to restore depth to cg->stackDepth.  It
> also removes JSOP_BACKPATCH_PUSH.
> 
> The try-catch(guard)-finally case requires a new opcode, JSOP_THROWING, because
> catch guards run in series, each picking up the pending exception *and clearing
> cx->throwing* with a JSOP_EXCEPTION opcode before the guard condition
> evaluation.  If the guard fails, control branches to the next catch, or to the
> rethrow sequence.  So after the first failing guard, cx->throwing will be false
> but cx->exception must still contain the pending exception.
> 
> If a guard throws, that new exception will clobber cx->{throwing,exception} and
> propagate -- no problem.  This always worked.
> 
> But what if a guard triggers a GC?  The GC will not scan cx->exception if
> !cx->throwing.  But the catch code binds the exception to the catch-var-named
> property of an Object on the scope chain, which saves us.  There is a GC hazard
> when popping that scope object (JSOP_LEAVEWITH) on guard failure, but it is not
> exploitable at present -- the only opcodes before another JSOP_EXCEPTION picks
> up the hanging-by-a-thread cx->exception are GC-safe (JSOP_SETSP, JSOP_THROWING
> [see below]).
> 
> And, if no guard succeeds and there is a finally block, the rethrow sequence of
> JSOP_SETSP; JSOP_GOSUB will not save the pending exception under JSOP_GOSUB,
> because cx->throwing was cleared by the first guard's JSOP_EXCEPTION.  Hence
> JSOP_THROWING, to restore cx->throwing right before the SETSP/GOSUB.
> 
> For try-catch(guard) without any finally, the rethrow sequence must once again
> use JSOP_EXCEPTION to push the pending (but !cx->throwing) exception, then
> JSOP_THROW to propagate it.
> 
> This all makes for fairly hairy case analysis, in part due to guards (a feature
> that was proposed for ECMA-262 Edition 3 but rejected), in part just because of
> the number of try/catch/finally combinations.  But guards hurt.
> 
> Trunk and branch patches soon, with yet another (unique)
> XUL_FASTLOAD_FILE_VERSION number change.
> 
> /be
> 

Comment 17

[Brendan Eich [:brendan]]

(In reply to comment #16)
> (In reply to comment #14)
> > This patch implements the above as directly as possible.  JSOP_GOSUB and
> > JSOP_RETSUB now save and restore, respectively, exception state.  They
> > therefore push and pop, respectively, two slots.
> 
> Just my 2 cents: but why 2 slots? Boolean jsval has enough space to encode
> jumps directly AFAICS.

The two slots are [exception or JSVAL_HOLE, INT_TO_JSVAL(retsub pc-index)].  While one could combine JSVAL_HOLE and (retsub pc-index), the exception is an arbitrary value, so it needs its own slot.

> Also what puzzled we even in Rhino's interpreter how
> gosub always followed either by goto or throw. Why not to make one command? But
> that is not for 1.8.0.

Right, nor for 1.8.1!  We can do a followup bug for the trunk.

(Massive over-quoting deleted. Igor, you might like this bookmarklet for enlarging textareas -- put it in your links toolbar and click it a few times for bugzilla, or tweak it to use a bigger delta:

javascript:(function(){var i,x; for(i=0;x=document.getElementsByTagName(%22textarea%22)[i];++i) x.rows += 5; })()

;-)

/be

Comment 18

[Brendan Eich [:brendan]]

Attachment: new approach, with GC safety for non-initial failing guards

JSOP_THROWING to the rescue:

...
00029:  ifeq 50 (21)
00032:  name "print"
00035:  pushobj
00036:  string "catch guard 1"
00039:  name "e"
00042:  call 2
00045:  pop
00046:  leavewith
00047:  goto 97 (50)
00050:  throwing
00051:  leavewith
00052:  nop
00053:  name "Object"
00056:  pushobj
00057:  newinit
00058:  exception
...

Clearly more could be done for bytecode efficiency.  The lexical scope change alone is good, but the gosub/retsub conceit is over-general and Java-influenced without paying its way as Igor notes.  But later, we've got 1.8.0.7 and 1.8.1 to get out.

/be

Comment 19

[Igor Bukanov]

(In reply to comment #17)
> > Just my 2 cents: but why 2 slots? Boolean jsval has enough space to encode
> > jumps directly AFAICS.
> 
> The two slots are [exception or JSVAL_HOLE, INT_TO_JSVAL(retsub pc-index)]. 
> While one could combine JSVAL_HOLE and (retsub pc-index), the exception is an
> arbitrary value, so it needs its own slot.

It is not that arbitrary to fill all cryptic boolean space. I already learned that there are 2^29 - 2 unused cryptic booleans. That should be enough to fit all the jumps. Or does SM supports 2^29 long scripts? 

> javascript:(function(){var i,x;
> for(i=0;x=document.getElementsByTagName(%22textarea%22)[i];++i) x.rows += 5;
> })()

Thanks !

Comment 20

[Brendan Eich [:brendan]]

(In reply to comment #19)
> (In reply to comment #17)
> > > Just my 2 cents: but why 2 slots? Boolean jsval has enough space to encode
> > > jumps directly AFAICS.
> > 
> > The two slots are [exception or JSVAL_HOLE, INT_TO_JSVAL(retsub pc-index)]. 
> > While one could combine JSVAL_HOLE and (retsub pc-index), the exception is an
> > arbitrary value, so it needs its own slot.
> 
> It is not that arbitrary to fill all cryptic boolean space. I already learned
> that there are 2^29 - 2 unused cryptic booleans. That should be enough to fit
> all the jumps.

Sure, combining HOLE and pc-index is no problem.  But the thrown value needs to be saved and rethrown in some cases, so that needs its own stack slot.  Or is your point that in that case, JSOP_RETSUB should become something that rethrows?  I got that already, just making sure I'm following what you wrote in case there is something to do for 1.8x.

> Or does SM supports 2^29 long scripts?

See JUMPX_OFFSET_(MIN|MAX) in jsopcode.h -- we do 32 bits.
 
> > javascript:(function(){var i,x;
> > for(i=0;x=document.getElementsByTagName(%22textarea%22)[i];++i) x.rows += 5;
> > })()
> 
> Thanks !

np.

/be

Comment 21

[Igor Bukanov]

(In reply to comment #20)
> Sure, combining HOLE and pc-index is no problem.  But the thrown value needs to
> be saved and rethrown in some cases, so that needs its own stack slot.  Or is
> your point that in that case, JSOP_RETSUB should become something that
> rethrows?  I got that already, just making sure I'm following what you wrote in
> case there is something to do for 1.8x.

Yes, that is exactly the idea.

> > Or does SM supports 2^29 long scripts?
> 
> See JUMPX_OFFSET_(MIN|MAX) in jsopcode.h -- we do 32 bits.

Well, then one can encode index into TryNotes.

Comment 22

[Igor Bukanov]

(In reply to comment #18)
> But later, we've got 1.8.0.7 and 1.8.1 to get out.

Honestly I really worry about the amount of changes that goes into 1.8.0. Clearly, there is not enough test coverage. Thus what about taking back the patch for the original bug that caused the regression and replace that for 1.8.0 by a code that detects the condition and throws Way-To-Complex problem exception?

Comment 23

[Bob Clary [:bc] (inactive)]

Checking in regress-350312.js;
/cvsroot/mozilla/js/tests/js1_5/Regress/regress-350312.js,v  <--  regress-350312.js
initial revision: 1.1

building 1.8.0.7 attachment 235798 [details] [diff] [review] now. Will run this test and report back and then run the full suite on windows/linux and report back in a couple of hours.

Comment 24

[Igor Bukanov]

Comment on attachment 235798 [details] [diff] [review]
new approach, with GC safety for non-initial failing guards

>           case JSOP_GOSUB:
>+            lval = cx->throwing ? cx->exception : JSVAL_HOLE;
>+            PUSH(lval);

For extra safety as this is a patch for 1.8.0 I suggest to check that exception is not JSVAL_HOLE and if it, then changing it to true or something. I do like the idea of a latent harmless bug that currently accidentally pushes JSVAL_HOLE becomming an arbitrary code execution exploit.

Comment 25

[Bob Clary [:bc] (inactive)]

assuming the test isn't bogus: trunk and 1.8.0.7 with the patch

BUGNUMBER: 350312
STATUS: Accessing wrong stack slot with nested catch/finally
42
FAILED!: [reported from test()] uncaught exception: 2
on winxp/linux

1.8 crashes js_Interpret(JSContext * cx=0x03dee680, unsigned char * pc=0x05ff4d3a, long * result=0x0012f5d8)  Line 2219 + 0x5 bytes	C

Comment 26

[Brendan Eich [:brendan]]

Attachment: new approach, with shaver suggestions and assertions for igor

sr=shaver, he said just a few minutes ago.

Interdiff against previous patch:

diff -u js/src/jsbool.h js/src/jsbool.h
--- js/src/jsbool.h     28 Aug 2006 21:30:05 -0000
+++ js/src/jsbool.h     28 Aug 2006 22:29:40 -0000
@@ -49,7 +49,8 @@
  * Crypto-booleans, not visible to script but used internally by the engine.
  *
  * JSVAL_HOLE is a useful value for identifying a hole in an array.  It's also
- * used in the interpreter to represent "no exception pending".
+ * used in the interpreter to represent "no exception pending".  In general it
+ * can be used to represent "no value".
  */
 #define JSVAL_HOLE      BOOLEAN_TO_JSVAL(2)
 
diff -u js/src/jsinterp.c js/src/jsinterp.c
--- js/src/jsinterp.c   28 Aug 2006 21:30:06 -0000
+++ js/src/jsinterp.c   28 Aug 2006 22:29:41 -0000
@@ -4939,6 +4939,7 @@
             break;
 
           case JSOP_GOSUB:
+            JS_ASSERT(cx->exception != JSVAL_HOLE);
             lval = cx->throwing ? cx->exception : JSVAL_HOLE;
             PUSH(lval);
             i = PTRDIFF(pc, script->main, jsbytecode) + len;
@@ -4947,6 +4948,7 @@
             break;
 
           case JSOP_GOSUBX:
+            JS_ASSERT(cx->exception != JSVAL_HOLE);
             lval = cx->throwing ? cx->exception : JSVAL_HOLE;
             PUSH(lval);
             i = PTRDIFF(pc, script->main, jsbytecode) + len;
@@ -4957,16 +4959,22 @@
           case JSOP_RETSUB:
             rval = POP();
             JS_ASSERT(JSVAL_IS_INT(rval));
-            i = JSVAL_TO_INT(rval);
-            pc = script->main + i;
-            len = 0;
             lval = POP();
             if (lval != JSVAL_HOLE) {
+                /*
+                 * Exception was pending during finally, throw it *before* we
+                 * adjust pc, because pc indexes into script->trynotes.  This
+                 * turns out not to be necessary, but it seems clearer.  And
+                 * it points out a FIXME: 350509, due to Igor Bukanov.
+                 */
                 cx->throwing = JS_TRUE;
                 cx->exception = lval;
                 ok = JS_FALSE;
                 goto out;
             }
+            i = JSVAL_TO_INT(rval);
+            pc = script->main + i;
+            len = 0;
             break;
 
           case JSOP_EXCEPTION:
@@ -4975,6 +4983,7 @@
             break;
 
           case JSOP_THROWING:
+            JS_ASSERT(!cx->throwing);
             cx->throwing = JS_TRUE;
             break;
 
/be

Comment 27

[Brendan Eich [:brendan]]

Comment on attachment 235808 [details] [diff] [review]
new approach, with shaver suggestions and assertions for igor

Igor, old bad code had GC safety hole, see my comments above.  I think this is worth reviewing and testing for 1.8.0, if not for 1.8.0.7, then for .0.8.  But having crawled over the code and written the testcase generator I gave to bclary (attached next), I think we are done.

In particular, I don't see a need to test for JSVAL_HOLE in cx->exception except by asserting.  The only place that might throw JSVAL_HOLE is the JSOP_RETSUB case as patched, but it tests for JSVAL_HOLE.  Were you worried about random native hooks throwing a crypto-boolean?

/be

Comment 28

[Brendan Eich [:brendan]]

Attachment: testcase generator

My patched 1.8.0 js shell produces identical output to the trunk unpatched js shell given this input.

/be

Comment 29

[Brendan Eich [:brendan]]

The patch for bug 349331 (replacing GeneratorExit with internal "asynchronous return" exception), Igor's great idea, should build on the trunk and 1.8 branch versions of this bug's patch, by adding JSVAL_ARETURN to jsbool.h.  These "occult" or crypto-booleans need to be hidden, but not scattered, and I reckon jsbool.h is the place to define them in their secret order.

/be

Comment 30

[Bob Clary [:bc] (inactive)]

The test I checked in was bogus. This should be better:
Checking in regress-350312.js;
/cvsroot/mozilla/js/tests/js1_5/Regress/regress-350312.js,v  <--  regress-350312.js
new revision: 1.2; previous revision: 1.1

With the patch 1.8.0.7 passes js1_5/Regress/regress-350312.js and does not regress
js1_5/Regress/regress-346494-01.js on winxp and linux.

Comment 31

[Daniel Veditz [:dveditz]]

Comment on attachment 235808 [details] [diff] [review]
new approach, with shaver suggestions and assertions for igor

approved for 1.8.0 branch, a=dveditz for drivers

Comment 32

[Brendan Eich [:brendan]]

(In reply to comment #21)
> (In reply to comment #20)
> > Sure, combining HOLE and pc-index is no problem.  But the thrown value needs to
> > be saved and rethrown in some cases, so that needs its own stack slot.  Or is
> > your point that in that case, JSOP_RETSUB should become something that
> > rethrows?  I got that already, just making sure I'm following what you wrote in
> > case there is something to do for 1.8x.
> 
> Yes, that is exactly the idea.

I filed bug 350509 on this.

/be

Comment 33

[Brendan Eich [:brendan]]

Attachment: 1.8 branch version of patch

Comment 34

[Brendan Eich [:brendan]]

Attachment: successor trunk patch based on 1.8 branch patch

Interdiff won't work too well, but diffing the patches helps if you grep out all context jitter.

/be

Comment 35

[Igor Bukanov]

Comment on attachment 235808 [details] [diff] [review]
new approach, with shaver suggestions and assertions for igor

I did not ask for asserts! I asked for defensive codding against bugs that put junk into cx->exception. Such bugs with the current code are harmless. After the patch they become arbitrary code execution exploits. 

So the idea is to patch JS_SetPendingException to  reject pseudo booleans. But that can be done in a separated bug. For know I learned all the problems that have to be addressed  and the patch does it right AFAICS.

Comment 36

[Brendan Eich [:brendan]]

(In reply to comment #35)
> So the idea is to patch JS_SetPendingException to  reject pseudo booleans.

If you fear an accidental exploit, there are lots of ways to do that (most likely by forgetting to root a GC-thing), but sure -- we should make JS_SetPendingException reject crypto-booleans.

> But
> that can be done in a separated bug.

I will file it later today.

> For know I learned all the problems that
> have to be addressed  and the patch does it right AFAICS.

Thanks,

/be

Comment 37

[Brendan Eich [:brendan]]

Attachment: updated successor trunk patch

This includes changes committed for bug 349331.  I moved JSVAL_ARETURN to jsbool.h and js_FindFinallyHandler to jsscript.c, updating its bytecode inspection to track this bug's fix.

I would like very much to get this into the trunk today.

/be

Comment 38

[Brendan Eich [:brendan]]

Attachment: updated patch for 1.8 branch

After this I will get the patch for lexical catch var scope ready to land on the branch, and we can be free of the merge pain here.

/be

Comment 39

[Igor Bukanov]

Comment on attachment 235954 [details] [diff] [review]
updated patch for 1.8 branch

>+            /*
>+             * We have a handler, is it finally?
>+             *
>+             * Catch bytecode begins with:   JSOP_SETSP JSOP_NOP
>+             * Finally bytecode begins with: JSOP_SETSP JSOP_(GOSUB|THROWING)
>+             */
>+            pc = script->main + tn->catchStart;
>+            JS_ASSERT(*pc == JSOP_SETSP);
>+            op2 = pc[JSOP_SETSP_LENGTH];
>+            if (op2 != JSOP_NOP) {
>+                JS_ASSERT(op2 == JSOP_GOSUB || op2 == JSOP_THROWING);
>+                return pc;
>+            }

This is not right. Consider:

function f(a, b, c)
{
        try {
                a();
        } catch (e if e == null) {
                b();
        } finally {
                c();
        }
}

dis(f)

==>

main:
00000:  try
00001:  getarg 0
00004:  pushobj
00005:  call 0
00008:  pop
00009:  gosub 57 (48)
00012:  goto 68 (56)
00015:  setsp 0
00018:  enterblock depth 0 {e: 0}
00021:  exception
00022:  setlocalpop 0
00025:  getlocal 0
00028:  null
00029:  eq
00030:  ifeq 50 (20)
00033:  getarg 1
00036:  pushobj
00037:  call 0
00040:  pop
00041:  leaveblock 1
00044:  gosub 57 (13)
00047:  goto 68 (21)
00050:  throwing
00051:  setsp 0
00054:  gosub 57 (3)
00057:  finally
00058:  getarg 2
00061:  pushobj
00062:  call 0
00065:  pop
00066:  retsub
00067:  nop
00068:  stop

Source notes:
  0:     0 [   0] newline
  1:     1 [   1] newline
  2:     5 [   4] pcbase   offset 4
  4:     9 [   4] hidden
  5:    12 [   3] hidden
  6:    18 [   6] catch    guard size 5
  8:    18 [   0] newline
  9:    33 [  15] xdelta
 10:    33 [   0] newline
 11:    37 [   4] pcbase   offset 4
 13:    41 [   4] catch    guard size 1
 15:    47 [   6] hidden
 16:    57 [  10] xdelta
 17:    57 [   0] newline
 18:    58 [   1] newline
 19:    62 [   4] pcbase   offset 4
 21:    67 [   5] endbrace

Exception table:
start   end     catch
  1     15      15
  1     50      50

That is, now finally starts with [throwing]. Should TryNote entry for finally starts after throwing and the test for finally be as before?

Comment 40

[Mike Shaver (:shaver emeritus)]

Comment on attachment 235952 [details] [diff] [review]
updated successor trunk patch

sr=shaver

Comment 41

[Mike Shaver (:shaver emeritus)]

Comment on attachment 235954 [details] [diff] [review]
updated patch for 1.8 branch

sr=shaver: we only jump to the finally case via the TryNote if there's an exception escaping, in which case we want to start with a [throwing].  For the end-of-try or end-of-catch cases we correctly [gosub] to 57 in your example, and don't execute [throwing].

Comment 42

[Brendan Eich [:brendan]]

(In reply to comment #39)
> (From update of attachment 235954 [details] [diff] [review] [edit])
> >+            /*
> >+             * We have a handler, is it finally?
> >+             *
> >+             * Catch bytecode begins with:   JSOP_SETSP JSOP_NOP
> >+             * Finally bytecode begins with: JSOP_SETSP JSOP_(GOSUB|THROWING)
> >+             */
> >+            pc = script->main + tn->catchStart;
> >+            JS_ASSERT(*pc == JSOP_SETSP);
> >+            op2 = pc[JSOP_SETSP_LENGTH];
> >+            if (op2 != JSOP_NOP) {
> >+                JS_ASSERT(op2 == JSOP_GOSUB || op2 == JSOP_THROWING);
> >+                return pc;
> >+            }
> 
> This is not right. Consider:
> 
> function f(a, b, c)
> {
>         try {
>                 a();
>         } catch (e if e == null) {
>                 b();
>         } finally {
>                 c();
>         }
> }

The finally handler has three forms, as commented in jsemit.c in the patches:

1. [setsp][gosub] for try-finally and try-catch(no-guard)-finally,

2. [throwing][setsp][gosub] for try-catch(guard)-finally, and

3. [throwing][exception][throw] for try-catch(guard) but no finally.

So the assertion is correct given current code generation.

> main:
> 00000:  try
> 00001:  getarg 0
> 00004:  pushobj
> 00005:  call 0
> 00008:  pop
> 00009:  gosub 57 (48)
> 00012:  goto 68 (56)
> 00015:  setsp 0
> 00018:  enterblock depth 0 {e: 0}
> 00021:  exception
> 00022:  setlocalpop 0
> 00025:  getlocal 0
> 00028:  null
> 00029:  eq
> 00030:  ifeq 50 (20)
> 00033:  getarg 1
> 00036:  pushobj
> 00037:  call 0
> 00040:  pop
> 00041:  leaveblock 1
> 00044:  gosub 57 (13)
> 00047:  goto 68 (21)
> 00050:  throwing
> 00051:  setsp 0
> 00054:  gosub 57 (3)
> 00057:  finally
> 00058:  getarg 2
> 00061:  pushobj
> 00062:  call 0
> 00065:  pop
> 00066:  retsub
> 00067:  nop
> 00068:  stop
> 
[snipping source notes]
> 
> Exception table:
> start   end     catch
>   1     15      15
>   1     50      50
> 
> That is, now finally starts with [throwing]. Should TryNote entry for finally
> starts after throwing and the test for finally be as before? 

Throwing is harmless but redundant (except in DEBUG builds, where it will assertbotch), because the exception is already pending in the try-finally and try-catch-finally cases (latter case must involve catch rethrowing).  In your example, with a catch guard, the ifeq 50 must go to throwing, and does.

But the finally handler could go to 51, you are right.  I will update the trunk and 1.8 branch patches and re-mark shaver's sr+ (he and I were talking when we saw your review, and he's up to date modulo the JSOP_THROWING assert botch ;-)).  I'm not touching the 1.8.0 branch again unless dveditz says so.

/be

Comment 43

[Brendan Eich [:brendan]]

Argh, I'm blind.  Igor, you are right, js_FindFinallyHandler is broken.  I'm tired of this bug now...

New patches shortly.

/be

Comment 44

[Brendan Eich [:brendan]]

(In reply to comment #43)
> Argh, I'm blind.  Igor, you are right, js_FindFinallyHandler is broken.

The assertion here will botch, I mean -- I was thinking JSOP_EXCEPTION but typing JSOP_THROWING.

>+             * Finally bytecode begins with: JSOP_SETSP JSOP_(GOSUB|THROWING)
>+             */
>+            pc = script->main + tn->catchStart;
>+            JS_ASSERT(*pc == JSOP_SETSP);
>+            op2 = pc[JSOP_SETSP_LENGTH];
>+            if (op2 != JSOP_NOP) {
>+                JS_ASSERT(op2 == JSOP_GOSUB || op2 == JSOP_THROWING);
>+                return pc;
>+            }

/be

Comment 45

[Brendan Eich [:brendan]]

Here is an demo based on Igor's test function, with the forthcoming patch applied:

js> function f(a, b, c)
{
        try {
                a();
        } catch (e if e == null) {
                b();
        } finally {
                c();
        }
}
js> f(function (){print('a')}, function(){print('b')},function(){print('c')})
a
c
js> f(function (){throw 'a'}, function(){print('b')},function(){print('c')})
c
uncaught exception: a
js> f(function (){throw null}, function(){print('b')},function(){print('c')})
b
c
js> f(function (){print('a')}, function(){throw 'b'},function(){print('c')})
a
c
js> f(function (){throw null}, function(){throw 'b'},function(){print('c')})
c
uncaught exception: b

This does not show js_FindFinallyHandler in action, we need a generator for that. The testsuite needs to cover all of these cases too.

/be

Comment 46

[Brendan Eich [:brendan]]

Attachment: successor trunk patch, v2

Comment 47

[Brendan Eich [:brendan]]

Attachment: updated patch for 1.8 branch, v2

Comment 48

[Brendan Eich [:brendan]]

And for once, bugzilla's interdiff works!

/be

Comment 49

[Igor Bukanov]

Comment on attachment 235971 [details] [diff] [review]
successor trunk patch, v2

Now it works as it should with generators as well:

js> var iter;
js> function gen()
{ 
  try {
    yield iter;
  } catch (e if e == null) {
    print("CATCH");
  } finally {
    print("FINALLY");
  }
}
js> (iter = gen()).next().close()
FINALLY
js> (iter = gen()).next().throw(1)
FINALLY
uncaught exception: 1
js> (iter = gen()).next().throw(null)
CATCH
FINALLY
uncaught exception: [object StopIteration]
js> (iter = gen()).next().next()      
FINALLY
uncaught exception: [object StopIteration]

Comment 50

[Brendan Eich [:brendan]]

Attachment: js shell testcase generator

Bob, take note.  This covers the try-catch(guard)-finally where the catch block throws, finding the finally handler and botching the JS_ASSERT(!cx->throwing) in the 1.8.0 branch's JSOP_THROWING interpreter case.

Patch for 1.8.0 branch next, we can take it for sanity and to avoid the assertion botching any time.

Still no generator-based js_FindFinallyHandler test here, but IIRC Igor had one in the relevant bug (the one on getting rid of GeneratorExit).

/be

Comment 51

[Brendan Eich [:brendan]]

Attachment: Fix jsemit.c to avoid assertbotching on the 1.8.0 branch

No pressure, but it would be good to take this change, to match code generation (finally handler entry point does not include JSOP_THROWING, therefore no assert botch) on all branches and trunk.

/be

Comment 52

[Igor Bukanov]

Comment on attachment 235972 [details] [diff] [review]
updated patch for 1.8 branch, v2

+ just after looking at the  diff of the patch compared with the trunk version.

Comment 53

[Brendan Eich [:brendan]]

Oops, didn't see your comment there, Igor!  Thanks.  I'll nominate the 1.8 branch patch after the trunk patch has landed without trouble.

/be

Comment 54

[Brendan Eich [:brendan]]

Fixed on the trunk.

/be

Comment 55

[Bob Clary [:bc] (inactive)]

Attachment: js1_5/Regress/regress-350312.js

updated version of the test. I mistakenly committed the original even though this is a security sensitive bug. I will be attaching the remainder here.

Comment 56

[Bob Clary [:bc] (inactive)]

Attachment: js1_5/Regress/regress-350312-01.js

from comment 1

Comment 57

[Bob Clary [:bc] (inactive)]

Attachment: js1_5/Regress/regress-350312-02.js

from Comment #45. I'm not sure this is tests what the original comment does as builds >= 20060825 pass this test.

Comment 58

[Bob Clary [:bc] (inactive)]

Attachment: js1_5/Regress/regress-350312-03.js

winxp 1.8.0.7 shell asserts

 	ntdll.dll!_DbgBreakPoint@0() 	
>	js32.dll!JS_Assert(const char * s=0x610b8200, const char * file=0x610b81f4, int ln=4986)  Line 59	
 	[Frames below may be incorrect and/or missing, no symbols loaded for js32.dll]	
 	js32.dll!js_Interpret(JSContext * cx=0x000323a8, unsigned char * pc=0x004448d1, long * result=0x0013e570)  Line 4986 + 0x25 bytes	
 	js32.dll!js_Invoke(JSContext * cx=0x000323a8, unsigned int argc=0, unsigned int flags=0)  Line 1207 + 0x13 bytes	
 	js32.dll!js_Interpret(JSContext * cx=0x000323a8, unsigned char * pc=0x0042aeec, long * result=0x0013ee20)  Line 3583 + 0xf bytes	
 	js32.dll!js_Execute(JSContext * cx=0x000323a8, JSObject * chain=0x000393c8, JSScript * script=0x0042ae80, JSStackFrame * down=0x00000000, unsigned int flags=0, long * result=0x0013fecc)  Line 1433 + 0x13 bytes	
 	js32.dll!JS_ExecuteScript(JSContext * cx=0x000323a8, JSObject * obj=0x000393c8, JSScript * script=0x0042ae80, long * rval=0x0013fecc)  Line 4027 + 0x19 bytes	

winxp 1.8 crashes

>	js32.dll!js_Interpret(JSContext * cx=0x00032f10, unsigned char * pc=0x00428b7b, long * result=0x0013ee10)  Line 3328 + 0x9d bytes	
 	[Frames below may be incorrect and/or missing, no symbols loaded for js32.dll]	
 	js32.dll!js_Execute(JSContext * cx=0x00032f10, JSObject * chain=0x000396a0, JSScript * script=0x00428f10, JSStackFrame * down=0x00000000, unsigned int flags=0, long * result=0x0013fec4)  Line 1599 + 0x13 bytes	
 	js32.dll!JS_ExecuteScript(JSContext * cx=0x00032f10, JSObject * obj=0x000396a0, JSScript * script=0x00428f10, long * rval=0x0013fec4)  Line 4256 + 0x19 bytes	

winxp trunk passes.

Comment 59

[Bob Clary [:bc] (inactive)]

Attachment: js1_7/geniter/regress-350312.js

winxp 1.8 asserts 	ntdll.dll!_DbgBreakPoint@0() 	
>	js32.dll!JS_Assert(const char * s=0x610cd63c, const char * file=0x610cd630, int ln=6382)  Line 59	
 	[Frames below may be incorrect and/or missing, no symbols loaded for js32.dll]	
 	js32.dll!js_FindFinallyHandler(JSScript * script=0x004238c0, unsigned char * pc=0x004238fd)  Line 6382 + 0x2c bytes	
 	js32.dll!js_Interpret(JSContext * cx=0x00032f28, unsigned char * pc=0x004238f6, long * result=0x0013e394)  Line 6245 + 0x10 bytes	
 	js32.dll!SendToGenerator(JSContext * cx=0x00032f28, int op=3, JSObject * obj=0x0003a6b8, JSGenerator * gen=0x00426560, long arg=-2147483647, long * rval=0x0013e4b4)  Line 764 + 0x14 bytes	
 	js32.dll!generator_op(JSContext * cx=0x00032f28, int op=3, JSObject * obj=0x0003a6b8, unsigned int argc=0, long * argv=0x0042db80, long * rval=0x0013e4b4)  Line 881 + 0x1d bytes	
 	js32.dll!generator_close(JSContext * cx=0x00032f28, JSObject * obj=0x0003a6b8, unsigned int argc=0, long * argv=0x0042db80, long * rval=0x0013e4b4)  Line 918 + 0x1b bytes	
 	js32.dll!js_Invoke(JSContext * cx=0x00032f28, unsigned int argc=0, unsigned int flags=0)  Line 1350 + 0x1a bytes	
 	js32.dll!js_Interpret(JSContext * cx=0x00032f28, unsigned char * pc=0x00423a4d, long * result=0x0013ee10)  Line 4047 + 0xf bytes	
 	js32.dll!js_Execute(JSContext * cx=0x00032f28, JSObject * chain=0x000396a0, JSScript * script=0x004242c0, JSStackFrame * down=0x00000000, unsigned int flags=0, long * result=0x0013fec4)  Line 1599 + 0x13 bytes	
 	js32.dll!JS_ExecuteScript(JSContext * cx=0x00032f28, JSObject * obj=0x000396a0, JSScript * script=0x004242c0, long * rval=0x0013fec4)  Line 4256 + 0x19 bytes	

trunk winxp passes

Brendan, is this what you meant for js_FindFinallyHandler ?

Comment 60

[Brendan Eich [:brendan]]

See comment 51 for the patch to cure the 1.8.0 branch assertbotch in JSOP_THROWING (which is not compiled in release builds, so not a problem blcoking 1.8.0.7).

As your testing shows, attachment 235972 [details] [diff] [review] is needed on the 1.8 branch.  I'll nominate it now.

/be

Comment 61

[Brendan Eich [:brendan]]

Comment on attachment 235972 [details] [diff] [review]
updated patch for 1.8 branch, v2

This is ready for 1.8.1.

/be

Comment 62

[Brendan Eich [:brendan]]

Fixed on the 1.8 branch.

/be

Comment 63

[Bob Clary [:bc] (inactive)]

verified fixed 1.9 20060830 windows/mac*/linux

1.8.0.7 20060830 linux/mac* Assertion failure: !cx->throwing, at jsinterp.c:4986

js1_5/Regress/regress-350312-02.js
js1_5/Regress/regress-350312-03.js

not verifying 1.8.0.7

Comment 64

[Daniel Veditz [:dveditz]]

Comment on attachment 235977 [details] [diff] [review]
Fix jsemit.c to avoid assertbotching on the 1.8.0 branch

Filed bug 350760 to cover landing this patch in 1.8.0.8

Comment 65

[Bob Clary [:bc] (inactive)]

verified fixed 1.8 20060831 windows/mac*/linux

Comment 66

[Bob Clary [:bc] (inactive)]

I accidentally checked in a local change as part of the checkin for the test in bug 350692.

Checking in regress-350312.js;
/cvsroot/mozilla/js/tests/js1_5/Regress/regress-350312.js,v  <--  regress-350312.js
new revision: 1.3; previous revision: 1.2
done

Comment 67

[Bob Clary [:bc] (inactive)]

verified fixed 1.8.0.7 2006090118 windows/mac*/linux

Comment 68

[Daniel Veditz [:dveditz]]

Comment on attachment 235977 [details] [diff] [review]
Fix jsemit.c to avoid assertbotching on the 1.8.0 branch

approved for 1.8.0 branch, a=dveditz for drivers

Comment 69

[Daniel Veditz [:dveditz]]

Comment on attachment 235977 [details] [diff] [review]
Fix jsemit.c to avoid assertbotching on the 1.8.0 branch

This apparently landed in 1.8.0.7 after all.

Comment 70

[Bob Clary [:bc] (inactive)]

RCS file: /cvsroot/mozilla/js/tests/js1_5/Regress/regress-350312-01.js,v
done
Checking in js1_5/Regress/regress-350312-01.js;
/cvsroot/mozilla/js/tests/js1_5/Regress/regress-350312-01.js,v  <--  regress-350312-01.js
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/js/tests/js1_5/Regress/regress-350312-02.js,v
done
Checking in js1_5/Regress/regress-350312-02.js;
/cvsroot/mozilla/js/tests/js1_5/Regress/regress-350312-02.js,v  <--  regress-350312-02.js
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/js/tests/js1_5/Regress/regress-350312-03.js,v
done
Checking in js1_5/Regress/regress-350312-03.js;
/cvsroot/mozilla/js/tests/js1_5/Regress/regress-350312-03.js,v  <--  regress-350312-03.js
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/js/tests/js1_7/geniter/regress-350312.js,v
done
Checking in js1_7/geniter/regress-350312.js;
/cvsroot/mozilla/js/tests/js1_7/geniter/regress-350312.js,v  <--  regress-350312.js
initial revision: 1.1
done