Closed
Bug 46196
Opened 24 years ago
Closed 24 years ago
JS stack overflow in JS_ReportOutOfmemory
Categories
(Core :: JavaScript Engine, defect, P2)
Tracking
()
VERIFIED
FIXED
People
(Reporter: alldritt, Assigned: rogerl)
References
Details
(Keywords: crash, js1.5, Whiteboard: [rtm-] fixed in trunk)
Attachments
(15 files)
11.85 KB,
patch
|
Details | Diff | Splinter Review | |
14.32 KB,
patch
|
Details | Diff | Splinter Review | |
11.85 KB,
patch
|
Details | Diff | Splinter Review | |
11.90 KB,
patch
|
Details | Diff | Splinter Review | |
17.17 KB,
patch
|
Details | Diff | Splinter Review | |
17.18 KB,
patch
|
Details | Diff | Splinter Review | |
11.92 KB,
patch
|
Details | Diff | Splinter Review | |
17.19 KB,
patch
|
Details | Diff | Splinter Review | |
11.92 KB,
patch
|
Details | Diff | Splinter Review | |
12.13 KB,
patch
|
Details | Diff | Splinter Review | |
17.40 KB,
patch
|
Details | Diff | Splinter Review | |
12.14 KB,
patch
|
Details | Diff | Splinter Review | |
17.41 KB,
patch
|
Details | Diff | Splinter Review | |
12.43 KB,
patch
|
Details | Diff | Splinter Review | |
17.59 KB,
patch
|
Details | Diff | Splinter Review |
There is a logic error in the JS_ReportOutOfMemory function that can lead to a infinite recursion stack overflow crash. The problem is that JS_ReportOutOfMemory indirectly calls JS_strdup which can call JS_ReportOutOfMemory if there is insufficient memory to duplicate the "out of memory" string. Note also that JS_ReportOutOfMemory indirectly calls js_ReportErrorVA which allocates additional memory. I found this on a Macintosh where out of memory conditions are common, but the problem could occure on any pla
Comment 1•24 years ago
|
||
I've seen JS_ReportOutOfmemory crashes before. This report is well-written. Confirming. One example of such a crash is bug 40802. A problem like this was also mentioned in bug 39125.
Reporter | ||
Comment 2•24 years ago
|
||
Oh, I'm using the jsref miplementation found in the M16 tarball. I've found a series of other low memory problems with M16's jsref: - jsexn.c's exn_initPrivate is not defensive against js_malloc returning null - jscntxt.c's ReportError can call js_ErrorToException with a NULL reportp parameter which js_ErrorToException explicitly asserts against
Reporter | ||
Comment 3•24 years ago
|
||
Another low memory problem related to JS_ReportOutOfmemory: - jscntxt.c's js_ExpandErrorArguments is not defensive against js_strdup returning null when setting messagep - causes to crash under low memory conditio
Comment 4•24 years ago
|
||
cc'ing Brendan -
Comment 5•24 years ago
|
||
Argh, JS is defensive about malloc wrappers returning null in general. This is a must-fix for js1.5. Roger, do you have time for it soon? /be
Keywords: js1.5
Comment 6•24 years ago
|
||
Is 41727 a dup of part of this bug? /be
Assignee | ||
Comment 8•24 years ago
|
||
OK, so if out of memory situations during error reporting: - should the out of memory error override any error in process? - should we pre-allocate a buffer to use for the exception object & message so that we guarantee not to cause further errors during error reporting?
Comment 10•24 years ago
|
||
As per discussion with JS Engine team, setting P2 priority and rtm keyword
Keywords: rtm
Priority: P3 → P2
Assignee | ||
Comment 11•24 years ago
|
||
Assignee | ||
Comment 12•24 years ago
|
||
The above patch attempts to handle the cases in expandErrorArguments and exn_initPrivate where an allocation fails. But the recursive death situation still exists. Brendan: are you saying that JS_malloc should NOT call JS_ReportOutOfMemory so that we never end up with recursion? Amother alternative would be for JS_ReportOutOfMemory to detect that an error report was in process (setting a flag in the context) and not calling JS_ReportErrorNumber.
Comment 13•24 years ago
|
||
I haven't reviewed the patch yet, but no, I don't propose that JS_malloc stop reporting an error that it exists to report (otherwise it's just malloc). What I think: the errors as exceptions work should include a pre-allocated exception (and whatever other data structures are needed) to use when out of memory. Can you work that into a new patch? (Quick nit-pick, I did spot this in a peek at the patch: newPrivate and newReport are uselessly initialized now.) /be
Assignee | ||
Comment 14•24 years ago
|
||
So I added a 'JSObject *outOfMemoryException' to Context, and initialized it in initExceptionClass, only that isn't getting run because of the new lazy initialization stuff. Is it going to mess things up if I require Exceptions to be initialized always?
Comment 15•24 years ago
|
||
Hrm. I don't like initializing the Exception classes early, because that will drag in Function and Object, in addition to the six or seven Error ones. Argh. Alternative idea: if you can't allocate an exception for Out of memory, just do the report and unwind. Too bad. How's that for a simplifying proposal? /be
Assignee | ||
Comment 16•24 years ago
|
||
Comment 17•24 years ago
|
||
Granting rtm+, as we are more likely to hit low memory situations than ever before.
Whiteboard: [rtm+]
Assignee | ||
Comment 18•24 years ago
|
||
r=,a= ? (or do I have a= since it's rtm+ ?)
Comment 19•24 years ago
|
||
a= is a code review (super-review) thing, it has nothing to do with [rtm+]. Except that adding [rtm+] before r= and a= are in the bug is likely to get the [rtm+] changed to [rtm need info]. My review: - I see js_ReportOutOfMemory defined and prototyped, but not called -- how's that work? Too bad it's so lengthy and stuff. - [nit] Comment style is non-conforming -- the stars should line up vertically to underhang by one space: +/* +* We don't post an exception in this case, since doing so runs into +* complications of pre-allocating an exception object which required +* running the Exception class initializer early etc. +* Instead we just invoke the errorReporter with an "Out Of Memory" +* type message, and then hope the process ends swiftly. +*/ - Any goto error in exn_initPrivate before all of the privateData is set will result in UMRs, assertbotches (report may be null), and crashes. memset the private data to all-zeros, and make exn_destroyPrivate null-tolerant. - [nit] exn_initPrivate is misnamed, it should be exn_newPrivate. - Use JS_strdup to clone report->filename into newReport->filename. - Use size_t capacity = js_strlen(report->uclinebuf) + 1; instead of jsint len = js_strlen(report->uclinebuf) + 1; - Same for ucmessage, and common the size_t capacity declaration to outermost block in function? - [nit] capricious initialization and no newline separating decls from code: + const char *msg; + /* Get the message for this error, but we won't expand any arguments. */ + const JSErrorFormatString *fmtData = (*errorCallback)(NULL, NULL, JSMSG_OUT_OF_MEMORY); + if (fmtData) + msg = fmtData->format; + else + msg = "Out Of Memory"; How about instead: + /* Get the message for this error, but we won't expand any arguments. */ + const JSErrorFormatString *fmtData = (*errorCallback)(NULL, NULL, JSMSG_OUT_OF_MEMORY); + const char *msg = fmtData ? fmtData->format : "Out Of Memory"; + or else uninitialized decls, with initializing statements after a newline? - [nit] tabs in new code (in js_ReportOutOfMemory) at: + if (fp) { + report.filename = fp->script->filename; + report.lineno = js_PCToLineNumber(fp->script, fp->pc); + } Fix your settings to expand tabs in new code, please. - This is a bogus pattern, don't clone it and do fix it where you find it: + if (cx->runtime->debugErrorHook && onError) { + JSDebugErrorHook hook = cx->runtime->debugErrorHook; + /* test local in case debugErrorHook changed on another thread */ + if (hook && !hook(cx, msg, NULL, + cx->runtime->debugErrorHookData)) { + onError = NULL; + } + } There is no thread safety in resampling hook. My version: + if (onError) { + JSDebugErrorHook hook = cx->runtime->debugErrorHook; + if (hook && + !hook(cx, msg, NULL, cx->runtime->debugErrorHookData)) { + onError = NULL; + } + } Did you copy that from elsewhere? I should grep (jband too). - Righteous tab expansion in js_ExpandErrorArguments (and error: cleanup case, but is that used by only one goto? maybe just inline it there instead and avoid a goto?). But diff -wu as well as diff -u so we can overlook it more easily, ok? Thanks. /be
Assignee | ||
Comment 20•24 years ago
|
||
Comment 21•24 years ago
|
||
exn_destroyPrivate still must defend against a null privateData->errorReport pointer, because of early failure in exn_newPrivate. Otherwise, looks ok. How about a diff -u and a separate diff -wu attachment (the latter can't be used to patch other people's code)? /be
Assignee | ||
Comment 22•24 years ago
|
||
Assignee | ||
Comment 23•24 years ago
|
||
Assignee | ||
Comment 24•24 years ago
|
||
Assignee | ||
Comment 25•24 years ago
|
||
Comment 26•24 years ago
|
||
marking need info. Please get a review and super review and then renominate
Whiteboard: [rtm+] → [rtm+ need info]
Comment 27•24 years ago
|
||
Has this been tested? fp in js_ReportOutOfMemory appears to be uninitialized; maybe it happens to be zero'd stack memory, most of the time (in debug builds). Please fix and reattach diffs of the affected file. /be
Assignee | ||
Comment 28•24 years ago
|
||
I've been testing by running in the debugger and forcing js_malloc to return NULL at arbitrary points, not exactly exhaustive but I'm not sure how else to go about it. I fixed the uninit'd fp (no thanks to MSVC) and will attach the new diffs.
Assignee | ||
Comment 29•24 years ago
|
||
Assignee | ||
Comment 30•24 years ago
|
||
Comment 31•24 years ago
|
||
In js_ReportOutOfMemory, 'report' doesn't seem to get used for much. You pass NULL where &report would go for the debug error hook and for onError; if this is intentional (because it results in allocation?), should 'report' go away?
Assignee | ||
Comment 32•24 years ago
|
||
No, that wasn't the intent. I had not been passing a report because of the associated allocations, but there were several intersting report elements that could still be useful and it seemed cheesy not to pass them along. Except then I did all the work but actually passing the report. Thanks!
Assignee | ||
Comment 33•24 years ago
|
||
Assignee | ||
Comment 34•24 years ago
|
||
Assignee | ||
Comment 35•24 years ago
|
||
Assignee | ||
Comment 36•24 years ago
|
||
Comment 37•24 years ago
|
||
Thanks to mccabe's review, I can give a more meaningful a=brendan@mozilla.org. /be
Comment 38•24 years ago
|
||
More reviewing... I see a report being passed to onError - but is there still a reason that the debugErrorHook gets NULL? Thanks for the thorough cruft-cleaning of my exn_initPrivate code in jsexn.c. bits - should 'len' and 'capacity' have the same type? (change the remaining 'len' to a use of 'capacity'?) - I see a 'goto error' but no 'error:' label! Yow! Good catch of a big oversight! obj = js_NewObject(cx, &ExceptionClass, JSVAL_TO_OBJECT(pval), NULL); if (!obj) return JS_FALSE; + *rval = OBJECT_TO_JSVAL(obj); js> foo = Error('skippy') js> print(foo) undefined
Assignee | ||
Comment 39•24 years ago
|
||
-You're referring to the NULL passed in js_ReportErrorAgain? i don't know why that doesn't get the report struct, I assumed there was a reason... - Changed len to capacity, thanks. - There is too an error: label! p.s. the Error() bug fix is actually sneaking in here from bug #56230; in for a penny, in for a pound :-)
Comment 40•24 years ago
|
||
Oops, I see the error: label, sorry. Manually reading the diffs... As for the passed null, this was the diff bit I'm referring to: (in js_ReportErrorAgain() ) - if (cx->runtime->debugErrorHook && onError) { + if (onError) { JSDebugErrorHook hook = cx->runtime->debugErrorHook; - /* test local in case debugErrorHook changed on another thread */ - if (hook && !hook(cx, message, reportp, - cx->runtime->debugErrorHookData)) { + if (hook && + !hook(cx, cx->lastMessage, NULL, cx->runtime->debugErrorHookData)) { onError = NULL; } } ... seems appropriate to still pass reportp there.
Assignee | ||
Comment 41•24 years ago
|
||
Ay Carumba! I saw the NULL and assumed that was how it was, but i must have copied the NULL when perpetrating brendan's suggested fix to the control-flow from the previous case which shouldn't have been NULL in the first place. umm. see? Thank you. New diffs coming, final this time I hope.
Assignee | ||
Comment 42•24 years ago
|
||
Assignee | ||
Comment 43•24 years ago
|
||
Comment 44•24 years ago
|
||
r=mccabe!
Assignee | ||
Comment 45•24 years ago
|
||
yay! fix checked in to trunk
Comment 47•24 years ago
|
||
Mccabe gets to give a=, I'll give r= henceforth. /be
Comment 48•24 years ago
|
||
This is an awfully big patch to take in the JS engine at this late point in the Seamonkey cycle. Is this a topcrash problem? Does it manifest on some popular web sites? Do we have any way to experimentally prove that it introduces no other problems? Marking [rtm need info]
Updated•24 years ago
|
Whiteboard: [rtm+] → [rtm need info]
Comment 49•24 years ago
|
||
All we know is what the initial reporter and the second comment say: this is a crash site known to Mac users. Cc'ing jpatel to see whether he's seen anything like it in talkback. /be
Reporter | ||
Comment 50•24 years ago
|
||
I've been following your discussions regarding a fix for the bug I reported. I had to find a solution because it was a show stopper in my application. I've embedded JavaScript as a Macintosh OSA component (so that it becomes a peer to AppleScript as a system-level scripting language). In this context it is common for JavaScript to be asked to run in as little as 200K. Under these conditions I found myself running into this error frequently. You can see the software I'm working on at http://www.latenightsw.com/freeware/JavaScriptOSA My changes to resolve the issue (against the M16 source drop) are as follows: 1 extend the definition of JSRuntime to include a new boolean flag: //MALL JSHashTable* deflated_string_cache; JSBool reportingError; }; This flag is initialized to false when the runtime is initialized 2 : modify JS_ReportOutOfMemory to use the flag to avoid recurring. In my testing, JS_ReportOutOfMemory was the bottleneck for the recursion. JS_PUBLIC_API(void) JS_ReportOutOfMemory(JSContext *cx) { // MALL if (!cx->runtime->reportingError) { cx->runtime->reportingError = JS_TRUE; JS_ReportError(cx, "out of memory"); cx->runtime->reportingError = JS_FALSE; } else { // changes to to a last change attempt at reporting an out of memory error JSDebugErrorHook hook = cx->runtime->debugErrorHook; /* test local in case debugErrorHook changed on another thread */ if (hook) hook(cx, "out of memory", NULL, cx->runtime->debugErrorHookData);
Comment 51•24 years ago
|
||
Mark - can you confirm whether the proposed fix (which is in the trunk, now) also fixes the problems you saw?
Comment 52•24 years ago
|
||
Removing 'need info,' recommending ++. jpatel, any supporting info? I was able to reproduce this crash trivially, and I'm sure it occurs in practice with limited-memory situations like that described by Mark. We're probably dying on any out-of-memory error. The patch fixes the crash I see. I went over Roger's patch with a fine-toothed comb, and it looks good. It doesn't have many interacting changes, just fixes a number of cases where we weren't handling out-of-memory cases correctly. The JS language testsuite also completes with no failures. (To reproduce, I set the JS memory pool to 32k in the standalone shell, and executed a simple script to use up memory: js> a = []; for (i = 0; i < 32 * 1024; i++) { a[i] = 'fooo' + i +' fooo' } zsh: segmentation fault (core dumped) js (the dumped core had 181000 frames in the backtrace! Wow!)
Whiteboard: [rtm need info] → [rtm]
Comment 53•24 years ago
|
||
I haven't seen anything like this in the latest branch talkback reports. Is there a specific stack signature that I should be looking for? Most of the other JS bugs have fallen off the radar and are no longer showing up in the topcrash list. If any of you crash reproducing this bug, please enter your email address in the talkback window so that I can go and look at what the stack trace looks like. Thanks.
Comment 54•24 years ago
|
||
Changing [rtm] to [rtm+] since I assume that was intended. Please correct me if I'm wrong here. > ------- Additional Comments From Mike McCabe 2000-10-16 17:56 ------- > > Removing 'need info,' recommending ++. Adding "fixed in trunk" per rogerl@netscape.com 2000-10-13 15:28.
Whiteboard: [rtm] → [rtm+] fixed in trunk
Comment 55•24 years ago
|
||
rtm-, not a topcrash, can't afford the risk of such a huge change right now.
Whiteboard: [rtm+] fixed in trunk → [rtm-] fixed in trunk
Comment 56•24 years ago
|
||
Ok, if the fix isn't going in the branch, and the fix is in the trunk, it's time to close this bug. So trunk verification only. /be
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
Comment 57•24 years ago
|
||
Verified on Linux and WinNT using Mike's idea at 2000-10-16 17:56 above: " (To reproduce, I set the JS memory pool to 32k in the standalone shell, and executed a simple script to use up memory: js> a = []; for (i = 0; i < 32 * 1024; i++) { a[i] = 'fooo' + i +' fooo' } " When I did this, I did not crash, but got the out-of-memory message instead. Tested this with both debug and optimized builds of the JS shell -
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•