Closed
Bug 237006
Opened 21 years ago
Closed 15 years ago
API for address of first stack variable to prevent Mozilla crashes
Categories
(Core :: JavaScript Engine, enhancement)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
People
(Reporter: igor, Assigned: igor)
References
Details
(Whiteboard: fixed-in-tracemonkey)
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
igor
:
review+
|
Details | Diff | Splinter Review |
To defend against a stack overflow caused by specially crafted JavaScript, jseng
now provides API to set boundary address for stack consummation, see bug 192414.
By default the checks are turned off and it is responsibility of jseng user to
call JS_SetThreadStackLimit with the correct information, see bug 220408 .
Now the problem is how to get this maximum allowed stack address as it seems it
is not very well documented subject on any platform. If NSPR would provide such
API it would be great, but past experience tells that it would not happen soon
(the original bug was reported more then a year).
On other hand if the minimal stack address is known with precision of 10K or so,
then adding 100K to it would provide a reasonable guess that would work out of
the box in the vast majority of cases.
Obtaining the imprecise minimal address is also trivial for any NSPR thread as
it will always have some code near beginning of the stack that calls the
supplied thread callback and returning the address of the first declared NSPR
variable should be sufficient to prevent crashing Mozilla with trivial scripts
(like attachment 113927 [details] which I just used to verify that it still crashes
Mozilla 1.6).
With such API it would be simple to close bug 220408 .
Actually, what's more important then the start of the stack is the size of the
stack. Also some NSPR define for getting the direction the stack grows would be
nice, although JS does supply us with that already.
Assignee | ||
Comment 2•21 years ago
|
||
(In reply to comment #1)
> Actually, what's more important then the start of the stack is the size of the
> stack. Also some NSPR define for getting the direction the stack grows would be
> nice, although JS does supply us with that already.
JS needs to know the maximum (or minimum if stack growth down) allowed address
to detect run away recursion so if the stack size information is available it
would allow to calculate the limit properly. The problem is that it seems that
this information is extremely platform dependent and for over a year nobody
suggested a code that works reliably.
On the other hand a hard coded limit like no more then 500K of stack space
covers most of interested cases and would prevent one-line scripts to crash
Mozilla or any other JS embedding that allows to execute untrusted scripts. Now
for the limit to work reliably one would need to know initial stack address but
that is trivial to expose from NSPR without any platform-specific code as it is
just an address of first variable in NSPR thread callback. It is this practical
considerations that caused me to limit the scope of this enhancement.
Updated•18 years ago
|
QA Contact: wtchang → nspr
Comment 3•15 years ago
|
||
See bug 516832 comment 34.
/be
Assignee | ||
Comment 4•15 years ago
|
||
This is based on Gregor's patch from the bug 516832.
Assignee: wtc → igor
Attachment #441340 -
Flags: review?(anygregor)
Updated•15 years ago
|
Component: NSPR → JavaScript Engine
Product: NSPR → Core
QA Contact: nspr → general
Version: other → unspecified
Updated•15 years ago
|
Attachment #441340 -
Flags: review?(anygregor) → review+
Assignee | ||
Comment 5•15 years ago
|
||
v1 could not compile on Mac due to the bug 564414. So while fixing the bug there I also added better comments here.
Attachment #441340 -
Attachment is obsolete: true
Attachment #444310 -
Flags: review+
Assignee | ||
Updated•15 years ago
|
Assignee | ||
Comment 6•15 years ago
|
||
Whiteboard: fixed-in-tracemonkey
Comment 7•15 years ago
|
||
Igor, is this possibly a fallout?
42271 INFO TEST-PASS | /tests/content/html/content/test/test_formSubmission.html | Correct number of items
Assertion failure: cx->requestDepth > 0, at /builds/slave/tracemonkey-linux-debug/build/js/src/jsgc.cpp:2336
++DOMWINDOW == 18 (0xc617d6c) [serial = 1016] [outer = 0xa922bae8]
NEXT ERROR 42272 ERROR TEST-UNEXPECTED-FAIL | /tests/content/html/content/test/test_formSubmission.html | [SimpleTest/SimpleTest.js, window.onerror] An error occurred - JSON.parse at http://mochi.test:8888/tests/content/html/content/test/test_formSubmission.html:579
JavaScript error: http://mochi.test:8888/tests/content/html/content/test/test_formSubmission.html, line 579: JSON.parse
Assignee | ||
Comment 8•15 years ago
|
||
(In reply to comment #7)
> 42271 INFO TEST-PASS |
> /tests/content/html/content/test/test_formSubmission.html | Correct number of
> items
> Assertion failure: cx->requestDepth > 0, at
It is hard to see what in the patch could have caused that as it does not change anything in the request-related code.
Assignee | ||
Comment 9•15 years ago
|
||
(In reply to comment #7)
> Assertion failure: cx->requestDepth > 0, at
> /builds/slave/tracemonkey-linux-debug/build/js/src/jsgc.cpp:2336
The assert is in the js_TriggerGC call. Most likely that comes from JSContext::checkMallocGCPressure called from JSContext::malloc. Which points that the assertion is bogus and should be replaced by the real check - JSContext::malloc could be called outside the request.
Comment 10•15 years ago
|
||
Want to file a bug for that?
Comment 11•15 years ago
|
||
Never mind you just did. Bug 566949.
Comment 12•15 years ago
|
||
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•