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)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: igor, Assigned: igor)

References

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 file, 1 obsolete file)

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) (deleted) — 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 v2 (deleted) — Splinter 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
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
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Depends on: 568996
Blocks: 569145
No longer blocks: 609626
Blocks: 609626
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: