Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Accessing wrong stack slot with nested catch/finally

VERIFIED FIXED in mozilla1.8.1

Status

()

Core
JavaScript Engine
P1
critical
VERIFIED FIXED
11 years ago
11 years ago

People

(Reporter: Igor Bukanov, Assigned: brendan)

Tracking

({regression, verified1.8.0.7, verified1.8.1})

Trunk
mozilla1.8.1
regression, verified1.8.0.7, verified1.8.1
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.8.1 +
blocking1.8.0.7 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical][schrep-181approval pending])

Attachments

(12 attachments, 8 obsolete attachments)

2.45 KB, patch
shaver
: review+
Details | Diff | Splinter Review
25.67 KB, patch
mrbkap
: review+
Igor Bukanov
: review+
brendan
: superreview+
Details | Diff | Splinter Review
886 bytes, text/plain
Details
29.77 KB, patch
Igor Bukanov
: review+
brendan
: superreview+
Details | Diff | Splinter Review
29.84 KB, patch
Igor Bukanov
: review+
brendan
: superreview+
mconnor
: approval1.8.1+
Details | Diff | Splinter Review
1.33 KB, text/plain
Details
2.26 KB, patch
brendan
: review+
Details | Diff | Splinter Review
2.95 KB, text/plain
Details
2.62 KB, text/plain
Details
3.82 KB, text/plain
Details
5.97 KB, text/plain
Details
3.18 KB, text/plain
Details
(Reporter)

Description

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

Updated

11 years ago
Blocks: 349331
(Reporter)

Comment 1

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


Summary: Stack corruption with yield-catch around yield → Accessing wrong stack slot with nested catch/finally
(Assignee)

Comment 2

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

/be
Assignee: general → brendan
Severity: normal → critical
Flags: blocking1.8.1?
Flags: blocking1.8.0.7?
Priority: -- → P1
Target Milestone: --- → mozilla1.8.1
(Assignee)

Updated

11 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 3

11 years ago
Created attachment 235596 [details] [diff] [review]
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
Attachment #235596 - Flags: review?(shaver)
(Assignee)

Comment 4

11 years ago
Created attachment 235597 [details] [diff] [review]
1.8 branch patch
Attachment #235597 - Flags: review?(shaver)
Attachment #235597 - Flags: approval1.8.1?
(Assignee)

Comment 5

11 years ago
Created attachment 235598 [details] [diff] [review]
1.8.0 branch patch

This should go in today for 1.8.0.7.

/be
Attachment #235598 - Flags: review?(shaver)
Attachment #235598 - Flags: approval1.8.0.7?
(Assignee)

Comment 6

11 years ago
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
Blocks: 346494
Comment on attachment 235596 [details] [diff] [review]
trunk patch

r=shaver

Let's get this into the suite, too.
Attachment #235596 - Flags: review?(shaver) → review+
Comment on attachment 235597 [details] [diff] [review]
1.8 branch patch

r=shaver, including the error-checking ride-along. :)
Attachment #235597 - Flags: review?(shaver) → review+
Comment on attachment 235598 [details] [diff] [review]
1.8.0 branch patch

r=shaver, it's good for 1.8.0.7.
Attachment #235598 - Flags: review?(shaver) → review+

Updated

11 years ago
Whiteboard: [schrep-181approval pending]
(Assignee)

Comment 10

11 years ago
Fixed on trunk.

/be
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED

Comment 11

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

a=schrep/beltnzer for drivers.
Attachment #235597 - Flags: approval1.8.1? → approval1.8.1+

Updated

11 years ago
Flags: blocking1.8.1? → blocking1.8.1+
Comment on attachment 235598 [details] [diff] [review]
1.8.0 branch patch

a=dveditz for 1.8.0
Attachment #235598 - Flags: approval1.8.0.7? → approval1.8.0.7+
Keywords: regression
(Assignee)

Comment 13

11 years ago
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Flags: blocking1.8.0.7? → blocking1.8.0.7+
(Assignee)

Comment 14

