Closed Bug 501124 Opened 16 years ago Closed 16 years ago

Missing strtointeger path in js_StringToInt32

Categories

(Core :: JavaScript Engine, defect)

Product:
Core
Component:
JavaScript Engine
Platform:
x86
Windows XP
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

()

Status:
VERIFIED FIXED
Tracking Flags:
Tracking Status
blocking1.9.1 --- .2+
status1.9.1 --- .2-fixed

People

(Reporter: bugs, Assigned: gal)

References

Details

(Keywords: regression, testcase, verified1.9.1, Whiteboard: fixed-in-tracemonkey)

Attachments

(2 files, 5 obsolete files)

clean patch
905 bytes, patch
brendan
: review+
Details | Diff | Splinter Review
patch for 1.9.1
837 bytes, patch
beltzner
: approval1.9.1.2+
Details | Diff | Splinter Review
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.9.0.11) Gecko/2009060215 Firefox/3.0.11 (.NET CLR 3.5.30729) Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.9.1) Gecko/20090624 Firefox/3.5 (.NET CLR 3.5.30729) As with FF 3.5 RC3 our ECMAScript/JavaScript based cryptographic login functions do not work anymore. It seems that some basic operators do not work correctly, at least when the calculation is performed without artificial delays. The bug is possibly exploitable. Reproducible: Always Steps to Reproduce: The script below shows at least a part of the problem. Compare the results of the script with FF < 3.5, Internet Explorer, and other browsers. <!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN" "http://www.w3.org/TR/html4/loose.dtd"> <html> <head> <script type="text/javascript"> function doTheTest() { var hexVal = "00000000000000000000000000000000DEADBABE"; var nblk = (((hexVal.length/2) + 8) >> 6) + 1; var blks = new Array(nblk * 16); for(var i = 0; i < nblk * 16; i++) blks[i] = 0; for(i = 0; i < hexVal.length; i++) { blks[i >> 3] |= ("0x"+hexVal.charAt(i)) << (28 - (i % 8) * 4); slowDown(300); // uncomment this line to make the script work (300 suffices on my machine) } alert(blks); } function slowDown(millis) { var date = new Date(); var curDate = null; do { curDate = new Date(); } while(curDate - date < millis); } </script> </head> <body> <form> <input type="button" name="go" onclick="doTheTest();" value="go"/> </form> </body> </html> Actual Results: FF 3.5 RC3 does not calculate the same results as older FF versions, and other browsers. Expected Results: FF 3.5 should calculate the same results as older FF versions, and other browsers.
Attached file testcase (obsolete) —
Assignee: nobody → general
Component: General → JavaScript Engine
Product: Firefox → Core
QA Contact: general → general
Version: unspecified → Trunk
Attached file JS shell testcase (obsolete) —
This testcase behaves differently on current mozilla-1.9.1, tracemonkey and m-c tips depending on whether the JIT is enabled: [gavin@gavin-mbp:~/obj-js-tracemonkey]$ ./js -f ~/test_501124.js 0,0,0,0,-559039810,0,0,0,0,0,0,0,0,0,0,0 [gavin@gavin-mbp:~/obj-js-tracemonkey]$ ./js -j -f ~/test_501124.js 0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0 The non-JIT behavior is consistent with Firefox 3.
Attachment #385746 - Attachment is obsolete: true
Status: UNCONFIRMED → NEW
Ever confirmed: true
The previous script was accidentally posted with uncommented slowDown-call.
Do sites use this? The testcase gives me a slow script warning, which makes me wonder if it's a problem with the wider web or not.
bogomip, can you provide more information about what application this breaks?
Flags: blocking1.9.1?
Keywords: regression, testcase
Attachment #385750 - Attachment is obsolete: true
Did this just break in rc3?
Keywords: regressionwindow-wanted
Copying some of our best regression window finders.
> Do sites use this? The testcase gives me a slow script warning, which makes me > wonder if it's a problem with the wider web or not. The script was accidentally posted with the uncommented slowDown-call. If you remove the respective line, the script will work as in production environments. See also Comment #3.
(In reply to comment #6) > Did this just break in rc3? A colleague of mine said that it worked in a previous RC or beta. He will try to find out which version it was.
(In reply to comment #9) > A colleague of mine said that it worked in a previous RC or beta. He will try > to find out which version it was. It worked in Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1a2) Gecko/20080829071937 Shiretoko/3.1a2
Well, Shiretoko/3.1a2 was the last release where JIT was off by default, so that basically puts the range at "where JIT was turned on."
Assignee: general → gal
Confirmed. This is bad. Digging into it. Doesn't look hard.
(In reply to comment #5) > bogomip, can you provide more information about what application this breaks? The application is a portal software. It has about 4000 installations. The script is based upon a popular SHA-1 script from http://pajhome.org.uk/crypt/md5/index.html.
(In reply to comment #12) > Confirmed. This is bad. Digging into it. Doesn't look hard. Note that it is probably a race condition ;)
partially reduced testcase var hexVal = "DEADBABE"; var blks = new Array(4); for(var i = 0; i < 4; i++) blks[i] = 0; for(i = 0; i < hexVal.length; i++) { print(("0x"+hexVal.charAt(i)) << (28 - (i % 8) * 4)); }
var hexVal = "DEAD"; for(i = 0; i < hexVal.length; i++) print(("0x"+hexVal.charAt(i)) << (28 - (i % 8) * 4));
for(i = 0; i < 4; i++) print(("0x"+'D') << (28 - (i % 8) * 4));
This breaks: for(i = 0; i < 4; i++) print(("0x" + "D") << (28 - (i % 8) * 4)); This works: for(i = 0; i < 4; i++) print(("0xD") << (28 - (i % 8) * 4));
This smells like a register allocation bug to me (we had one that had just the same symptoms), but I have to reduce further. The shift op (<<) requires ECX, and the concat op might be spilling it or something weird. Just a guess. I will reduce a bit more.
FWIW, this worked in 9a9070f84098 (with JIT enabled) and bisection points to ce02fcaa233d, but that's just when we started tracing charAt.
I would say this qualifies for scary: for(i = 0; i < 4; i++) print(("0x" + "D") << 16); 851968 851968 851968 851968 851968 851968 851968 0
without jit: 851968 851968 851968 851968 with jit: 851968 851968 851968 0
This still breaks. I am betting on regalloc. Digging into the assembly now. for(i = 0; i < 4; i++) print(("0xD" + "") << 16);
Much easier to look at at the assembly level: var x = 0; for(i = 0; i < 4; i++) x = ("0xD" + "") << 16; print(x);
Ok, this is fortunately not regalloc, but a highly specific number conversion bug. When converting a hex number to an integer, we fail: js_StringToInt32(JSContext* cx, JSString* str) { const jschar* bp; const jschar* end; const jschar* ep; jsdouble d; str->getCharsAndEnd(bp, end); - if (!js_strtod(cx, bp, end, &ep, &d) || js_SkipWhiteSpace(ep, end) != end) + if ((!js_strtod(cx, bp, end, &ep, &d) || + js_SkipWhiteSpace(ep, end) != end) && + (!js_strtointeger(cx, bp, end, &ep, 0, &d) || + js_SkipWhiteSpace(ep, end) != end)) { return 0; + } return js_DoubleToECMAInt32(d); } JS_DEFINE_CALLINFO_2(extern, INT32, js_StringToInt32, CONTEXT, STRING, 1, 1) We have a specialized function to convert strings to numbers that is used when we know that the result is used like an integer. The << op forces this to happen. In the integer special casing somehow we lost strtointeger, which does the hex conversion. Patch in a sec.
Attached patch patch (obsolete) — — Splinter Review
Awesome original shell testcase by the way, sped up identifying the issue tremendously. Thanks.
Attachment #385749 - Attachment is obsolete: true
patch contains some accidental changes, please ignore
Attached patch clean patch — — Splinter Review
Attachment #385774 - Attachment is obsolete: true
regressor: http://hg.mozilla.org/tracemonkey/rev/3e6f2131c8b6 this is ancient
A Whistler-era bug from me, wonderful. The workaround would be to use decimal, I guess? Definitely will take this fix for 3.5.1 (subject to test and review), need to decide if we want to stop-ship for this now, hrm.
Flags: blocking1.9.1.1+
Note: I simply copied the code from a working function above: jsdouble FASTCALL js_StringToNumber(JSContext* cx, JSString* str) { const jschar* bp; const jschar* end; const jschar* ep; jsdouble d; str->getCharsAndEnd(bp, end); if ((!js_strtod(cx, bp, end, &ep, &d) || js_SkipWhiteSpace(ep, end) != end) && (!js_strtointeger(cx, bp, end, &ep, 0, &d) || js_SkipWhiteSpace(ep, end) != end)) { return js_NaN; } return d; } JS_DEFINE_CALLINFO_2(extern, DOUBLE, js_StringToNumber, CONTEXT, STRING, 1, 1) So this should be a very safe and minimal fix.
Simple work around: var x = 0; for(i = 0; i < 4; i++) x = ("0xD" + "") * 1 << 16; print(x); The multiplication does not force the result to be an integer, so we take the js_StringToNumber slow path instead of the buggy js_StringToInt32 path.
Comment on attachment 385776 [details] [diff] [review] clean patch This excerpts from js_ValueToNumber (could have an inline helper for use here and there as a followup). Better than a "bigger" (in control flow novelty) change (second patch, which is deceptively short-looking but IMHO bigger in possible unwanted effects). /be
Attachment #385776 - Flags: review+
Attachment #385779 - Attachment is obsolete: true
Keywords: regressionwindow-wanted
OS: Windows XP → All
Priority: -- → P2
Hardware: x86 → All
Summary: Firefox 3.5 RC3 JavaScript breaks cryptographic login routines → Missing strtointeger path in js_StringToInt32
Target Milestone: --- → mozilla1.9.1
I don't think we need to stop the ship of Firefox 3.5 for this, as it hasn't expressed itself in the large since we started shipping it in nightlies back in October or through a near-year's worth of betas. That said, and as Shaver's blocking flag indicates, let's take the fix in 3.5.1.
Flags: blocking1.9.1? → blocking1.9.1-
OS: All → Windows XP
Priority: P2 → --
Hardware: All → x86
Target Milestone: mozilla1.9.1 → ---
http://hg.mozilla.org/tracemonkey/rev/0b78e49bcc40
Whiteboard: fixed-in-tracemonkey
We should land this on trunk soon.
http://hg.mozilla.org/mozilla-central/rev/0b78e49bcc40
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Andreas: Please request approval on a patch that applies to 1.9.1.
blocking1.9.1: --- → .2+
Flags: blocking1.9.1.1+ → blocking1.9.1.1-
This fix is trivially safe and the bug is biting other sites (see dups). There's no reason for this fix to miss 1.9.1.1. Andreas is gonna get a patch attached (but it may well be the same as the patch attached here -- Sam, did that patch not apply or what?). /be
Attached patch patch for 1.9.1 — — Splinter Review
Trivial conflict resolved.
Attachment #388595 - Flags: approval1.9.1.1?
(In reply to comment #42) > Sam, did that patch not apply or what? I don't test to see if patches apply. If the patch here applies, you can request approval on it. It's simply a way of making sure developers check to see if their patches apply before requesting approval. This missed 1.9.1.1 because we're in firedrill mode. It'll block 1.9.1.2 though.
The patch should apply cleanly. Could someone please land on 1.9.1? The conflict was a simple renaming that 1.9.1 doesn't have yet. This is not security sensitive but it bites crypto algorithms in JS in particular. The fix has been on m-c for a while and is very safe.
Flags: blocking1.9.1.1- → blocking1.9.1.1?
Removing blocking 1.9.1.1? as per #44.
Well I guess I can't take that back?
You should be able to *remove* it, but not put it back to minus.
Flags: blocking1.9.1.1? → blocking1.9.1.1-
In general, if people want things on the branch, they should be asking for approval when they know they have a patch that works on the branch, and landing as early as they can -- waiting until we're at the cusp of code freeze to try to get it in is worse for everyone. If we'd had a 1.9.1 patch with an approval request any time in the 9 days after the m-c landing, this would have been in 3.5.1 for sure!
Comment on attachment 388595 [details] [diff] [review] patch for 1.9.1 a=beltzner for 1.9.1.2, please land on mozilla-1.9.1 (and be careful with that merge!)
Attachment #388595 - Flags: approval1.9.1.1? → approval1.9.1.2+
v 1.9.1.2 on mac shell and browser.
Keywords: verified1.9.1
http://hg.mozilla.org/tracemonkey/rev/58d20947e6f2 /cvsroot/mozilla/js/tests/js1_5/Regress/regress-501124.js,v <-- regress-501124.js initial revision: 1.1
Flags: in-testsuite+
v 1.9.2,1.9.3
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: