46196 - JS stack overflow in JS_ReportOutOfmemory

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[Mark Alldritt]

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

[Andreas Franke (gone)]

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.

Comment 2

[Mark Alldritt]

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

Comment 3

[Mark Alldritt]

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

[Phil Schwartau]

cc'ing Brendan -

Comment 5

[Brendan Eich [: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

Comment 6

[Brendan Eich [:brendan]]

Is 41727 a dup of part of this bug?

/be

Comment 7

[rogerl (gone)]

i'll take a look at this'n today

Comment 8

[rogerl (gone)]

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 9

[rogerl (gone)]

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

Comment 10

[Phil Schwartau]

As per discussion with JS Engine team, setting P2 priority and rtm keyword

Comment 11

[rogerl (gone)]

Attachment: Clean-up for alloc failures etc.

Comment 12

[rogerl (gone)]

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

[Brendan Eich [:brendan]]

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

Comment 14

[rogerl (gone)]

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

[Brendan Eich [:brendan]]

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

Comment 16

[rogerl (gone)]

Attachment: Including new js_ReportOutOfMemory.

Comment 17

[Patrick C. Beard]

Granting rtm+, as we are more likely to hit low memory situations than ever 
before.

Comment 18

[rogerl (gone)]

r=,a= ? (or do I have a= since it's rtm+ ?)

Comment 19

[Brendan Eich [:brendan]]

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

Comment 20

[rogerl (gone)]

Attachment: All nits picked (I think), diff -wu for better viewing.

Comment 21

[Brendan Eich [:brendan]]

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

Comment 22

[rogerl (gone)]

Attachment: new -wu diff

Comment 23

[rogerl (gone)]

Attachment: new -u patch

Comment 24

[rogerl (gone)]

Attachment: d'oh, wrong files. This is better.

Comment 25

[rogerl (gone)]

Attachment: d'oh, wrong files. This is better.

Comment 26

[Michael La Guardia]

marking need info.  Please get a review and super review and then renominate

Comment 27

[Brendan Eich [:brendan]]

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

Comment 28

[rogerl (gone)]

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.

Comment 29

[rogerl (gone)]

Attachment: diff -u

Comment 30

[rogerl (gone)]

Attachment: diff -wu

Comment 31

[Mike McCabe]

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?

Comment 32

[rogerl (gone)]

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!

Comment 33

[rogerl (gone)]

Attachment: diff -wu

Comment 34

[rogerl (gone)]

Attachment: diff -u

Comment 35

[rogerl (gone)]

Attachment: diff -wu, I need coffee

Comment 36

[rogerl (gone)]

Attachment: diff -u, badly

Comment 37

[Brendan Eich [:brendan]]

Thanks to mccabe's review, I can give a more meaningful a=[e-mail].

/be

Comment 38

[Mike McCabe]

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

Comment 39

[rogerl (gone)]

-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

[Mike McCabe]

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.

Comment 41

[rogerl (gone)]

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.

Comment 42

[rogerl (gone)]

Attachment: diff -wu

Comment 43

[rogerl (gone)]

Attachment: diff -u

Comment 44

[Mike McCabe]

r=mccabe!

Comment 45

[rogerl (gone)]

yay! fix checked in to trunk

Comment 46

[Mike McCabe]

r= and a=, removing 'need info'

Comment 47

[Brendan Eich [:brendan]]

Mccabe gets to give a=, I'll give r= henceforth.

/be

Comment 48

[Phil Peterson]

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]

Comment 49

[Brendan Eich [:brendan]]

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

Comment 50

[Mark Alldritt]

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

[Mike McCabe]

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

Comment 52

[Mike McCabe]

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!)

Comment 53

[Jay Patel [:jay]]

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

[Andreas Franke (gone)]

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 [e-mail] 2000-10-13 15:28.

Comment 55

[selmer (gone)]

rtm-, not a topcrash, can't afford the risk of such a huge change right now.

Comment 56

[Brendan Eich [:brendan]]

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

Comment 57

[Phil Schwartau]

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 -