11 years ago
Created attachment 235780 [details] [diff] [review]
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
Attachment #235780 - Flags: superreview?(shaver)
Attachment #235780 - Flags: review?(mrbkap)
Attachment #235780 - Flags: approval1.8.0.7?
(Assignee)

Updated

11 years ago
Attachment #235598 - Attachment is obsolete: true
(Assignee)

Updated

11 years ago
Attachment #235597 - Attachment is obsolete: true
(Assignee)

Comment 15

11 years ago
(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
Status: REOPENED → ASSIGNED
(Reporter)

Comment 16

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

(Assignee)

Comment 17

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

Comment 18

11 years ago
Created attachment 235798 [details] [diff] [review]
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
Attachment #235780 - Attachment is obsolete: true
Attachment #235798 - Flags: superreview?(shaver)
Attachment #235798 - Flags: review?(mrbkap)
Attachment #235798 - Flags: approval1.8.0.7?
Attachment #235780 - Flags: superreview?(shaver)
Attachment #235780 - Flags: review?(mrbkap)
Attachment #235780 - Flags: approval1.8.0.7?
(Reporter)

Comment 19

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

Comment 20

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

Comment 21

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

Comment 22

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

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

Comment 24

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

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

(Assignee)

Comment 26

11 years ago
Created attachment 235808 [details] [diff] [review]
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
Attachment #235798 - Attachment is obsolete: true
Attachment #235808 - Flags: superreview+
Attachment #235808 - Flags: review?(mrbkap)
Attachment #235798 - Flags: superreview?(shaver)
Attachment #235798 - Flags: review?(mrbkap)
Attachment #235798 - Flags: approval1.8.0.7?
(Assignee)

Comment 27

11 years ago
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
Attachment #235808 - Flags: review?(igor.bukanov)
(Assignee)

Comment 28

11 years ago
Created attachment 235809 [details]
testcase generator

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

/be
(Assignee)

Comment 29

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

11 years ago
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.
Group: security
Whiteboard: [schrep-181approval pending] → [sg:critical][schrep-181approval pending]
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
Attachment #235808 - Flags: approval1.8.0.7+
(Assignee)

Comment 32

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

Updated

11 years ago
Keywords: fixed1.8.0.7
(Assignee)

Comment 33

11 years ago
Created attachment 235826 [details] [diff] [review]
1.8 branch version of patch
Attachment #235826 - Flags: review?(shaver)
(Assignee)

Comment 34

11 years ago
Created attachment 235831 [details] [diff] [review]
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
Attachment #235831 - Flags: review?(shaver)
(Assignee)

Updated

11 years ago
Attachment #235597 - Flags: review+
Attachment #235597 - Flags: approval1.8.1+
(Assignee)

Updated

11 years ago
Attachment #235598 - Flags: review+
Attachment #235598 - Flags: approval1.8.0.7+
(Assignee)

Updated

11 years ago
Blocks: 349592
(Assignee)

Updated

11 years ago
Blocks: 350553
(Reporter)

Comment 35

11 years ago
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.
Attachment #235808 - Flags: review?(igor.bukanov) → review+
(Assignee)

Comment 36

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

Comment 37

11 years ago
Created attachment 235952 [details] [diff] [review]
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
Attachment #235952 - Flags: superreview?(shaver)
Attachment #235952 - Flags: review?(igor.bukanov)
(Assignee)

Updated

11 years ago
Attachment #235831 - Attachment is obsolete: true
Attachment #235831 - Flags: review?(shaver)
(Assignee)

Comment 38

11 years ago
Created attachment 235954 [details] [diff] [review]
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
Attachment #235826 - Attachment is obsolete: true
Attachment #235954 - Flags: superreview?(shaver)
Attachment #235954 - Flags: review?(igor.bukanov)
Attachment #235826 - Flags: review?(shaver)
(Reporter)

Comment 39

11 years ago
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?
Attachment #235954 - Flags: review?(igor.bukanov) → review-
Comment on attachment 235952 [details] [diff] [review]
updated successor trunk patch

sr=shaver
Attachment #235952 - Flags: superreview?(shaver) → superreview+
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].
Attachment #235954 - Flags: superreview?(shaver)
Attachment #235954 - Flags: superreview+
Attachment #235954 - Flags: review?
Attachment #235954 - Flags: review-
(Assignee)

