JS stack overflow in JS_ReportOutOfmemory

VERIFIED FIXED

Status

()

P2
normal
VERIFIED FIXED
19 years ago
17 years ago

People

(Reporter: alldritt, Assigned: rogerl)

Tracking

({crash, js1.5})

Trunk
All
Other
crash, js1.5
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [rtm-] fixed in trunk)

Attachments

(15 attachments)

(Reporter)

Description

19 years ago
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

19 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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: crash
(Reporter)

Comment 2

19 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

19 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

19 years ago
cc'ing Brendan -
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
Is 41727 a dup of part of this bug?

/be
(Assignee)

Comment 7

19 years ago
i'll take a look at this'n today
Status: NEW → ASSIGNED
(Assignee)

Comment 8

19 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?
(Assignee)

Comment 9

18 years ago
*** Bug 41727 has been marked as a duplicate of this bug. ***

Comment 10

18 years ago
As per discussion with JS Engine team, setting P2 priority and rtm keyword
Keywords: rtm
Priority: P3 → P2
(Assignee)

Comment 11

18 years ago
Created attachment 16344 [details] [diff] [review]
Clean-up for alloc failures etc.
(Assignee)

Comment 12

18 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.
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

18 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?
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

18 years ago
Created attachment 16388 [details] [diff] [review]
Including new js_ReportOutOfMemory.

Comment 17

18 years ago
Granting rtm+, as we are more likely to hit low memory situations than ever 
before.
Whiteboard: [rtm+]
(Assignee)

Comment 18

18 years ago
r=,a= ? (or do I have a= since it's rtm+ ?)
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

18 years ago
Created attachment 16546 [details] [diff] [review]
All nits picked (I think), diff -wu for better viewing.
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

18 years ago
Created attachment 16552 [details] [diff] [review]
new -wu diff
(Assignee)

Comment 23

18 years ago
Created attachment 16553 [details] [diff] [review]
new -u patch
(Assignee)

Comment 24

18 years ago
Created attachment 16561 [details] [diff] [review]
d'oh, wrong files. This is better.
(Assignee)

Comment 25

18 years ago
Created attachment 16564 [details] [diff] [review]
d'oh, wrong files. This is better.

Comment 26

18 years ago
marking need info.  Please get a review and super review and then renominate
Whiteboard: [rtm+] → [rtm+ need info]
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

18 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

18 years ago
Created attachment 16773 [details] [diff] [review]
diff -u
(Assignee)

Comment 30

18 years ago
Created attachment 16774 [details] [diff] [review]
diff -wu

Comment 31

18 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

18 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

18 years ago
Created attachment 17015 [details] [diff] [review]
diff -wu
(Assignee)

Comment 34

18 years ago
Created attachment 17016 [details] [diff] [review]
diff -u
(Assignee)

Comment 35

18 years ago
Created attachment 17017 [details] [diff] [review]
diff -wu, I need coffee
(Assignee)

Comment 36

18 years ago
Created attachment 17018 [details] [diff] [review]
diff -u, badly
Thanks to mccabe's review, I can give a more meaningful a=brendan@mozilla.org.

/be

Comment 38

18 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

18 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

18 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

18 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

18 years ago
Created attachment 17052 [details] [diff] [review]
diff -wu
(Assignee)

Comment 43

18 years ago
Created attachment 17054 [details] [diff] [review]
diff -u

Comment 44

18 years ago
r=mccabe!
(Assignee)

Comment 45

18 years ago
yay! fix checked in to trunk

Comment 46

18 years ago
r= and a=, removing 'need info'
Whiteboard: [rtm+ need info] → [rtm+]
Mccabe gets to give a=, I'll give r= henceforth.

/be

Comment 48

18 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

18 years ago
Whiteboard: [rtm+] → [rtm need info]
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

18 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

18 years ago
Mark - can you confirm whether the proposed fix (which is in the trunk, now)
also fixes the problems you saw?

Comment 52

18 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

18 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.
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

18 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
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
Last Resolved: 18 years ago
Resolution: --- → FIXED

Comment 57

18 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.