Closed
Bug 271477
Opened 21 years ago
Closed 21 years ago
C stack overflow with js_obj_toSource
Categories
(Core :: JavaScript Engine, defect, P3)
Tracking
()
RESOLVED
FIXED
mozilla1.8alpha6
People
(Reporter: jband_mozilla, Assigned: brendan)
Details
(Keywords: js1.5)
Attachments
(2 files)
9.62 KB,
text/plain
|
Details | |
941 bytes,
patch
|
jband_mozilla
:
superreview+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•21 years ago
|
||
note that the top frame is just where it happened to overflow.
Assignee | ||
Comment 2•21 years ago
|
||
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
Reporter | ||
Comment 3•21 years ago
|
||
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
Assignee | ||
Comment 4•21 years ago
|
||
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.
Assignee | ||
Comment 6•21 years ago
|
||
Easy to fix, patch next.
/be
Assignee | ||
Comment 7•21 years ago
|
||
Probably checking in today with rs=jband.
/be
Reporter | ||
Comment 8•21 years ago
|
||
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+
Assignee | ||
Comment 9•21 years ago
|
||
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
Assignee | ||
Comment 10•21 years ago
|
||
Fixed. Thanks!
/be
Status: ASSIGNED → RESOLVED
Closed: 21 years ago → 21 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•