27767 - stacksize in JS_NewContext() not respected

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P3)

Description

[chouck]

From Bug Helper:
User-Agent: Mozilla/4.7 [en] (WinNT; U)
BuildID:    
It looks like the stack size we are passing into JS_NewContext is 
not getting used correctly.  Its being used to set the size of the
arenas to use, but it doesn't limit the number of arenas that actually
get created.

We had some code that was recursing but had large JS stacks.  Since
the JS-stack size was growing infinitely we ended up overrunning the
C-stack.

Reproducible: Always
Steps to Reproduce:
1. Go into jsinterp.c and change the defn of MAX_INTERP_LEVEL to
   something like 100000 (from 1000).

2. Run the JS shell:

js> function f(v) { print(v); f(v   1); }
js> f(0)

Actual Results:  On NetBSD this will count up to 3224 and then dump core.
Expected Results:  It should have, at somepoint, hit the error condition in 
js_AllocStack() and popped up a helpful error message instead of
crashing.

Comment 1

[Mike McCabe]

Passing to Roger.

Comment 2

[Brendan Eich [:brendan]]

The argument to JS_NewContext is an arena size, not an upper bound on the stack 
arena pool.  It never was, and it should not be changed to a total upper bound. 
It's there so savvy API users can avoid wasting space ("internal fragmentation") 
by using small chunks, if they know most scripts don't need more than one chunk 
(arena, I mean).

The problem where you crash overflowing the C stack is harder to fix.  It would 
be better to fix directly, by C stack limit querying (getrlimit on Unix, e.g.) 
and safening, or by adding a VM "redzone".

But the JS interpreter should not use a C stack frame (let alone two frames) for 
every scripted lightweight (!JSFUN_HEAVYWEIGHT) function call!  I have fixed it 
to avoid such piggish behavior.  See next attachment.  If someone gives me an r= 
I will check the patch in.

/be

Comment 3

[chouck]

Whaaa.  If that's actually the case, it would seem that the second
parameter to JS_NewContext() should be renamed, its not really the
"stacksize" but really "stack_arena_chunk_size" or something.

In addition, the documentation at:
http://developer.netscape.com/docs/manuals/javascriptapi/index.htm
is wrong:

> A context specifies a stack size for the script, the amount, in 
> bytes, of private memory to allocate to the execution stack for 
> the script.

-Chris

Comment 4

[Brendan Eich [:brendan]]

Adding cc: buddies or interested 3rd parties.

/be

Comment 5

[Mike Shaver (:shaver emeritus)]

Um, what attachment?

Comment 6

[Brendan Eich [:brendan]]

Wahhh!  Write a letter to the doc owner ([e-mail]).  If mccabe or 
anyone wants to rename the stacksize formal parameter to JS_NewContext to such a 
name as stackChunkSize, that'd be cool.

/be

Comment 7

[Brendan Eich [:brendan]]

Waitforit!  I'm re-diffing after cvs update right now.  cvs diff -u output 
coming soon -- are we there yet? (justalittlefurther)

/be

Comment 8

[Brendan Eich [:brendan]]

Attachment: patch to avoid consuming C stack for every JS function call

Comment 9

[Brendan Eich [:brendan]]

There are other recursive procedures in the JS engine that need safety checks to 
avoid crashing on C stack overflow, or just to avoid using too much C stack on 
machines that have a small-ish stack hardware or OS-enforced segment size.  In 
rough order of criticality:

- gc_mark (AKA gc_mark_node) in jsgc.c
- js_obj_toSource in jsobj.c
- Decompile and related functions in jsopcode.c

Anyone (djw?) game or already involved in patches to fix these?

/be

Comment 10

[Brendan Eich [:brendan]]

Still looking for help from avantgo folks.  For gc_mark, rather than a safety 
check to avoid crashing on stack overflow, the code should be converted to use a 
arena-amortized, malloc'ed queue.  Standard iterative mark phase trick.  Who's 
up for it?  I'm on sabbatical!

/be

Comment 11

[Brendan Eich [:brendan]]

Attachment: better patch, fixes exception thrown across inline call

Comment 12

[Brendan Eich [:brendan]]

Notes on latest patch:

- Major change from before: inline call and return cases to avoid recursive
  activation of js_Interpret via js_Invoke for lightweight functions.  Fixed
  in this patch to respect !ok in the inline_return: code, which arises when
  an exception is thrown but not caught within the activation of a lightweight
  function.
- Move js_Invoke()'s JSVAL_IS_FUNCTION re-test into block that tried
  clasp->convert, as there is no reason to re-test unconditionally in the
  successor block, in the case where control doesn't flow into the
  convert-attempting block.
- Prefer jsint to ptrdiff_t, which burned us on Win16 (where, apparently in
  full ANSI C compliance, it fails to have enough signed bits to express all
  pointer differences!) in js_Interpret.
- Use JUMP_OFFSET_LEN and ATOM_INDEX_LEN from jsopcode.h rather than magic 2.
- Comment and whitespace fussing.

Comment 13

[Brendan Eich [:brendan]]

Attachment: fixed JSInlineFrame initialization order blunder

Comment 14

[Brendan Eich [:brendan]]

Sometimes I stare at code long enough to see through a blind spot.

Anyone out there using this code yet?  I was, without all my testing (including 
full Mozilla runs) disclosing the bug in the next-to-last patch.  Waterson, any 
qfy results?

/be

Comment 15

[Mike Shaver (:shaver emeritus)]

Should we stick this on a branch so we can get to it more easily?  I volunteer
Rob to cut the branch. =)

Comment 16

[Brendan Eich [:brendan]]

Attachment: added MAX_INTERP_LEVEL check to limit recursion

Comment 17

[Brendan Eich [:brendan]]

Anyone had time to play with this yet?  Here's another patch.  MAX_INTERP_LEVEL 
is 1000 for all but Win16, where it was set empirically (to avoid crashing on 
64K stack seg overflow) a while ago (4.x daze).  Maybe it should be a runtime 
tuneable?

/be

Comment 18

[Brendan Eich [:brendan]]

Attachment: better, separate inline-call-count recursion limit

Comment 19

[Brendan Eich [:brendan]]

My hasty previous patch wrongly reused MAX_INTERP_LEVEL to limit inline_call: 
iteration and heap use for JSInlineFrames.  This patch uses a separate constant, 
MAX_INLINE_CALL_COUNT.  It's set at 1000, because that must have been generous 
enough lo these many years (when MAX_INTERP_LEVEL was 1000 on all platforms but 
Win16, and all function activations recursively actived js_Interpret, invoking 
the MAX_INTERP_LEVEL check).  But MAX_INLINE_CALL_COUNT is independent of Win16 
or other platform conditioning.

MAX_INTERP_LEVEL still sucks in that it must be tuned to avoid crashing on stack 
overflow, but that's a lesser problem.  Anyone care to bash on this patch with 
some highly recursive lightweight and heavyweight (use a with statement to make 
a function heavy, or use eval -- but with is faster) runaways?

/be

Comment 20

[Brendan Eich [:brendan]]

Just to bludgeon the dying horse, an overdesigned recursion limiter might sum 
cx->interpLevel and all the local inlineCallCount variables from each activation 
of js_Interpret (all cx->interpLevel activations), and limit that number -- but 
it doesn't seem worth it.

Either you run out of C stack and crash, which means that MAX_INTERP_LEVEL 
should be tuned for your platform (or someone should hack up a getrlimit-based 
or redzone-based solution), or you recur through too many lightweight functions 
and hit MAX_INLINE_CALL_COUNT.  The weird case where indirect recursion via a 
heavyweight function means you use some C stack (but don't hit MAX_INTERP_LEVEL) 
and lots of inline-frame heap (but don't hit MAX_INLINE_CALL_COUNT for any given 
activation of js_Interpret) sounds like a don't-care case: the indirectly 
recursive set of functions must converge eventually.  Only diverging functions 
will hit one limit or the other.

/be

Comment 21

[rogerl (gone)]

Brendan - 
I applied the patch and ran the test suite - one new failure in 
ecma/String/15.5.4.6-2.js which boils down to :

function f() { return this; }
function g() { var h = f; return h(); }
g()

should print [object global], but gets null instead. I think that's because we 
just pull the 'this' value off the stack and go. There's a bunch of code in 
js_invoke to set the thisp of the new frame that needs to be duplicated into the 
inline call sequence.

Comment 22

[Brendan Eich [:brendan]]

Attachment: best patch ever, I swear!

Comment 23

[Brendan Eich [:brendan]]

Thanks -- I fixed that by subroutining the ugly 'this' parameter computation and 
calling it from js_Invoke and inline_call.  I also fixed a buglet in the inline 
call case, where newsp was not restored after a js_AllocStack failure.  Stick a 
fork in me, I'm done!

/be

Comment 24

[rogerl (gone)]

OK, new patch passes tests fine.

Comment 25

[rogerl (gone)]

Want me to check this in (r=brendan) or t'other way around?

Comment 26

[Brendan Eich [:brendan]]

I'll take the cvsblame, r=[e-mail] -- thanks.

/be

Comment 27

[Brendan Eich [:brendan]]

All checked in, including the stacksize=>stackChunkSize parameter renaming.  
That should carry over into the doc -- rginda, can you handle that?

Waterson, didja ever get qfy A-B comparison data?

/be