Comment 42

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

Comment 43

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

New patches shortly.

/be
(Assignee)

Comment 44

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

Comment 45

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

Comment 46

11 years ago
Created attachment 235971 [details] [diff] [review]
successor trunk patch, v2
Attachment #235952 - Attachment is obsolete: true
Attachment #235971 - Flags: superreview+
Attachment #235971 - Flags: review?(igor.bukanov)
Attachment #235952 - Flags: review?(igor.bukanov)
(Assignee)

Comment 47

11 years ago
Created attachment 235972 [details] [diff] [review]
updated patch for 1.8 branch, v2
Attachment #235954 - Attachment is obsolete: true
Attachment #235972 - Flags: superreview+
Attachment #235972 - Flags: review?(igor.bukanov)
Attachment #235954 - Flags: review?
(Assignee)

Comment 48

11 years ago
And for once, bugzilla's interdiff works!

/be
(Reporter)

Comment 49

11 years ago
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]
Attachment #235971 - Flags: review?(igor.bukanov) → review+
(Assignee)

Comment 50

11 years ago
Created attachment 235976 [details]
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
(Assignee)

Comment 51

11 years ago
Created attachment 235977 [details] [diff] [review]
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
Attachment #235977 - Flags: review+
Attachment #235977 - Flags: approval1.8.0.8?
Attachment #235977 - Flags: approval1.8.0.7?
(Reporter)

Comment 52

11 years ago
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.
Attachment #235972 - Flags: review?(igor.bukanov) → review+
(Assignee)

Comment 53

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

Comment 54

11 years ago
Fixed on the trunk.

/be
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago11 years ago
Resolution: --- → FIXED

Comment 55

11 years ago
Created attachment 235997 [details]
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

11 years ago
Created attachment 235999 [details]
js1_5/Regress/regress-350312-01.js

from comment 1

Comment 57

11 years ago
Created attachment 236007 [details]
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

11 years ago
Created attachment 236023 [details]
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

11 years ago
Created attachment 236025 [details]
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 ?

Updated

11 years ago
Flags: in-testsuite+
(Assignee)

Comment 60

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

Comment 61

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

This is ready for 1.8.1.

/be
Attachment #235972 - Flags: approval1.8.1?

Updated

11 years ago
Attachment #235972 - Flags: approval1.8.1? → approval1.8.1+
(Assignee)

Comment 62

11 years ago
Fixed on the 1.8 branch.

/be
Keywords: fixed1.8.1

Comment 63

11 years ago
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
Status: RESOLVED → VERIFIED
Blocks: 350760
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
Attachment #235977 - Flags: approval1.8.0.7? → approval1.8.0.7-

Comment 65

11 years ago
verified fixed 1.8 20060831 windows/mac*/linux
Keywords: fixed1.8.1 → verified1.8.1

Comment 66

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

(Assignee)

Updated

11 years ago
Depends on: 350837

Updated

11 years ago
Attachment #235808 - Flags: review?(mrbkap) → review+
(Assignee)

Updated

11 years ago
Attachment #235977 - Flags: approval1.8.0.8?
(Assignee)

Updated

11 years ago
Attachment #235977 - Flags: approval1.8.0.8?

Comment 67

11 years ago
verified fixed 1.8.0.7 2006090118 windows/mac*/linux
Keywords: fixed1.8.0.7 → verified1.8.0.7
(Assignee)

Updated

11 years ago
Blocks: 352873
Attachment #235977 - Flags: approval1.8.0.8?
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
Attachment #235977 - Flags: approval1.8.0.9?
Attachment #235977 - Flags: approval1.8.0.8?
Attachment #235977 - Flags: approval1.8.0.8+
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.
Attachment #235977 - Flags: approval1.8.0.8+
Group: security

Comment 70

11 years ago
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
You need to log in before you can comment on or make changes to this bug.