14044 - jsdtoa.c doesn't check for out-of-memory

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P4)

Description

[Waldemar Horwat]

David Gay's code in jsdtoa.c assumes that malloc will never fail.  It will
corrupt memory and/or crash when malloc does fail.  This is a very rare
situation but should be checked and handled properly.

    Waldemar

Comment 1

[Waldemar Horwat]

Attachment: Smaller test case

Comment 2

[Waldemar Horwat]

Please ignore the attachment; I attached it to the wrong bug report.

Comment 3

[leger]

Setting all Javacript bugs to rginda QA Contact.

Comment 4

[Phil Schwartau]

Updating QA contact -

Comment 5

[Matthias Versen [:Matti]]

[e-mail] :
You are assigned on this bug.
Can you add a comment that you are working on it or can you close it ?
(Last comment : 1999-09-17 16:12 )

Comment 6

[Peter A Jonsson]

Attachment: Possible patch.

Malloc is only used on two places in jsdtoa.c, one is in Balloc() and the other
is in JS_dtobasestr(), the second where it is checked for null returns. This
patch makes Balloc() check for a null return from malloc() and if so avoid
writing to the pointer, returning null. Is it legal for Balloc() to return
null? This patch is untested, how should one test it?

Comment 7

[Phil Schwartau]

cc'ing reviewers for this patch -

Comment 8

[Patrick C. Beard]

Comment on attachment 73866 [details] [diff] [review]
Possible patch.

r=beard

Comment 9

[Steven Cole]

To address the question in comment 6:  Nope, it's not currently valid for Balloc
to return null.  This will result in numerous crashes all over jsdtoa.c.  It
looks like in addition to what this patch fixes, we need to fix return values
all over the code, and then check to make sure we haven't leaked anything.

My "malloc failure" generator that I've been using in a number of other bugs
recently is appropriate to test here, provided that we have a JS test that
excersizes all of the allocations here.  (Hopefully, all of the "Number" tests
in the JS testsuite do that.)

I'd also like to run such a test under Purify and PureCoverage to make sure
we've hit it all.

I can take this on (I have all the tools necessary to make it happen, and memory
failure is something that I seem to have boxed myself into lately), but I can't
guarantee speediness.

--scole

Comment 10

[Steven Cole]

I think, though, that I'll wait for the fixes in bug 120992 and bug 85267 to get
checked in, to save myself from merge heck.  (Looks like only slight reviewing
is necessary there, though.)

--scole

Comment 11

[Steven Cole]

Attachment: Proposed Patch

This big patch checks the return values of each routine in the dtoa suite that
may succumb to out-of-memory errors.  It also slightly changes the API for
JS_strtod: the "err" return parameter may now contain ENOMEM, in addtion to
ERANGE, so callers to this function check for that as well.

This patch contains my diagnostic code as well (turned off via #ifdef
ENABLE_OOM_TESTING); I'm not sure whether it's best to leave it in or to remove
it for checkin.  Opionions welcomed.

This patch has undergone about 3 days of coverage testing; there are still some
paths I haven't been able to make run, but there are very few of them.	(I have
run the portions of the JS testsuite dealing with Number objects and Type
conversion through.)

--scole

Comment 12

[Steven Cole]

Nominating for JS 1.5.

Comment 13

[Phil Schwartau]

Need r=, sr= to get this patch in for JS 1.5 -

Comment 14

[Brendan Eich [:brendan]]

Comment on attachment 86082 [details] [diff] [review]
Proposed Patch

>+static int allocationNum;               /* which allocation is next? */
>+static int desiredFailure = -1;         /* which allocation should fail? */
>+#ifdef ENABLE_OOM_TESTING
>+    if (desiredFailure == allocationNum++)

Nit: mightn't (++allocationNum == desiredFailure) be a little better in
compiled code, and maybe in clarity?  Then the default value for desiredFailure
could be 0, not -1.

