Missed recursion check in Decompile from jsopcode.c

VERIFIED FIXED in mozilla1.8.1beta2

Status

()

VERIFIED FIXED
12 years ago
12 years ago

People

(Reporter: igor, Assigned: igor)

Tracking

({verified1.8.0.7, verified1.8.1})

Trunk
mozilla1.8.1beta2
verified1.8.0.7, verified1.8.1
Points:
---
Bug Flags:
blocking1.8.1 +
blocking1.8.0.7 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments)

(Assignee)

Description

12 years ago
Decompile from jsopcode.c is a recursive function but it does not check for a C stack overflow.
(Assignee)

Comment 1

12 years ago
Created attachment 234201 [details] [diff] [review]
Fix v1
Assignee: general → igor.bukanov
Status: NEW → ASSIGNED
Attachment #234201 - Flags: review?(brendan)
(Assignee)

Updated

12 years ago
Flags: blocking1.8.1?
Flags: blocking1.8.0.7?
(Assignee)

Updated

12 years ago
Blocks: 329530
(Assignee)

Comment 2

12 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

12 years ago
Created attachment 234212 [details]
Test case for jsshell

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
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 on attachment 234201 [details] [diff] [review]
Fix v1

r=me, thanks.

/be
Attachment #234201 - Flags: review?(brendan) → review+

Comment 6

12 years ago
Putting on the 1.8.1/FF2 radar...
Flags: blocking1.8.1? → blocking1.8.1+
(Assignee)

Comment 7

12 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

12 years ago
Igor, is this ready for the trunk?

Comment 9

12 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

12 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

12 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

12 years ago
Created attachment 234404 [details]
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.
(Assignee)

Comment 13

12 years ago
I committed the patch from comment 1 to the trunk.
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
(Assignee)

Updated

12 years ago
Attachment #234201 - Flags: approval1.8.1?

Comment 14

12 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.
(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 on attachment 234201 [details] [diff] [review]
Fix v1

a=beltzner on behalf of drivers.
Attachment #234201 - Flags: approval1.8.1? → approval1.8.1+
Target Milestone: --- → mozilla1.8.1beta2
(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 → ---
Target Milestone: --- → mozilla1.8.1beta2
Flags: blocking1.8.0.7? → blocking1.8.0.7+
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

12 years ago
Whiteboard: [checkin needed (1.8 branch)]
(Assignee)

Comment 19

12 years ago
I committed the patch from comment 1 to MOZILLA_1_8_BRANCH
(Assignee)

Updated

12 years ago
Keywords: fixed1.8.1
(Assignee)

Comment 20

12 years ago
Created attachment 234516 [details] [diff] [review]
Fix for 1.8.0 branch

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 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

12 years ago
I committed the patch from comment 20 to MOZILLA_1_8_0_BRANCH
(Assignee)

Updated

12 years ago
Keywords: fixed1.8.0.7

Comment 23

12 years ago
verified fixed 1.8, 1.9 20060821 windows/mac*/linux
Status: RESOLVED → VERIFIED
Keywords: fixed1.8.1 → verified1.8.1

Comment 24

12 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

12 years ago
Keywords: fixed1.8.0.7 → verified1.8.0.7
Whiteboard: [checkin needed (1.8 branch)]
You need to log in before you can comment on or make changes to this bug.