Last Comment Bug 516862 - Array indexing error in js/src/dtoa.c's Balloc() leads to floating point memory vulnerability (SA36711)
: Array indexing error in js/src/dtoa.c's Balloc() leads to floating point memo...
Status: VERIFIED FIXED
[sg:critical] same issue as 516396 (C...
: crash, testcase, verified1.8.1.24, verified1.9.0.15, verified1.9.0.16, verified1.9.1
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: P1 critical (vote)
: mozilla1.9.3a1
Assigned To: Reed Loden [:reed] (use needinfo?)
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on: 520629 CVE-2009-0689
Blocks:
  Show dependency treegraph
 
Reported: 2009-09-15 19:31 PDT by Reed Loden [:reed] (use needinfo?)
Modified: 2013-03-19 07:29 PDT (History)
23 users (show)
dsicore: blocking1.9.2+
dveditz: blocking1.9.0.15+
dveditz: wanted1.9.0.x+
dveditz: blocking1.8.1.next+
dveditz: wanted1.8.1.x+
choller: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
beta1-fixed
.4+
.4-fixed


Attachments
patch - v1 (3.23 KB, patch)
2009-09-15 19:49 PDT, Reed Loden [:reed] (use needinfo?)
crowderbt: review+
mrbkap: review+
dveditz: approval1.9.1.4+
Details | Diff | Splinter Review
patch - v1 (1.9.0) (4.12 KB, patch)
2009-09-18 09:18 PDT, Reed Loden [:reed] (use needinfo?)
crowderbt: review+
mrbkap: review+
dveditz: approval1.9.0.15+
Details | Diff | Splinter Review
Add an input length check to _strtod - v1 (3.28 KB, patch)
2009-09-28 19:56 PDT, Reed Loden [:reed] (use needinfo?)
no flags Details | Diff | Splinter Review
Add an input length check to _strtod - v2 (3.51 KB, patch)
2009-10-02 17:28 PDT, Reed Loden [:reed] (use needinfo?)
no flags Details | Diff | Splinter Review
Add an input length check to _strtod - v2.1 (4.28 KB, patch)
2009-10-02 18:43 PDT, Reed Loden [:reed] (use needinfo?)
crowderbt: review+
mrbkap: review+
Details | Diff | Splinter Review
Follow-up to 1.9.0 patch to fix use of uninitialized 'rv' - v1 (1.01 KB, patch)
2009-10-15 03:38 PDT, Reed Loden [:reed] (use needinfo?)
crowderbt: review+
wtc: review+
samuel.sidler+old: approval1.9.0.15+
samuel.sidler+old: approval1.9.0.16+
Details | Diff | Splinter Review
rollup for MOZILLA_1_8_BRANCH (2.29 KB, patch)
2010-02-18 23:18 PST, :Gavin Sharp [email: gavin@gavinsharp.com]
no flags Details | Diff | Splinter Review

Description Reed Loden [:reed] (use needinfo?) 2009-09-15 19:31:31 PDT
[split from bug 516396]

Secunia notified security@ of a problem in nsprpub/pr/src/misc/prdtoa.c's Balloc() function. The same issue also occurs in js/src/dtoa.c, so the patch from bug 416396 needs to be ported to that copy of dtoa.
Comment 1 Reed Loden [:reed] (use needinfo?) 2009-09-15 19:49:24 PDT
Created attachment 400936 [details] [diff] [review]
patch - v1

I ported wtc's patch in bug 516396 to the copy js uses.

I did a diff between the two files, and there are a lot of differences, mostly in casting, but there are some code changes as well...
Comment 2 Brian Crowder 2009-09-16 07:33:26 PDT
Comment on attachment 400936 [details] [diff] [review]
patch - v1

Thanks, reed...  You may want/need a proper js peer for this, too.  But it looks good to me.
Comment 3 Reed Loden [:reed] (use needinfo?) 2009-09-16 07:37:34 PDT
Comment on attachment 400936 [details] [diff] [review]
patch - v1

Since it's a security bug, need sr anyway... tagging brendan.
Comment 4 Blake Kaplan (:mrbkap) 2009-09-16 09:27:06 PDT
Comment on attachment 400936 [details] [diff] [review]
patch - v1

js/src is still exempt from the sr policy, but I'll stamp this as a JS peer.
Comment 5 Reed Loden [:reed] (use needinfo?) 2009-09-16 23:32:31 PDT
http://hg.mozilla.org/mozilla-central/rev/2aa5b8b5afe9
Comment 6 Daniel Veditz [:dveditz] 2009-09-17 11:32:23 PDT
Comment on attachment 400936 [details] [diff] [review]
patch - v1

Approved for 1.9.1.4 and 1.9.0.15, a=dveditz
Comment 7 Damon Sicore (:damons) 2009-09-17 12:20:48 PDT
Blocking 1.9.2+.  P1.
Comment 9 Reed Loden [:reed] (use needinfo?) 2009-09-18 09:16:14 PDT
Comment on attachment 400936 [details] [diff] [review]
patch - v1

1.9.0 is completely different... need a new patch.
Comment 10 Reed Loden [:reed] (use needinfo?) 2009-09-18 09:18:24 PDT
Created attachment 401451 [details] [diff] [review]
patch - v1 (1.9.0)

Untested patch for 1.9.0. This version of dtoa doesn't have any of the locking stuff, so definitely please take a closer look here to make sure something isn't missing or wrong or something.
Comment 11 Brian Crowder 2009-09-18 10:12:32 PDT
Comment on attachment 401451 [details] [diff] [review]
patch - v1 (1.9.0)

Rubber-stamp.  Are we even sure this version has the same bug?
Comment 12 Al Billings [:abillings] 2009-09-21 15:10:40 PDT
Is there a way to reproduce this problem on both 1.9.1 and 1.9.0 for verification of the fix? I assume not.
Comment 13 Daniel Veditz [:dveditz] 2009-09-21 17:47:42 PDT
Comment on attachment 401451 [details] [diff] [review]
patch - v1 (1.9.0)

Approved for 1.9.0.15, a=dveditz
Comment 14 Reed Loden [:reed] (use needinfo?) 2009-09-21 23:16:54 PDT
Checking in js/src/jsdtoa.c;
/cvsroot/mozilla/js/src/jsdtoa.c,v  <--  jsdtoa.c
new revision: 3.42; previous revision: 3.41
done
Comment 15 Daniel Veditz [:dveditz] 2009-09-23 15:45:30 PDT
The testcases in bug 516396 use JavaScript -- are they testing the NSPR version of the function in that bug or are they really testing _this_ fix?
Comment 16 Daniel Veditz [:dveditz] 2009-09-23 16:14:15 PDT
They appear to be testing this bug. They turned what appeared to be a hang into an outright immediate crash. Slightly different places, I'm not sure I'm happy with either.
  bp-f2a1d879-1b83-4a00-b844-e1e0a2090923
  bp-b026cdaa-d503-4d54-b2b1-f4b4a2090923
Comment 17 Wan-Teh Chang 2009-09-23 16:17:14 PDT
I actually created an NSPR test case for this bug, but
it took too long to finish, so I can't add it to the
NSPR test suite.

Should we consider failing early if the input string
is too long?
Comment 18 Wan-Teh Chang 2009-09-23 17:23:44 PDT
dveditz: the first crash report crashed in malloc:
  bp-f2a1d879-1b83-4a00-b844-e1e0a2090923
Perhaps it ran out of memory?
Comment 19 Al Billings [:abillings] 2009-09-24 10:21:34 PDT
Wan-Teh, can you grab last night's 1.9.0 build and run your test case for this bug against it in order to verify that this bug is fixed?
Comment 20 Reed Loden [:reed] (use needinfo?) 2009-09-24 12:09:48 PDT
(In reply to comment #19)
> Wan-Teh, can you grab last night's 1.9.0 build and run your test case for this
> bug against it in order to verify that this bug is fixed?

This bug isn't fixed, as per bug 516396. Need a different patch for both bugs.
Comment 21 Wan-Teh Chang 2009-09-25 21:09:44 PDT
Al, I don't have a test case for this JavaScript bug.
Reed's test case in NSPR bug 516396 comment 2 is actually
a test case for this bug.  You can use Reed's test case
to verify this bug is fixed.
Comment 22 Al Billings [:abillings] 2009-09-28 11:11:55 PDT
Running the bug 516396 comment 2 testcase on Firefox 3.5.3 causes a crash. When I run it on a nightly 3.5.4 build, Firefox hangs (without actually crashing). Is this really fixed?

Tested on XP with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.4pre) Gecko/20090924 Shiretoko/3.5.4pre (.NET CLR 3.5.30729.
Comment 23 Wan-Teh Chang 2009-09-28 11:20:45 PDT
Al: the hang is expected.  Firefox doesn't really hang; that
test case causes Firefox to take a very long time to convert
the string to a double.

I have an NSPR patch in bug 516396 comment 41 to address
the hang.  You may want a similar patch for JavaScript.
Comment 24 Reed Loden [:reed] (use needinfo?) 2009-09-28 15:05:19 PDT
Not sure how you can verify something that isn't completely fixed yet. We need to port the second patch from the NSPR version of this bug over to js. I've been working on a patch in my spare time, but I just haven't had time to finish it completely yet.
Comment 25 Wan-Teh Chang 2009-09-28 15:25:50 PDT
Reed: In JavaScript, you can do the string length check in
jsdtoa.cpp, function JS_strtod:

129 JS_FRIEND_API(double)
130 JS_strtod(const char *s00, char **se, int *err)
131 {
132     double retval;
        // Check s00 length here!
133     if (err)
134         *err = 0;
135     LOCK_DTOA();
136     retval = _strtod(s00, se);
137     UNLOCK_DTOA();
138     return retval;
139 }

You need to add JS_DTOA_EINVAL if you want to follow
the 0 return value and EINVAL error code in the strtod'
man page in Single Unix Specification:
http://www.opengroup.org/onlinepubs/9699919799/functions/strtod.html

Note: |err| is not set by JS_strtod correctly on return.  You need
to do this:

    LOCK_DTOA();
+   errno = 0;
    retval = _strtod(s00, se);
+   if (err) {
+       switch (errno) {
+       case ERANGE:
+           *err = JS_DTOA_ERANGE;
+           break;
+       case ENOMEM:
+           *err = JS_DTOA_ENOMEM;
+           break;
+       }
+   }
    UNLOCK_DTOA();
Comment 26 Al Billings [:abillings] 2009-09-28 16:11:25 PDT
I was marking it "fixed" based on the comment that the hang without a crash was expected.

We're most of a week past code complete now and cutting builds within the new week. If your patch doesn't get checked in Reed, this is going to ship as it is now.

So, everyone, is the current behavior acceptable and "fixed" or not? If not, perhaps we need to back the whole thing out rather than ship half of a fix?

Dveditz, any comments?
Comment 27 Wan-Teh Chang 2009-09-28 16:18:59 PDT
Al: the hang does not allow the attacker to execute arbitrary code,
so don't back out what's checked in.
Comment 28 Al Billings [:abillings] 2009-09-28 16:22:15 PDT
Good point. It isn't my call anyway. It is up to Dveditz and Beltzner to make the call. The question is whether we need Reed's patch or not. I assume we do but it seems likely to miss the train at this point, leaving this bug in limbo.
Comment 29 Reed Loden [:reed] (use needinfo?) 2009-09-28 19:56:32 PDT
Created attachment 403404 [details] [diff] [review]
Add an input length check to _strtod - v1

Thanks, wtc. I was on the right path, but you greatly simplified what I was doing.

JS_DTOA_ENOMEM doesn't seem to be actually used anywhere, as far as actually being set. dtoa.c never sets errno = ENOMEM directly, though I guess one of the memory-related functions could do it.

I just made JS_DTOA_EINVAL return out-of-memory error like JS_DTOA_ENOMEM. If wanted, I can add a new error number/message for it.

I haven't tested this yet... just getting the patch posted before I start a build.
Comment 30 Brian Crowder 2009-09-29 08:39:02 PDT
Comment on attachment 403404 [details] [diff] [review]
Add an input length check to _strtod - v1

It concerns me that this length-checking stuff isn't being done inside dtoa, since I think it means you're paying to walk the string twice...  what're the perf implications of that?  Is this just meant to prevent a DOS?
Comment 31 Wan-Teh Chang 2009-09-29 13:50:46 PDT
Comment on attachment 403404 [details] [diff] [review]
Add an input length check to _strtod - v1

Reed: I like your change to JS_strtod.

We should do the string length check in JS_strtod instead
of _strtod because I believe dtoa.c should be kept as close
to the upstream version as possible.

We can report a long input string as JSMSG_NEED_DIET, or
JSMSG_CANT_CONVERT, or JSMSG_CANT_CONVERT_TO.
Comment 32 Brian Crowder 2009-10-02 12:34:14 PDT
The ship has already sailed on keeping dtoa.c close-to-upstream, we should do the thing that least impacts perf here.  Can we emulate the length-check that NSPR got here instead, please?
Comment 33 Reed Loden [:reed] (use needinfo?) 2009-10-02 12:38:57 PDT
(In reply to comment #32)
> The ship has already sailed on keeping dtoa.c close-to-upstream, we should do
> the thing that least impacts perf here.  Can we emulate the length-check that
> NSPR got here instead, please?

The length-check in my patch is the exact same one that NSPR is using, so I'm not sure what you mean. wtc did post a new patch in bug 516396 to do the length-check inline, but it's not complete or ready yet.
Comment 34 Brian Crowder 2009-10-02 12:58:21 PDT
What's not ready about wtc's inline length-check?  Also, hasn't the actual security-sensitive part of this landed?  The length-check only prevents a hang/DoS, unless I am misunderstanding.

If I'm right, we shouldn't rush in a patch that -must- have performance implications when we have something potentially far better (even if it means waiting a short while for the "far better" bit).
Comment 35 Reed Loden [:reed] (use needinfo?) 2009-10-02 13:03:34 PDT
(In reply to comment #34)
> What's not ready about wtc's inline length-check?

Read the entire second paragraph in bug 516396, comment #57.
Comment 36 Brian Crowder 2009-10-02 13:05:41 PDT
JS doesn't allow strings that long anyway.
Comment 37 Reed Loden [:reed] (use needinfo?) 2009-10-02 17:28:24 PDT
Created attachment 404370 [details] [diff] [review]
Add an input length check to _strtod - v2

ok, how about this then?
Comment 38 Wan-Teh Chang 2009-10-02 17:59:54 PDT
Comment on attachment 404370 [details] [diff] [review]
Add an input length check to _strtod - v2

>  ret0:
>+			errno = EINVAL;
> 			s = s00;
> 			sign = 0;

Nit: You may want to put that inside #ifndef NO_ERRNO.
Comment 39 Reed Loden [:reed] (use needinfo?) 2009-10-02 18:29:38 PDT
(In reply to comment #38)
> Nit: You may want to put that inside #ifndef NO_ERRNO.

Yeah, I thought about that, but the comment describing NO_ERRNO clearly says:
 * #define NO_ERRNO if strtod should not assign errno = ERANGE when
 *      the result overflows to +-Infinity or underflows to 0.
... and this is for EINVAL, not ERANGE. I guess I can just update the comment unless somebody thinks that define is specific to ERANGE for some reason.
Comment 40 Reed Loden [:reed] (use needinfo?) 2009-10-02 18:43:03 PDT
Created attachment 404380 [details] [diff] [review]
Add an input length check to _strtod - v2.1

with the NO_ERRNO #ifndef
Comment 41 Daniel Veditz [:dveditz] 2009-10-05 14:21:23 PDT
The original crash/security bug is fixed everywhere. The hang that results from not crashing before we process the entire string has been moved to bug 520629 and can be fixed in the next release.
Comment 42 Al Billings [:abillings] 2009-10-05 14:36:28 PDT
Does this mean that I can undo Reed's removal of the Verified1.9.1 keyword now?
Comment 43 Blake Kaplan (:mrbkap) 2009-10-06 17:11:13 PDT
Comment on attachment 404380 [details] [diff] [review]
Add an input length check to _strtod - v2.1

>diff --git a/js/src/jsdtoa.cpp b/js/src/jsdtoa.cpp
>+        switch (errno) {
>+            case ERANGE:
>+                *err = JS_DTOA_ERANGE;
>+                break;
>+            case ENOMEM:
>+                *err = JS_DTOA_ENOMEM;
>+                break;
>+            case EINVAL:
>+                *err = JS_DTOA_EINVAL;
>+                break;
>+            default:
>+                *err = 0;
>+                break;

Nit: the case labels should be indented 2 past the switch and the actual case bodies should be indented 2 past the case labels.
Comment 44 Brian Crowder 2009-10-06 17:20:07 PDT
It is a little weird that we're reporting OOM for EINVAL here.  Is there a better error we could use?
Comment 45 Reed Loden [:reed] (use needinfo?) 2009-10-07 02:22:46 PDT
(In reply to comment #43)
> Nit: the case labels should be indented 2 past the switch and the actual case
> bodies should be indented 2 past the case labels.

I based the whitespacing in my patch on prevailing style of another switch in jsdtoa.cpp: http://mxr.mozilla.org/mozilla-central/source/js/src/jsdtoa.cpp#191

Do you still wish for me to change the whitespacing to meet your nit?
Comment 46 Reed Loden [:reed] (use needinfo?) 2009-10-07 02:24:39 PDT
(In reply to comment #44)
> It is a little weird that we're reporting OOM for EINVAL here.  Is there a
> better error we could use?

I asked mrbkap about this a while ago, and he recommended using OOM, but if something else is wanted, I can use another error or create a new one...
Comment 47 Reed Loden [:reed] (use needinfo?) 2009-10-07 02:27:57 PDT
Any new patches will be attached to bug 520629.
Comment 48 Wan-Teh Chang 2009-10-07 07:00:30 PDT
Re: reporting OOM for EINVAL

I already looked into better errors we could use in comment 31.
(Caveat: I am not familiar with the JS source code.)  I
suggested some alternative errors that we can report for EINVAL:
JSMSG_NEED_DIET, JSMSG_CANT_CONVERT, or JSMSG_CANT_CONVERT_TO.
Comment 49 Henrik Skupin (:whimboo) 2009-10-08 14:14:37 PDT
Verified fixed on trunk, 1.9.2, and 1.9.1 based on the JS testcase in bug 516396 comment 2. No crash happens anymore.

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.3a1pre)
Gecko/20091007 Minefield/3.7a1pre ID:20091007093342

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2b1pre)
Gecko/20091007 Namoroka/3.6b1pre ID:20091007034618

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1.4)
Gecko/20091006 Firefox/3.5.4 ID:20091006224018
Comment 50 Martin Stránský 2009-10-15 01:06:35 PDT
Comment on attachment 401451 [details] [diff] [review]
patch - v1 (1.9.0)

This patch is wrong. In Balloc(), the Bigint *rv is a local variable and when k > Kmax happens the rv variable is used w/o initialization.
Comment 51 Martin Stránský 2009-10-15 01:08:02 PDT
static Bigint *Balloc(int32 k)
{
    int32 x;
    Bigint *rv;

    [...]

    if (k <= Kmax && (rv = freelist[k]) != NULL)
        freelist[k] = rv->next;
    if (rv == NULL) { << here, if k > Kmax
Comment 52 Reed Loden [:reed] (use needinfo?) 2009-10-15 02:33:16 PDT
(In reply to comment #50)
> (From update of attachment 401451 [details] [diff] [review])
> This patch is wrong. In Balloc(), the Bigint *rv is a local variable and when k
> > Kmax happens the rv variable is used w/o initialization.

Huh? It's (rv = freelist[k]), so rv is set to freelist[k] during the |if|.
Comment 53 Martin Stránský 2009-10-15 02:37:48 PDT
Isn't short boolean evaluation by default in C++?
Comment 54 Martin Stránský 2009-10-15 02:40:29 PDT
Anyway, freelist[] is a static array of [Kmax+1] elements so it should not be accessed when k > Kmax+1, right?
Comment 55 Reed Loden [:reed] (use needinfo?) 2009-10-15 02:48:27 PDT
(In reply to comment #53)
> Isn't short boolean evaluation by default in C++?

This is C, not C++ (both nsprpub/pr/src/misc/prdtoa.c and js/src/dtoa.c).

(In reply to comment #54)
> Anyway, freelist[] is a static array of [Kmax+1] elements so it should not be
> accessed when k > Kmax+1, right?

Sure, which is why the check is for |k <= Kmax| before freelist[] is accessed.
Comment 56 Reed Loden [:reed] (use needinfo?) 2009-10-15 02:51:40 PDT
(In reply to comment #55)
> (In reply to comment #53)
> > Isn't short boolean evaluation by default in C++?
> 
> This is C, not C++ (both nsprpub/pr/src/misc/prdtoa.c and js/src/dtoa.c).

Ah, but js/src/jsdtoa.cpp is C++, and it |#include|s dtoa.c, so yes, it is compiled using C++. Sorry about that.
Comment 57 Martin Stránský 2009-10-15 03:21:01 PDT
Yes, but I'm still talking about uninitialized rv when (k > Kmax).
Comment 58 Reed Loden [:reed] (use needinfo?) 2009-10-15 03:27:09 PDT
Oh! I was looking at the NSPR code and the upstream dtoa.c, not the js code. My apologies. This is what I get for reading bugmail at 5am.

This problem is because we do the check slightly differently between NSPR and js code:

NSPR:
	if (k <= Kmax && (rv = freelist[k]))
 		freelist[k] = rv->next;
 	else {

js:
     if (k <= Kmax && (rv = freelist[k]) != NULL)
         freelist[k] = rv->next;
     if (rv == NULL) {

Martin correctly points out that rv could never be set because we'll short-circuit on |k <= Kmax| if that returns false. Simple fix would be just to swap to how NSPR does it, I think. Does that seem reasonable?
Comment 59 Reed Loden [:reed] (use needinfo?) 2009-10-15 03:29:03 PDT
No, my last comment is still wrong. Basically, the patch we use for 1.9.1 and greater is fine. It's the 1.9.0 patch (attachment 401451 [details] [diff] [review]) that is wrong. *sigh*
Comment 60 Reed Loden [:reed] (use needinfo?) 2009-10-15 03:38:05 PDT
Created attachment 406420 [details] [diff] [review]
Follow-up to 1.9.0 patch to fix use of uninitialized 'rv' - v1

Thanks, Martin, for noticing this. My apologies again for looking at the wrong patch when trying to check what you were seeing.

Anyway, this changes the code to look more like what NSPR and js >= 1.9.1 is using, which I believe should fix this issue.

I did say "untested patch" in comment #10! :)
Comment 61 Reed Loden [:reed] (use needinfo?) 2009-10-15 03:39:09 PDT
Doesn't look like this was ever verified on 1.9.0.15 either.
Comment 62 Wan-Teh Chang 2009-10-15 07:20:58 PDT
Comment on attachment 406420 [details] [diff] [review]
Follow-up to 1.9.0 patch to fix use of uninitialized 'rv' - v1

r=wtc.  Nice catch, Martin.
Comment 63 Brian Crowder 2009-10-15 08:44:18 PDT
Comment on attachment 406420 [details] [diff] [review]
Follow-up to 1.9.0 patch to fix use of uninitialized 'rv' - v1

Yes, thanks Martin!
Comment 64 Samuel Sidler (old account; do not CC) 2009-10-15 08:57:43 PDT
(In reply to comment #10)
> Untested patch for 1.9.0. This version of dtoa doesn't have any of the locking
> stuff, so definitely please take a closer look here to make sure something
> isn't missing or wrong or something.

Why was this untested? We should never land untested patches on stable branches.
Comment 65 Mike Shaver (:shaver -- probably not reading bugmail closely) 2009-10-15 09:00:47 PDT
It's OK that it was untested at that point.  It's not really OK that it was review-stamped without testing, or that it was approved based on a rubber stamp of an untested patch, though!

Thanks, Martin -- you saved us from an embarrassing error here.
Comment 66 Samuel Sidler (old account; do not CC) 2009-10-15 09:11:56 PDT
Did the new, follow-up patch get tested?
Comment 67 Mike Shaver (:shaver -- probably not reading bugmail closely) 2009-10-15 10:40:12 PDT
(In reply to comment #66)
> Did the new, follow-up patch get tested?

This is sort of important, folks -- has anyone tested the follow-up patch?  Can they chime in here if so, before we land another untested patch?
Comment 68 Reed Loden [:reed] (use needinfo?) 2009-10-15 10:44:13 PDT
(In reply to comment #67)
> (In reply to comment #66)
> > Did the new, follow-up patch get tested?
> 
> This is sort of important, folks -- has anyone tested the follow-up patch?  Can
> they chime in here if so, before we land another untested patch?

I just sent it off to the try server. Once a build pops out, I'll test it against the testcases.
Comment 69 Reed Loden [:reed] (use needinfo?) 2009-10-15 11:51:21 PDT
(In reply to comment #68) 
> I just sent it off to the try server. Once a build pops out, I'll test it
> against the testcases.

I can confirm that the follow-up patch does indeed fix the crash.
Comment 70 Samuel Sidler (old account; do not CC) 2009-10-15 12:50:43 PDT
Comment on attachment 406420 [details] [diff] [review]
Follow-up to 1.9.0 patch to fix use of uninitialized 'rv' - v1

Dan's looked over this patch as well, which makes me a bit more confident.

Approved for 1.9.0.15 and 1.9.0.16. a=ss for release-drivers. Please land on both HEAD and GECKO190_20091006_RELBRANCH as soon as possible.
Comment 71 Reed Loden [:reed] (use needinfo?) 2009-10-15 13:00:53 PDT
CVS HEAD:

Checking in js/src/jsdtoa.c;
/cvsroot/mozilla/js/src/jsdtoa.c,v  <--  jsdtoa.c
new revision: 3.43; previous revision: 3.42
done

CVS GECKO190_20091006_RELBRANCH:

Checking in js/src/jsdtoa.c;
/cvsroot/mozilla/js/src/jsdtoa.c,v  <--  jsdtoa.c
new revision: 3.42.2.1; previous revision: 3.42
done
Comment 72 Al Billings [:abillings] 2009-10-16 10:57:36 PDT
I've run the testcase in bug 516396, which doesn't crash on 1.9.0.15 (and does on 1.9.0.14) but takes five minutes to run. 

I find the comments above less than clear so, to be 100% certain, I ask, "How do we verify *this* fix for 1.9.0.15?"
Comment 73 Al Billings [:abillings] 2009-10-16 17:10:26 PDT
Per conversation with Sam, this is verified via the testcase in bug 516396 (which should be attached here) and is now verified for 1.9.0.
Comment 74 Al Billings [:abillings] 2009-10-16 17:11:28 PDT
I've also reverified this for 1.9.1 with build 3 of 1.9.1.
Comment 75 Al Billings [:abillings] 2009-11-23 10:38:50 PST
Verified this again for 1.9.0.16 (since it needed to land in 1.9.0.15 and .16 separately) with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.16pre) Gecko/2009111921 GranParadiso/3.0.16pre (.NET CLR 3.5.30729).
Comment 76 :Gavin Sharp [email: gavin@gavinsharp.com] 2010-02-18 23:18:55 PST
Created attachment 427723 [details] [diff] [review]
rollup for MOZILLA_1_8_BRANCH

Attachment 401451 [details] [diff] with attachment 406420 [details] [diff] [review] applied on top of it.
Comment 77 :Gavin Sharp [email: gavin@gavinsharp.com] 2010-02-19 08:54:18 PST
Landed on MOZILLA_1_8_BRANCH:
mozilla/js/src/jsdtoa.c 	3.33.2.5
Comment 78 Al Billings [:abillings] 2010-02-26 17:37:22 PST
Verified for 1.8.1.24 using Seamonkey (Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1.24pre) Gecko/20100225 SeaMonkey/1.1.19pre).

Note You need to log in before you can comment on or make changes to this bug.