271477 - C stack overflow with js_obj_toSource

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P3)

Description

[John Bandhauer]

I has trying to reproduce a crash and discovered that the following use of the
scripts in the js/tests suite will overflow the C stack...

load("js1_2/shell.js");
load("js1_2/String/match.js");
load("ecma/shell.js");
load("ecma/String/15.5.2.js");

I can reproduce this in the trunk jsshell on Win32 by putting the 4 lines above
in a file (blowthestack.js) and running the jsshell in the js/tests directory like:

  ..\src\Debug\jsshell.exe blowthestack.js

From the command line on Windows this will just do an early abort. In the
debugger it will stop with a stack overflow exception.

I'll attach a stack excerpt.

FWIW, this does not look anything like the problem I was trying to reproduce.

Comment 1

[John Bandhauer]

Attachment: stack

note that the top frame is just where it happened to overflow.

Comment 2

[Brendan Eich [:brendan]]

jband: are you guys using JS_SetThreadStackLimit?  Prolly not.  If you were,
then the JS_CHECK_STACK_SIZE call at the top of js_obj_toSource should save you.

If you aren't using this API, can you INVALIDate this bug?  Thanks,

/be

Comment 3

[John Bandhauer]

Fair enough. We are not calling JS_SetThreadStackLimit. But, I doubt our product
has ever seen this problem. I just happened to hit it in some test code. I
didn't realize that it would be considered ok for this to occur in non-evil
code. But, since there is a mechaninsm for protection... Marking Invalid.

Thanks!

Comment 4

[Brendan Eich [:brendan]]

Oh, argh.  Maybe I should WONTFIX this.  It's a true runaway recursion, not a
deep one induced by JS1.2 + a hard-case test.  It's ugly, and the right answer
is death to 1.2 (bug 255895).

/be

Comment 5

[timeless]

fwiw, i'm pretty sure that phil reported that the stack limit used by xpcshell
and friends wasn't correct on windows, but i haven't had time to look for it.

Comment 6

[Brendan Eich [:brendan]]

Easy to fix, patch next.

/be

Comment 7

[Brendan Eich [:brendan]]

Attachment: proposed fix

Probably checking in today with rs=jband.

/be

Comment 8

[John Bandhauer]

Comment on attachment 166966 [details] [diff] [review]
proposed fix

Cool. I still get to do a rs=jband?

Yes. I buy this. Thanks.

Comment 9

[Brendan Eich [:brendan]]

Comment on attachment 166966 [details] [diff] [review]
proposed fix

js/src is single-review, and rs stands for rubberstamp, but sr downgrades to r,
so cool! ;-)

/be

Comment 10

[Brendan Eich [:brendan]]

Fixed.  Thanks!

/be