Closed Bug 342854 Opened 18 years ago Closed 18 years ago

Missed call to JS_SetThreadStackLimit

Categories

(Core :: XPConnect, defect)

x86
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: igor, Assigned: igor)

References

Details

(Keywords: fixed1.8.1)

Attachments

(1 file)

This is a spin-off of bug 342737:

Currently test cases that checks defence against stack overflow in JS like in bug
324278 fail in the browser when running with jsd debugger. It happens because the debugger triggers invocation to JS_NewContext without corresponding JS_SetThreadStackLimit call.  According to http://lxr.mozilla.org/seamonkey/ident?i=JS_NewContext only dom/src/base/nsJSEnvironment.cpp sets the stack limit, the rest of the calls to JS_NewContext are not protected with this.
The fix uses just introduced JS_SetContextCallback to set a callback that will always set the stack limit. The stack limit is calculated for each thread separately and is stored in the thread-local data.
Assignee: dbradley → igor.bukanov
Status: NEW → ASSIGNED
Attachment #227237 - Flags: review?(dbradley)
Prolly want jst reviewing this.

/be
Attachment #227237 - Flags: superreview?(jst)
Comment on attachment 227237 [details] [diff] [review]
Fix using JSContext callback

sr=jst
Attachment #227237 - Flags: superreview?(jst) → superreview+
Ping: any chance to get a review?
Comment on attachment 227237 [details] [diff] [review]
Fix using JSContext callback

Let's get this in.
Attachment #227237 - Flags: review+
Attachment #227237 - Flags: review?(dbradley)
Attachment #227237 - Flags: review+
Attachment #227237 - Flags: approval1.8.1?
I committed to the trunk the patch from comment 1.
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
This patch is straightforward as a code change, and the benefit is that it provides universal stack overflow checking in Gecko embeddings, resolving Venkman and possibly other crash-on-stack-overflow (D.O.S.) bugs.  I think we should take it for 1.8.1/Firefox 2.

/be
Comment on attachment 227237 [details] [diff] [review]
Fix using JSContext callback

approved by schrep for drivers
Attachment #227237 - Flags: approval1.8.1? → approval1.8.1+
I committed the patch from comment 1 to MOZILLA_1_8_BRANCH.
Keywords: fixed1.8.1
*** Bug 307451 has been marked as a duplicate of this bug. ***
I'm seeing a problem related to this, I think. js_Interpret thinks we're out of
stack space.  

  cx->stackLimit: afa7c
  &stackDummy:    af348

The current stackDummy is under the limit, so the test fails (windoze, stack growth
is towards 0).

For cx->stackLimit to be afa7c, the stack address when the limit was set must have been:

  afa7c + 80000 = 12fa7c

I don't know where the stack address of 12fa7c came from. I'm on the main thread, and looking
at the stuff at the bottom of the call stack, I see their stack locals have addresses
around b2600. 

There's no way I am anywhere near exceeding the stack limit. What's going on? How did my 
cx->stackLimit get set to something that doesn't seem to correspond to my stack?
Anyone else seeing anything funny in this area?
(In reply to comment #11)
> How did my 
> cx->stackLimit get set to something that doesn't seem to correspond to my
> stack?

The stack limit is set at
 http://lxr.mozilla.org/seamonkey/source/js/src/xpconnect/src/xpcthreadcontext.cpp#408

Could you check if that is properly initialized to what you see later?
Ignore comment #11. The stack overflow code is working. I really do have 1000's
of call frames on the stack. I have a different problem :-)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: