Bugzilla@Mozilla – Bug 446494
Buffer overflow in Number.toLocaleString()
Last modified: 2010-11-27 21:59:33 PST
Summon comment box
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';".
So we don't handle exponential notation in this routine at all.
Non-finite values aren't handled properly either -- Infinity.toLocaleString() == "In,fin,ity" in my default locale (en_GB).
Created attachment 330789 [details] [review] v1 Will ask for review after testing.
Created attachment 330792 [details] [review] v2
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 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.
changeset: 16181:c37a070f8397
Created attachment 331172 [details] [review] 1.9 branch patch (jsnum.cpp => jsnum.c
Created attachment 331175 [details] [review] 1.8 branch patch
(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
Can we get an automated test for this before landing on the 1.9 branch?
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?)
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.
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.
dveditz: Am I cleared-to-land for branches, once the testcase is landed?
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 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.)
/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.
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.
Maybe we need to set some type of policy for tests in relation to security fixes?
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.
> 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).
verified 1.8.1, 1.9.0, 1.9.1 shell/browser linux
Created attachment 336242 [details] [review] for 1.8.0 (just context changes in backport) a=asac for 1.8.0.15