Closed Bug 271477 Opened 21 years ago Closed 21 years ago

C stack overflow with js_obj_toSource

Categories

(Core :: JavaScript Engine, defect, P3)

x86
Windows XP
defect

Tracking

()

RESOLVED FIXED
mozilla1.8alpha6

People

(Reporter: jband_mozilla, Assigned: brendan)

Details

(Keywords: js1.5)

Attachments

(2 files)

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.
Attached file stack
note that the top frame is just where it happened to overflow.
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
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!
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → INVALID
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
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
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.
Easy to fix, patch next. /be
Status: REOPENED → ASSIGNED
Keywords: js1.5
Priority: -- → P3
Target Milestone: --- → mozilla1.8alpha6
Attached patch proposed fixSplinter Review
Probably checking in today with rs=jband. /be
Comment on attachment 166966 [details] [diff] [review] proposed fix Cool. I still get to do a rs=jband? Yes. I buy this. Thanks.
Attachment #166966 - Flags: superreview+
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
Fixed. Thanks! /be
Status: ASSIGNED → RESOLVED
Closed: 21 years ago21 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: