Last Comment Bug 446494 - Buffer overflow in Number.toLocaleString()
: Buffer overflow in Number.toLocaleString()
Status: VERIFIED FIXED
: [sg:critical?]
: verified1.8.1.17, verified1.9.0.2
Product: Core
Classification: Components
Component: JavaScript Engine
: 1.9.0 Branch
: x86 Linux
: -- critical (vote)
: ---
Assigned To: Brian Crowder
: general
:
: 614931
:
  Show dependency treegraph
 
Reported: 2008-07-21 16:09 PDT by Philip Taylor
Modified: 2010-11-27 21:59 PST (History)
13 users (show)
dveditz: blocking1.9.0.2+
samuel.sidler+old: wanted1.9.0.x+
dveditz: blocking1.8.1.17+
asac: blocking1.8.0.next+
bclary: in‑testsuite+
bclary: in‑litmus-
See Also:
Crash Signature:


Attachments
v1 (2.79 KB, patch)
2008-07-22 09:48 PDT, Brian Crowder
no flags Details | Diff | Splinter Review
v2 (2.79 KB, patch)
2008-07-22 10:20 PDT, Brian Crowder
no flags Details | Diff | Splinter Review
v3, leave room for '\0' (2.79 KB, patch)
2008-07-22 10:26 PDT, Brian Crowder
mrbkap: review+
Details | Diff | Splinter Review
1.9 branch patch (jsnum.cpp => jsnum.c (2.80 KB, patch)
2008-07-24 12:52 PDT, Brian Crowder
samuel.sidler+old: approval1.9.0.2+
Details | Diff | Splinter Review
1.8 branch patch (2.88 KB, patch)
2008-07-24 12:58 PDT, Brian Crowder
samuel.sidler+old: approval1.8.1.17+
Details | Diff | Splinter Review
ecma_3/Number/15.7.4.3-02.js (2.23 KB, text/plain)
2008-08-04 12:07 PDT, Bob Clary [:bc:]
no flags Details
for 1.8.0 (just context changes in backport) (2.68 KB, patch)
2008-08-31 06:01 PDT, Alexander Sack
asac: approval1.8.0.next+
Details | Diff | Splinter Review

Summon comment box

Description Philip Taylor 2008-07-21 16:09:02 PDT
Using SpiderMonkey from CVS trunk:

$ LANG=C valgrind ./js -e '1e-10.toLocaleString()'

==3190== Invalid read of size 1
==3190==    at 0x80A5DD0: num_toLocaleString (jsnum.c:367)
==3190==    by 0x81226A4: js_Interpret (jsinterp.c:4835)
==3190==    by 0x80A0D96: js_Execute (jsinterp.c:1536)
==3190==    by 0x80620DB: JS_EvaluateUCScriptForPrincipals (jsapi.c:4999)
==3190==    by 0x8062049: JS_EvaluateUCScript (jsapi.c:4980)
==3190==    by 0x8061F53: JS_EvaluateScript (jsapi.c:4946)
==3190==    by 0x804A076: ProcessArgs (js.c:518)
==3190==    by 0x804F4D5: main (js.c:3931)
==3190==  Address 0x41cc0ce is 0 bytes after a block of size 6 alloc'd
==3190==    at 0x402291D: malloc (in /usr/lib/valgrind/x86-linux/vgpreload_memcheck.so)
==3190==    by 0x805BBE0: JS_malloc (jsapi.c:1791)
==3190==    by 0x80F6F62: js_DeflateString (jsstr.c:2927)
==3190==    by 0x80F7B08: js_GetStringBytes (jsstr.c:3232)
==3190==    by 0x80A5BDB: num_toLocaleString (jsnum.c:323)
==3190==    by 0x81226A4: js_Interpret (jsinterp.c:4835)
==3190==    by 0x80A0D96: js_Execute (jsinterp.c:1536)
==3190==    by 0x80620DB: JS_EvaluateUCScriptForPrincipals (jsapi.c:4999)
==3190==    by 0x8062049: JS_EvaluateUCScript (jsapi.c:4980)
==3190==    by 0x8061F53: JS_EvaluateScript (jsapi.c:4946)
==3190==    by 0x804A076: ProcessArgs (js.c:518)
==3190==    by 0x804F4D5: main (js.c:3931)
==3190==
==3190== Invalid write of size 1
==3190==    at 0x80A5E80: num_toLocaleString (jsnum.c:384)
==3190==    by 0x81226A4: js_Interpret (jsinterp.c:4835)
==3190==    by 0x80A0D96: js_Execute (jsinterp.c:1536)
==3190==    by 0x80620DB: JS_EvaluateUCScriptForPrincipals (jsapi.c:4999)
==3190==    by 0x8062049: JS_EvaluateUCScript (jsapi.c:4980)
==3190==    by 0x8061F53: JS_EvaluateScript (jsapi.c:4946)
==3190==    by 0x804A076: ProcessArgs (js.c:518)
==3190==    by 0x804F4D5: main (js.c:3931)
==3190==  Address 0x41cc146 is 0 bytes after a block of size 6 alloc'd
==3190==    at 0x402291D: malloc (in /usr/lib/valgrind/x86-linux/vgpreload_memcheck.so)
==3190==    by 0x805BBE0: JS_malloc (jsapi.c:1791)
==3190==    by 0x80A5D98: num_toLocaleString (jsnum.c:360)
==3190==    by 0x81226A4: js_Interpret (jsinterp.c:4835)
==3190==    by 0x80A0D96: js_Execute (jsinterp.c:1536)
==3190==    by 0x80620DB: JS_EvaluateUCScriptForPrincipals (jsapi.c:4999)
==3190==    by 0x8062049: JS_EvaluateUCScript (jsapi.c:4980)
==3190==    by 0x8061F53: JS_EvaluateScript (jsapi.c:4946)
==3190==    by 0x804A076: ProcessArgs (js.c:518)
==3190==    by 0x804F4D5: main (js.c:3931)

jsnum.c:367 is the line "while (*tmpSrc == '-' || remainder--)".
jsnum.c:384 is the line "*tmpDest++ = '\0';".
Comment 1 Brian Crowder 2008-07-21 16:44:56 PDT
So we don't handle exponential notation in this routine at all.
Comment 2 Philip Taylor 2008-07-22 05:26:15 PDT
Non-finite values aren't handled properly either -- Infinity.toLocaleString() == "In,fin,ity" in my default locale (en_GB).
Comment 3 Brian Crowder 2008-07-22 09:48:33 PDT
Created attachment 330789 [details] [review]
v1

Will ask for review after testing.
Comment 4 Brian Crowder 2008-07-22 10:20:33 PDT
Created attachment 330792 [details] [review]
v2
Comment 5 Brian Crowder 2008-07-22 10:26:07 PDT
Created attachment 330794 [details] [review]
v3, leave room for '\0'

Since we use strcpy() later, we need the '\0'.  It's semi-extraneous, since we use JS_NewString at the end (ie., we don't need the terminator), but the string routines are convenient and clean, so I think it's a worthwhile tradeoff.
Comment 6 Blake Kaplan (:mrbkap) 2008-07-24 12:29:38 PDT
Comment on attachment 330794 [details] [review]
v3, leave room for '\0'

