Closed
Bug 348986
Opened 18 years ago
Closed 18 years ago
Missed recursion check in Decompile from jsopcode.c
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
VERIFIED
FIXED
mozilla1.8.1beta2
People
(Reporter: igor, Assigned: igor)
References
Details
(Keywords: verified1.8.0.7, verified1.8.1)
Attachments
(4 files)
1.58 KB,
patch
|
brendan
:
review+
dveditz
:
approval1.8.0.7+
beltzner
:
approval1.8.1+
|
Details | Diff | Splinter Review |
840 bytes,
text/plain
|
Details | |
4.42 KB,
text/plain
|
Details | |
3.04 KB,
patch
|
dveditz
:
approval1.8.0.7+
|
Details | Diff | Splinter Review |
Decompile from jsopcode.c is a recursive function but it does not check for a C stack overflow.
Assignee | ||
Comment 1•18 years ago
|
||
Assignee | ||
Updated•18 years ago
|
Flags: blocking1.8.1?
Flags: blocking1.8.0.7?
Assignee | ||
Comment 2•18 years ago
|
||
I will try to make a test case that can show the stack overflow without hitting a stack overflow in the parser.
Summary: Missing recursion check in Decompile from jsopcode.c → Missed recursion check in Decompile from jsopcode.c
Assignee | ||
Comment 3•18 years ago
|
||
Since the parser AFAICS consumes more stack space than the decompiler, the test case consists of 2 parts. First it builds the source of the form "function f(){function f() ..}}" and compiles it. Second it run another recursive code which at the recursion bottom calls toSource on the compiled function.
With the test case on i386 Ubuntu 6.06 after I set the stack limit to 510K and run the optimized build of jsshell assuming the deault internal stack limit for jsshell of 500000 bytes I got without the patch:
~> ulimit -s 510
...
~> ~/m/trunk/mozilla/js/src/Linux_All_OPT.OBJ/js ~/s/decompile_recursion_check.js
Segmentation fault
With the patch:
~> ulimit -s
510
...
~> ~/m/trunk/mozilla/js/src/Linux_All_OPT.OBJ/js ~/s/decompile_recursion_check.js
success=true
Comment 4•18 years ago
|
||
IIRC the parser avoids consuming linear stack space in number of chained if-then statements, while the decompiler calls itself recursively to decompile then parts. You might be able to use this asymmetry.
/be
Comment 5•18 years ago
|
||
Comment on attachment 234201 [details] [diff] [review]
Fix v1
r=me, thanks.
/be
Attachment #234201 -
Flags: review?(brendan) → review+
Assignee | ||
Comment 7•18 years ago
|
||
(In reply to comment #4)
> IIRC the parser avoids consuming linear stack space in number of chained
> if-then statements, while the decompiler calls itself recursively to decompile
> then parts. You might be able to use this asymmetry.
Here is the relevant code from Statement in jsparse.c:
Statement(JSContext *cx, JSTokenStream *ts, JSTreeContext *tc)
{
...
case TOK_IF:
/* An IF node has three kids: condition, then, and optional else. */
pn = NewParseNode(cx, ts, PN_TERNARY, tc);
if (!pn)
return NULL;
pn1 = Condition(cx, ts, tc);
if (!pn1)
return NULL;
js_PushStatement(tc, &stmtInfo, STMT_IF, -1);
pn2 = Statement(cx, ts, tc);
if (!pn2)
return NULL;
if (js_MatchToken(cx, ts, TOK_ELSE)) {
stmtInfo.type = STMT_ELSE;
pn3 = Statement(cx, ts, tc);
I.e. it recurses exactly in the same way as Decompile does. Moreover, number of locals in both functions are on the same scale and I do not want a test case to depend on the ability of the compiler to optimize them.
Comment 8•18 years ago
|
||
Igor, is this ready for the trunk?
Comment 9•18 years ago
|
||
Checking in regress-348986.js;
/cvsroot/mozilla/js/tests/js1_5/Function/regress-348986.js,v <-- regress-348986.js
initial revision: 1.1
Flags: in-testsuite+
Assignee | ||
Comment 10•18 years ago
|
||
(In reply to comment #8)
> Igor, is this ready for the trunk?
>
Yes, but the tinderbox is red. Or should I just consider Firefox tinderbox?
Comment 11•18 years ago
|
||
(In reply to comment #10)
Except for balsa on seamonkey-ports, it *looks* green to me, but wait until you are ready and sure everything is ok. I'll kill my builds and wait for you to check in later.
Assignee | ||
Comment 12•18 years ago
|
||
This test case deduces the stack limit automatically and prepare the sources in a such way so stack a overflow exception must be thrown.
Assignee | ||
Comment 13•18 years ago
|
||
I committed the patch from comment 1 to the trunk.
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•18 years ago
|
Attachment #234201 -
Flags: approval1.8.1?
Comment 14•18 years ago
|
||
Checking in regress-348986.js;
/cvsroot/mozilla/js/tests/js1_5/Function/regress-348986.js,v <-- regress-348986.js
new revision: 1.2; previous revision: 1.1
with minor edits.
Comment 15•18 years ago
|
||
(In reply to comment #7)
> I.e. it recurses exactly in the same way as Decompile does. Moreover, number of
> locals in both functions are on the same scale and I do not want a test case to
> depend on the ability of the compiler to optimize them.
Argh, I was thinking of the code generator. Sorry, have to do better next time with by-memory shooting from the hip.
/be
Comment 16•18 years ago
|
||
Comment on attachment 234201 [details] [diff] [review]
Fix v1
a=beltzner on behalf of drivers.
Attachment #234201 -
Flags: approval1.8.1? → approval1.8.1+
Updated•18 years ago
|
Target Milestone: --- → mozilla1.8.1beta2
Comment 17•18 years ago
|
||
(In reply to comment #12)
> Created an attachment (id=234404) [edit]
> Better test case
>
> This test case deduces the stack limit automatically and prepare the sources in
> a such way so stack a overflow exception must be thrown.
Clever! The testsuite could use more of this kind of auto-sensing goodness.
/be
Target Milestone: mozilla1.8.1beta2 → ---
Updated•18 years ago
|
Target Milestone: --- → mozilla1.8.1beta2
Updated•18 years ago
|
Flags: blocking1.8.0.7? → blocking1.8.0.7+
Comment 18•18 years ago
|
||
Comment on attachment 234201 [details] [diff] [review]
Fix v1
approved for 1.8.0 branch, a=dveditz for drivers
Attachment #234201 -
Flags: approval1.8.0.7+
Updated•18 years ago
|
Whiteboard: [checkin needed (1.8 branch)]
Assignee | ||
Comment 19•18 years ago
|
||
I committed the patch from comment 1 to MOZILLA_1_8_BRANCH
Assignee | ||
Updated•18 years ago
|
Keywords: fixed1.8.1
Assignee | ||
Comment 20•18 years ago
|
||
The version for 1.8.0 branch that removes 3 junk trailing white spaces to stay close to post 1.8.0 versions.
Attachment #234516 -
Flags: approval1.8.0.7?
Comment 21•18 years ago
|
||
Comment on attachment 234516 [details] [diff] [review]
Fix for 1.8.0 branch
a=dveditz
Attachment #234516 -
Flags: approval1.8.0.7? → approval1.8.0.7+
Assignee | ||
Comment 22•18 years ago
|
||
I committed the patch from comment 20 to MOZILLA_1_8_0_BRANCH
Assignee | ||
Updated•18 years ago
|
Keywords: fixed1.8.0.7
Comment 23•18 years ago
|
||
verified fixed 1.8, 1.9 20060821 windows/mac*/linux
Status: RESOLVED → VERIFIED
Keywords: fixed1.8.1 → verified1.8.1
Comment 24•18 years ago
|
||
verified fixed 1.8.0.7 20060824 windows/mac*/linux no crash
JavaScript Error: ./js1_5/Function/regress-348986.js:64: too much recursion.
Updated•18 years ago
|
Keywords: fixed1.8.0.7 → verified1.8.0.7
Updated•18 years ago
|
Whiteboard: [checkin needed (1.8 branch)]
You need to log in
before you can comment on or make changes to this bug.
Description
•