Closed Bug 237006 Opened 17 years ago Closed 11 years ago

API for address of first stack variable to prevent Mozilla crashes

Categories

(Core :: JavaScript Engine, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: igor, Assigned: igor)

References

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 file, 1 obsolete file)

v2
24.31 KB, 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.
(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.

QA Contact: wtchang → nspr
Attached patch v1 (obsolete) — Splinter Review
This is based on Gregor's patch from the bug 516832.
Assignee: wtc → igor
Attachment #441340 - Flags: review?(anygregor)
Component: NSPR → JavaScript Engine
Product: NSPR → Core
QA Contact: nspr → general
Version: other → unspecified
Attachment #441340 - Flags: review?(anygregor) → review+
Attached patch v2Splinter Review
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+
Blocks: 516832
Depends on: 564414
http://hg.mozilla.org/tracemonkey/rev/a01307d7ba0e
Whiteboard: fixed-in-tracemonkey
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
(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.
(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.
Want to file a bug for that?
Never mind you just did. Bug 566949.
Blocks: 567937
http://hg.mozilla.org/mozilla-central/rev/a01307d7ba0e
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Depends on: 568996
Blocks: 569145
Blocks: 609626
No longer blocks: 609626
Blocks: 609626
You need to log in before you can comment on or make changes to this bug.