>--- mozilla.7c4f66de2dee/js/src/jsnum.cpp	2008-07-22 10:24:36.000000000 -0700

>+    while(*nint >= '0' && *nint <= '9')
>+        nint++;

Nit: space after while. r=mrbkap with that.
Comment 7 Brian Crowder 2008-07-24 12:50:38 PDT
changeset:   16181:c37a070f8397
Comment 8 Brian Crowder 2008-07-24 12:52:36 PDT
Created attachment 331172 [details] [review]
1.9 branch patch (jsnum.cpp => jsnum.c
Comment 9 Brian Crowder 2008-07-24 12:58:23 PDT
Created attachment 331175 [details] [review]
1.8 branch patch
Comment 10 Brendan Eich [:brendan] 2008-07-24 16:49:31 PDT
(In reply to comment #6)
> (From update of attachment 330794 [details] [review])
> >--- mozilla.7c4f66de2dee/js/src/jsnum.cpp	2008-07-22 10:24:36.000000000 -0700
> 
> >+    while(*nint >= '0' && *nint <= '9')
> >+        nint++;
> 
> Nit: space after while. r=mrbkap with that.

Uber-nit: usually SpiderMonkey style uses number-line order: '0' <= c && c <= '9'. Could also rearrange (check that gcc doesn't do this) to avoid a branch:

(uintN)(c - '0') <= '9' - '0'

or

(uintN)(c - '0') < 10

/be
Comment 11 Samuel Sidler (old account; do not CC) 2008-07-29 17:06:34 PDT
Can we get an automated test for this before landing on the 1.9 branch?
Comment 12 Daniel Veditz 2008-08-04 11:31:51 PDT
QA doesn't want to take on the 1.8 branch without a testcase, and not on the 1.9.0 branch without an automated one (which was probably a requirement for mozilla-central, right?)
Comment 13 Brian Crowder 2008-08-04 11:40:01 PDT
The original bug will be difficult to write a testcase for, but the In,fin,ity one and similar bugs may not be.  The reason for difficulty in writing a case for the original bug is that the overflow is caused by an off-by-one in the buffer math, which means we usually don't overflow the space provided by malloc().  I'll see if I can figure out something more consistent/reliable.
Comment 14 Bob Clary [:bc:] 2008-08-04 12:07:08 PDT
Created attachment 332233 [details]
ecma_3/Number/15.7.4.3-02.js

Even if we can't get a reliable crash for the overrun, we can still detect this using valgrind. I'll check this test in later today.
Comment 15 Brian Crowder 2008-08-04 13:44:31 PDT
dveditz:  Am I cleared-to-land for branches, once the testcase is landed?
Comment 16 Samuel Sidler (old account; do not CC) 2008-08-04 16:06:26 PDT
Comment on attachment 331172 [details] [review]
1.9 branch patch (jsnum.cpp => jsnum.c

Approved for 1.9.0.2. Please land in CVS. a=ss

(After the test lands.)
Comment 17 Samuel Sidler (old account; do not CC) 2008-08-04 16:07:19 PDT
Comment on attachment 331175 [details] [review]
1.8 branch patch

Approved for 1.8.1.17. Please land in CVS. a=ss

(After the test lands.)
Comment 18 Bob Clary [:bc:] 2008-08-04 16:49:39 PDT
/cvsroot/mozilla/js/tests/ecma_3/Number/15.7.4.3-02.js,v  <--  15.7.4.3-02.js
initial revision: 1.1

mozilla-central: changeset:   16381:779beff502f0

Typically I don't check in tests for outstanding security bugs, but here it is.
Comment 19 Brian Crowder 2008-08-05 07:52:26 PDT
Yeah, I confess I'm not sure I understand the procedure here, and why we went this route.  Fortunately, the test doesn't -look- like an obvious security bug (or so it seems to me), but it still seems odd that we're landing a test for it before the fix has shipped in all relevant browsers.
Comment 20 Reed Loden [:reed] (very busy) 2008-08-05 09:29:07 PDT
Maybe we need to set some type of policy for tests in relation to security fixes?
Comment 21 Samuel Sidler (old account; do not CC) 2008-08-05 11:13:35 PDT
We have a policy and the policy is to land the test after fixed releases are out. This was my bad for not noticing the s-s of this bug and pushing to get the test landed.
Comment 22 Brian Crowder 2008-08-05 14:12:17 PDT
> or
> (uintN)(c - '0') < 10

For what it's worth, gcc -does- do this optimization if -fif-conversion is enabled (which it is for -0 and up).
Comment 23 Bob Clary [:bc:] 2008-08-21 11:57:05 PDT
verified 1.8.1, 1.9.0, 1.9.1 shell/browser linux
Comment 24 Alexander Sack 2008-08-31 06:01:34 PDT
Created attachment 336242 [details] [review]
for 1.8.0 (just context changes in backport)

a=asac for 1.8.0.15

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