>+    {

Nit: prevailing style in the engine, if not in jsdtoa.c (;-), puts this on the
same line as the if.

0123456789012345678901234567890123456789012345678901234567890123456789012345678
9
>-/* Return b*m + a.  Deallocate the old b.  Both a and m must be between 0 and 65535 inclusive. */
>+/* Return b*m + a.  Deallocate the old b.  Both a and m must be between
>+ * 0 and 65535 inclusive.  NOTE: old b is deallocated on memory
>+ * failure. */

Uber-nit: column 80 is cool, but this seems to wrap before column 74.  Might
fit better with the closing */ indented one space on its own line, too.

>+#ifdef ENABLE_OOM_TESTING
>+    if (desiredFailure == allocationNum++)
>+    {
>+        // Faux allocation, because I'm not getting all of the failure
>+        // paths without it.

Whoop, C++ comment alert!  This is C code.

>@@ -691,6 +793,10 @@
> #ifdef JS_THREADSAFE
>             Bigint *wasted_effort = NULL;
>             p51 = mult(p5, p5);
>+            if (!p51) {
>+                Bfree(b);
>+                return NULL;
>+            }
>             PR_Lock(p5s_lock);
>             if (!p5->next) {
>                 p5->next = p51;
>@@ -704,8 +810,14 @@
>                 Bfree(wasted_effort);
>             }
> #else
>-            p51 = p5->next = mult(p5,p5);
>+            p51 = mult(p5,p5);
>+            if (!p51)
>+            {

Nit: More discordant if-then brace style.

>@@ -2763,9 +2999,21 @@
>             /* We have a fraction. */
>             int32 e, bbits, s2, done;
>             Bigint *b, *s, *mlo, *mhi;
>+
>+            b = s = mlo = mhi = NULL;
>             
>             *p++ = '.';
>             b = d2b(df, &e, &bbits);
>+            if (!b)
>+            {

Same nit.

>+++ jsnum.c	2002/06/03 19:11:51
>@@ -829,7 +829,7 @@
> 
>     /* Use cbuf to avoid malloc */
>     if (length >= sizeof cbuf) {
>-        cstr = (char *) malloc(length + 1);
>+        cstr = (char *) JS_malloc(cx, length + 1);
>         if (!cstr)
>            return JS_FALSE;
>     } else {
>@@ -853,6 +853,13 @@
>     } else {
> 	int err;
> 	d = JS_strtod(cstr, &estr, &err);
>+        if (err == ENOMEM)
>+        {

Please do follow brace-on-if-line style here, jsnum.c is part of the "house",
not a previous house's foundation (as jsdtoa.c arguably is, if it has any
single owner).

>@@ -973,7 +980,7 @@
>              */
> 	    size_t i;
> 	    size_t length = s1 - start;
>-	    char *cstr = (char *) malloc(length + 1);
>+	    char *cstr = (char *) JS_malloc(cx, length + 1);
> 	    char *estr;
>             int err=0;
> 

Free free to expand tabs per the Emacs modeline as you can.

Thanks for doing this, it looks good to me.  I got a little tired of checking
after a while, so an r= from khanson (perhaps reviewing from bottom up in the
file :-) would help.  sr=[e-mail] with nits picked.

/be

Comment 15

[Steven Cole]

Attachment: Nits Cleaned.

Nits picked.  Also de-tabbed all of js_strtod and js_strtointeger.  And since
GetNextBinaryDigit was sitting there right between, it got de-tabbed too.  (A
-uw patch is coming next...)

--scole

Comment 16

[Steven Cole]

Attachment: A -uw patch for jsnum.c, for reviewing clarity

The "ignore whitespace" patch, for reviewing ease.

--scole

Comment 17

[Kenton Hanson (gone)]

ENOMEM not defined in Mac version

Comment 18

[Steven Cole]

Oh, isn't that nice.   Kenton, I don't have access to a Mac development
environment.  Can you scan for where ERANGE is defined (somewhere in the system
includes, probably) and see if there's an appropriate "no memory" error we can
use for the Mac?

Alternately, we can just define ENOMEM to be (ERANGE+1) since all we really care
is that ENOMEM != ERANGE and ENOMEM != 0.  (Though it would probably be more
appropriate to re-enumerate and make custom dtoa errors instead of using errno
values.)

Does anyone have any opinions about the "right" way to fix this?  (While I'm
waiting for comments, I'll work on a patch that removes the dependency on ENOMEM
and switches to JS_ERANGE and JS_ENOMEM instead, though today's a busy day and
it might take a while.)

--scole

Comment 19

[Kenton Hanson (gone)]

scole, I too am curious about the best way to handle the ENOMEM reference.  I
simply moved the #define ENOMEM outside the conditional compile in file ceerno.
 No need to do an interim patch.

Comment 20

[Kenton Hanson (gone)]

file cerrno

Comment 21

[Steven Cole]

The more I think about it, the more I like JS_ENOMEM and JS_ERANGE.  I only used
ENOMEM because the code that was there passed back the ERANGE error, and ENOMEM
is the same sort of style.  But if it introduces platform-dependence to the
code, then it's not really a winning solution.  (Since *this patch* generates
the ENOMEM condition and not something in the underlying OS, there's no reason
for a platform-dependent solution to this problem.)

My proposed patch (yet to be worked on) translates the ERANGE error that exists
now (and presumably works for all platforms) to JS_ERANGE, and then just uses
the new JS_ENOMEM when an out-of-memory error needs to be generated.  No
platform dependencies there at all --- which means that we won't be plagued by
some odd embedding's compiler support in the future either.  (I hate compiler
configuration bugs...)

[Though maybe these should be JS_DTOA_ERANGE and JS_DTOA_ENOMEM...]

--scole

Comment 22

[Steven Cole]

Attachment: New patch: No ENOMEM.

Ok, here's a version of the fix that removes ENOMEM and ERANGE and replaces
them with JS_DTOA_ERANGE and JS_DTOA_ENOMEM, and eliminates the dependency on
<errno.h>.

--scole

Comment 23

[Brendan Eich [:brendan]]

scole, how about a -wu diff?  I'll sr= it this weekend.  Thanks,

/be

Comment 24

[Steven Cole]

Attachment: -wu style

And what am *I* doing Saturday night?  Making patches.	What a life.  :-P

Here's your -wu patch, Brendan.

--scole

Comment 25

[Brendan Eich [:brendan]]

Comment on attachment 88484 [details] [diff] [review]
New patch: No ENOMEM.

sr=[e-mail] contingent on khanson's r= stamp of approval.  Kenton,
can you please review and if all is well, check this into the trunk and drive
it into the 1.0 branch?  Thanks,

/be

Comment 26

[Steven Cole]

I hate to be picky, but...  Does this patch really merit committal on the 1.0
branch?  It doesn't seem to meet any of the 1.0 criteria.  This patch is a boon
to Spidermonkey embedders, not mozilla-the-browser...

--scole

Comment 27

[Kenton Hanson (gone)]

Comment on attachment 88484 [details] [diff] [review]
New patch: No ENOMEM.

r=khanson

Comment 28

[Kenton Hanson (gone)]

checked into trunk.  I will seek approval for branch.

Comment 29

[Alec Flett]

this seems to have increased mozilla startup (Ts) by about 10ms:

http://tegu.mozilla.org/graph/query.cgi?testname=startup&tbox=comet&autoscale=1&days=7&avg=1&showpoint=2002:06:23:23:54:34,1498

Comment 30

[Alec Flett]

verified on a 2nd tinderbox
http://tegu.mozilla.org/graph/query.cgi?testname=startup&tbox=luna&autoscale=1&days=7&avg=1&showpoint=2002:06:24:01:08:42,3182

(on both graphs, the green diamond is the build which first had this checkin)
Tp and Txul don't seem to be affected

Comment 31

[Steven Cole]

Well, I hate to admit it, but I'm not surprised by the timing tests.  I knew
this was going to hurt performance, though it's kind of lousy that it's this
noticable.

I think it'd take a pretty significant redesign of the dtoa code to get the
performance back up *and* get memory safety as well.

--scole

Comment 32

[Kenton Hanson (gone)]

The latest patch is on the trunk but not the branch.  It didn't receive driver
approval because of the large size of the change and the fact that no known
crash has been caused by this deficiency.  So, I agree with Steve Cole,
complexity and degradation of speed are reasons for not including the patch,
while code correctness (possibly a security issue) are reasons for including the
patch.  I would favor inclusion, but its not my call.

Comment 33

[Steven Cole]

I just added bug 156253 to address the speed issues of this patch.

--scole

Comment 34

[Steven Cole]

So if this patch isn't going to get on the branch but is already on the trunk,
can we mark it FIXED?  (And a related question: Is JS 1.5 supposed to match
mozilla1.0.1 or mozilla1.1?  Or are they separate?  In other words, will JS1.5
get rolled from the trunk or from one of the mozilla branches?)

--scole

Comment 35

[Brendan Eich [:brendan]]

I would still like to see js1.5 wrap up soon: RC5 at the end of July, Final
release at the end of August?

I think that the js engine should not differ materially until 1.5 releases
between trunk and branch.  We can probably get all the fixes in, although we may
need to wait till 1.2/1.0.2 to get drivers' approval.

So it seems best to me that we fix bug 156253 on branch and trunk, soon.  scole,
do you agree?  What do others think of my release plan sketch-question, above?

/be

Comment 36

[Steven Cole]

[My responses to Brendan's question in comment 35 are in bug 156253.  --scole]

Comment 37

[Brendan Eich [:brendan]]

Does this bug serve any useful purpose any longer?  Maybe it can be closed.

/be

Comment 38

[Steven Cole]

All right...  This bug was being left open during the "push for 1.0" stage 
because bugs at that time weren't getting marked FIXED until they got checked 
in on the 1.0 branch.  This fix never went there, but has been present in the 
1.1 and 1.2 builds.

So I'm going to mark this FIXED now.

Comment 39

[Phil Schwartau]

Marking Verified -

For the record, recall this point from Comment #29:
> this seems to have increased mozilla startup (Ts) by about 10ms

And this response from Comment #33:
> I just added bug 156253 to address the speed issues of this patch.