Last Comment Bug 358569 - Crash @ js_dtoa/jsdtoa.c when running with reduced CPU float precision, e.g. after loading Direct3D plugin
: Crash @ js_dtoa/jsdtoa.c when running with reduced CPU float precision, e.g. ...
Status: RESOLVED FIXED
[sg:moderate] needs landing
: crash, fixed1.8.0.9, fixed1.8.1.1
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86 Windows XP
: -- critical (vote)
: mozilla1.9alpha1
Assigned To: Ben Bucksch (:BenB)
:
Mentors:
: 345308 (view as bug list)
Depends on:
Blocks: 362134
  Show dependency treegraph
 
Reported: 2006-10-28 19:21 PDT by Ben Bucksch (:BenB)
Modified: 2007-05-23 13:11 PDT (History)
14 users (show)
dveditz: blocking1.8.1.1+
dveditz: blocking1.8.0.9+
bob: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Fix, v1, by Keith Victor (742 bytes, patch)
2006-11-03 15:24 PST, Ben Bucksch (:BenB)
no flags Details | Diff | Splinter Review
Fix, v1, by Keith Victor (2.78 KB, patch)
2006-11-04 17:36 PST, Ben Bucksch (:BenB)
igor: review+
crowderbt: review+
Details | Diff | Splinter Review
Fix, v2, by Keith Victor, comment changed (780 bytes, patch)
2006-11-09 10:20 PST, Ben Bucksch (:BenB)
dveditz: approval1.8.0.9+
dveditz: approval1.8.1.1+
Details | Diff | Splinter Review
NSPR Windows test program that reproduces the crash (76.14 KB, text/plain)
2006-11-10 10:35 PST, Wan-Teh Chang
no flags Details
(correct) NSPR Windows test program that reproduces the crash (599 bytes, text/plain)
2006-11-10 10:36 PST, Wan-Teh Chang
no flags Details

Description Ben Bucksch (:BenB) 2006-10-28 19:21:05 PDT
Attachment:
I'll attach 2 plugins, based on the example plugin. Plugin "basic-crash" sets up Direct3D and creates a Direct3D device. The other plugin "basic" is identical, just that it does not call CreateDevice (see #ifdef FIREFOX_CRASH at bottom of dx8test.cpp) (and of course has a different mimetype to differentiate them).

Reproduction:
1. Make sure you have DirectX 8 and 3D drivers installed and working.
2. Copy the attached plugins (plugings/np*.dll) into your plugins dir
3. Load test.html
4. Click on first link for "good plugin" (calls the "basic" plugin)
5. Click on last link for a new window/tab with a normal HTML page
6. Click on first link for "crash plugin" (calls the "basic_crash" plugin)
7. Click on last link for a new window/tab with a normal HTML page
8. Crash.

Builds:
I tested this with Firefox 2.0 all the way back to Firebird 0.6, they all consistently crash.
HOWEVER, no Mozilla or Seamonkey does. I tested Mozilla 1.4 up to Seamonkey 1.5 trunk nightlies.

So, this is clearly caused or at least triggered by Firefox or toolkit changes.

Stacktrace:
If you do that with a debug build and debugger, you'll see an infinite recursion in NTDLL. I didn't see anything else in the stack dropdown the Visual Studio debugger, only 100 or so times NTDLL, or is there a way to see the full stack?

I have no idea how to further localize this problem, other than it being a bug introduced by Firefox changes.
Comment 1 Ben Bucksch (:BenB) 2006-10-28 19:30:14 PDT
Too bad bugzilla doesn't let me attach them. I put them on my webserver, but I can't garantee that the URL is stable.
Testcase created by Keith Victor.
http://download.developer.beonex.com/manyone/testcase/358569-d3dplugincrash-binaries.zip
http://download.developer.beonex.com/manyone/testcase/358569-d3dplugincrash-source.zip
Comment 2 Ben Bucksch (:BenB) 2006-10-28 19:39:02 PDT
FWIW, the crash reporter of Firefox (Quality Feedback Agent) doesn't come up either, so there's no incident ID.
The results with my debug build (mentioned above) are not 100% consistent either, the debugger somestimes doesn't catch the crash. When it does, it reports "Access violation" and the mentioned infinite NTDLL stacktrace. On related cases, I sometimes (rarely) instead saw "WS2HELP" as the only stacktrace entry.
Comment 3 Ben Bucksch (:BenB) 2006-10-28 19:45:32 PDT
bz, any ideas? What could cause it or how to localize the problem?
Comment 4 Boris Zbarsky [:bz] (still a bit busy) 2006-10-28 23:17:29 PDT
Not offhand, no.
Comment 5 timeless 2006-10-29 07:34:21 PST
so, let's start with basics.

when an exception happens, if there's space for an exception handler, one is added to the stack and it handles the exception.

if you crash because of infinite recursion, there's no stack space for the exception handler, and thus you can't catch it with talkback.

good, now that we've settled the basics. use windbg.
http://developer.mozilla.org/en/docs/How_to_get_a_stacktrace_with_WinDbg

don't skip steps, but feel free to substitute your own symboled firefox for the one available there.
Comment 6 Ben Bucksch (:BenB) 2006-10-29 09:33:37 PST
Thanks, timeless. Indeed WinDbg catches something, gives me:
js3250.dll, jsdtoa.c, line 2394:
*s++ = '0' + (char)L;
(search for "Check_FLT_ROUNDS")
Comment 7 Brendan Eich [:brendan] 2006-10-29 10:55:59 PST
(In reply to comment #6)
> Thanks, timeless. Indeed WinDbg catches something, gives me:
> js3250.dll, jsdtoa.c, line 2394:
> *s++ = '0' + (char)L;
> (search for "Check_FLT_ROUNDS")

What's the stack backtrace?  That's mandatory for bugs like this, please include it in the future.

/be
Comment 8 Ben Bucksch (:BenB) 2006-10-29 11:45:02 PST
Sorry. (And I am aware that not all crashes in JS eng are JS eng bugs.) Here's all that WinDbg gave me so far:

(144.588): Access violation - code c0000005 (first chance)
First chance exceptions are reported before any exception handling.
This exception may be expected and handled.
eax=00165000 ebx=c0000000 ecx=00165000 edx=00000037 esi=0012fbe8 edi=0012fbe0
eip=1003b3dc esp=0012d688 ebp=0012d77c iopl=0         nv up ei pl nz na po nc
cs=001b  ss=0023  ds=0023  es=0023  fs=0038  gs=0000             efl=00010202
*** WARNING: Unable to verify checksum for D:\firefox\2.0\debug\dist\bin\js3250.dll
js3250!js_dtoa+0x9ec:
1003b3dc 8810            mov     byte ptr [eax],dl          ds:0023:00165000=??
> kp
ChildEBP RetAddr  
0012d77c 1003a5f2 js3250!js_dtoa(double d = 614392135680, int mode = 0, int biasUp = 0, int ndigits = 0, int * decpt = 0x0012d7d8, int * sign = 0x0012d7c8, char ** rve = 0x0012d7d0, char * buf = 0x0012d812 "1162146954127074568435940754584148853492109305004538761240507369235060330867665185145493591991413595810327477489008667529237287506436002096103435731301565943423209993860238121124159982815589127230019825950735979787827677740797995192978354319593114438439249011638145044801458359520529912308290120533811699539168349926059315847863352916074145859807158156720695744581781693193343782976440668260094560582106489578044308103342932319208942400848455273875998221790077867256240521315295735658399287772250034388049398940169478717939562876652952459733888927161893843618192688947346981657436904920023502520357316685675370491782149323908266868361439422236391104685306009598664551281239306749369055753835465513743004199022608347189275834214038217711117527085609939346869984110580466097275345577614392125211904103287619944334946888019619381249645712041846833336380029103227064677215199253557215393240110273495490962687122824951547892188755604413351544536745353009397337945553392284857596498193142980679534911131578002050490377057440994367224678380166990971411278457279277737379636937556951151740153854937571421502073349335186749814700563871751488962215787897932330848135458194684387456953694536884289310248467290555168572748475039534984531898310552030079843821616446757340855352283258518028534374784914471292710726303424334591682870115081216405836662463042144366765926069277317949209777067192749240500343775636410647529119322706598793526161814151844031017139696014327742647683259975548959429723223861277522106970612263515927712396265650440595908369314227813360483645196142432274277466584863195891822802595907215880827960856309108664980148850608525342352194522770457074281126112955752786889621319018180210894848888074145335519148413351806680739723203938333421471979213417722796042324721831090277872645924273590785551219111162687940714213972157340273392604673293524735750936152907549223146410833651372255696698523570258369130727019706937378693219348468014072414398678811918167313408875435660784796244332891679042814406857713152288889371233649681426540481167935408418780809034058796540817760833135577732356957679125966485589256204869731141658094886165002871525415962236759417035454451268053462172455497642116474642648889122721142542830921928974776683151447931431694231434092849043944771678822613642498156337965476335050434933597318179896323814943571373791860498901980340635151307946528567771930218701497366550705833228376707740588279980807566052789511061270978992482933632550315133783094405392852661532326077115427246487715244198149669021884829746318747700246939651481307605741373139121918980588937962225749369580041843425464432661070952620795149619738814088719753599011629756437506803003698327486532974411673822845902096208293331797752133102437487011125391404578056628871911396762853741565552828675355811716840707345799388225821852376372065170439217030067391978673401265919610048923279016379805758307642231818002739404856243834815396556231828619571566205567034256977408084809351515176617426704373610509248178019897253902196871580571059743341840489451690891719517879112492282215184966711347689499002643460119867966126451976104389935524810724101414601132619369086162538717881891216935367524308980214596322294508425937340420193240760390636496106476601913022055747675081084809550744630540457050688164689871509267891247658660383191608389761461232616834153745099254008103133217125487810664594692283232303624481523664839901776392643470605631272625256840831077747393538430152614996524459975428373182428108810207939830787816143404304168561476279685633427448862881056969858333909059801338559704188012562464982010073014315159735351690813076319557725001197998087048061481155562213428601771988624385578691803770941411517008322547739516110164443587233502146802083572949277305366297474721816410214467216368415598380911479914941264506653843749264722450798658417113573363989034693185007605846230719750453283678609018697158848780856744267733174846906264822065499429938181902247937235562598780314106178544185262671960417689765865251887...3581778285323300617034536947203851039490092893346278587250279366866327201844589383868988487860996241554162673722025338970536241774363649033930870181770651689974217267832003115712459180240779265007663517909521174786545006822035352870461111296577979296569800400084174438748054665413472917217094030526930420342662835181769078822508784917354876911269151163767318578355210882790266034219228668211860063061359520739627521345704439670801321782488800420531406528407864084297908570116549222622122856539900255957405971928750696029007806648548439859807158156720695744581781693193343782976440668260094560582106489578044308103342932319208942400848455273875998221790077867256240521315295735658399287772250034388049398940169478717939562876652952459733888927161893843618192688947346981657436904920023502520357316685675370491782149323908266868361439422236391104685306009598664551281239306749369055753835465513743004199022608347189275834214038217711117527085609939346869984110580466097275345577614392125211904103287619944334946888019619381249645712041846833336380029103227064677215199253557215393240110273495490962687122824951547892188755604413351544536745353009397337945553392284857596498193142980679534911131578002050490377057440994367224678380166990971411278457279277737379636937556951151740153854937571421502073349335186749814700563871751488962215787897932330848135458194684387456953694536884289310248467290555168572748475039534984531898310552030079843821616446757340855352283258518028534374784914471292710726303424334591682870115081216405836662463042144366765926069277317949209777067192749240500343775636410647529119322706598793526161814151844031017139696014327742647683259975548959429723223861277522106970612263515927712396265650440595908369314227813360483645196142432274277466584863195891822802595907215880827960856309108664980148850608525342352194522770457074281126112955752786889621319018180210894848888074145335519148413351806680739723203938333421471979213417722796042324721831090277872645924273590785551219111162687940714213972157340273392604673293524735750936152907549223146410833651372255696698523570258369130727019706937378693219348468014072414398678811918167313408875435660784796244332891679042814406857713152288889371233649681426540481167935408418780809034058796540817760833135577732356957679125966485589256204869731141658094886165002871525415962236759417035454451268053462172455497642116474642648889122721142542830921928974776683151447931431694231434092849043944771678822613642498156337965476335050434933597318179896323814943571373791860498901980340635151307946528567771930218701497366550705833228376707740588279980807566052789511061270978992482933632550315133783094405392852661532326077115427246487715244198149669021884829746318747700246939651481307605741373139121918980588937962225749369580041843425464432661070952620795149619738814088719753599011629756437506803003698327486532974411673822845902096208293331797752133102437487011125391404578056628871911396762853741565552828675355811716840707345799388225821852376372065170439217030067391978673401265919610048923279016379805758307642231818002739404856243834815396556231828619571566205567034256977408084809351515176617426704373610509248178019897253902196871580571059743341840489451690891719517879112492282215184966711347689499002643460119867966126451976104389935524810724101414601132619369086162538717881891216935367524308980214596322294508425937340420193240760390636496106476601913022055747675081084809550744630540457050688164689871509267891247658660383191608389761461232616834153745099254008103133217125487810664594692283232303624481523664839901776392643470605631272625256840831077747393538430152614996524459975428373182428108810207939830787816143404304168561476279685633427448862881056969858333909059801338559704188012562464982010073014315159735351690813076319557725001197998087048061481155562213428601771988624385578691803770941411517008322547739516110164443587233502146802083572949277305366297474721816410214467216368415598380911479914941264506653...", unsigned int bufsize = 0x18)+0x9ec [d:/firefox/2.0/debug/js/src/../../../mozilla/js/src/jsdtoa.c @ 2395]
0012d7e0 10090df0 js3250!JS_dtostr(char * buffer = 0x0012d810 "", unsigned int bufferSize = 0x1a, int mode = 0, int precision = 0, double d = 1162146962369)+0xf2 [d:/firefox/2.0/debug/js/src/../../../mozilla/js/src/jsdtoa.c @ 2770]
0012d82c 38343134 js3250!js_NumberToString(struct JSContext * cx = 0x34333538, double d = 1.6913684579389284e-052)+0x70 [d:/firefox/2.0/debug/js/src/../../../mozilla/js/src/jsnum.c @ 713]
WARNING: Frame IP not in any known module. Following frames may be wrong.
0012d830 34333538 0x38343134


From another run:

(36c.4c0): Access violation - code c0000005 (first chance)
First chance exceptions are reported before any exception handling.
This exception may be expected and handled.
eax=0015b000 ebx=c0000000 ecx=0015b000 edx=00000034 esi=0012fbe8 edi=0012fbe0
eip=1003b3dc esp=0012d688 ebp=0012d77c iopl=0         nv up ei pl nz na po nc
cs=001b  ss=0023  ds=0023  es=0023  fs=0038  gs=0000             efl=00010202
*** WARNING: Unable to verify checksum for D:\firefox\2.0\debug\dist\bin\js3250.dll
js3250!js_dtoa+0x9ec:
1003b3dc 8810            mov     byte ptr [eax],dl          ds:0023:0015b000=??
0:000> kp
ChildEBP RetAddr  
0012d77c 1003a5f2 js3250!js_dtoa(double d = 719792111616, int mode = 0, int biasUp = 0, int ndigits = 0, int * decpt = 0x0012d7d8, int * sign = 0x0012d7c8, char ** rve = 0x0012d7d0, char * buf = 0x0012d812 "1162151321446944459203152163584541283066450357103038320995847080066642246498064692419547186811680664802415185110890582442033925496230732857652754278908376654263179733270267323968035699293749131029009575595883203509728006416760727229422137824961928792639066585623413548676837973707659847895334483679537008283750436506461167919679779229356077530663350137347728768207246328855973040241201316827967147765332050983207756461099762242421728224992428722227189850978148379273081701896559429985367823153134828000439877632951919717213424014251720896756667144934176551628580092699359704555014169931964895109354418683553576834251262669024401412458105450570488269421643945662968455758842416000378535932968906646503716520314954214420947953373148349388664126607951771448607763027805090364705212209239015559819531353313169800106482893367250106351821360803932736647101405163948528202343187254180069599230782140725585779127119919346332064614693852950519868260776135008355577884400459302504733534246334360996136696562732998057058788414355818270440863295244400524838511364833715178137862055303340972792777373796369375569511517401538549375714215020733493351867498147005638717514889622157878979323308481354581946843874569536945368842893102484672905551685727484750395349845318983105520300798438216164467573408553522832585180285343747849144712927107263034243345916828701150812164058366624630421443667659260692773179492097770671927492405003437756364106475291193227065987935261618141518440310171396960143277426476832599755489594297232238612775221069706122635159277123962656504405959083693142278133604836451961424322742774665848631958918228025959072158808279608563091086649801488506085253423521945227704570742811261129557527868896213190181802108948488880741453355191484133518066807397232039383334214719792134177227960423247218310902778726459242735907855512191111626879407142139721573402733926046732935247357509361529075492231464108336513722556966985235702583691307270197069373786932193484680140724143986788119181673134088754356607847962443328916790428144068577131522888893712336496814265404811679354084187808090340587965408177608331355777323569576791259664855892562048697311416580948861650028715254159622367594170354544512680534621724554976421164746426488891227211425428309219289747766831514479314316942314340928490439447716788226136424981563379654763350504349335973181798963238149435713737918604989019803406351513079465285677719302187014973665507058332283767077405882799808075660527895110612709789924829336325503151337830944053928526615323260771154272464877152441981496690218848297463187477002469396514813076057413731391219189805889379622257493695800418434254644326610709526207951496197388140887197535990116297564375068030036983274865329744116738228459020962082933317977521331024374870111253914045780566288719113967628537415655528286753558117168407073457993882258218523763720651704392170300673919786734012659196100489232790163798057583076422318180027394048562438348153965562318286195715662055670342569774080848093515151766174267043736105092481780198972539021968715805710597433418404894516908917195178791124922822151849667113476894990026434601198679661264519761043899355248107241014146011326193690861625387178818912169353675243089802145963222945084259373404201932407603906364961064766019130220557476750810848095507446305404570506881646898715092678912476586603831916083897614612326168341537450992540081031332171254878106645946922832323036244815236648399017763926434706056312726252568408310777473935384301526149965244599754283731824281088102079398307878161434043041685614762796856334274488628810569698583339090598013385597041880125624649820100730143151597353516908130763195577250011979980870480614811555622134286017719886243855786918037709414115170083225477395161101644435872335021468020835729492773053662974747218164102144672163684155983809114799149412645066538437492647224507986584171135733639890346931850076058462307197504532836786090186971588487808567442677331748469062648220654994299381819022479372355625987803141061785441852...6267196041768976586525188735817782853233006170345369472038510394900928933462785872502793668663272018445893838689884878609962415541626737220253389705362417743636490339308701817706516899742172678320031157124591802407792650076635179095211747865450068220353528704611112965779792965698004000841744387480546654134729172170940305269304203426628351817690788225087849173548769112691511637673185783552108827902660342192286682118600630613595207396275213457044396708013217824888004205314065284078640842979085701165492226221228565399002559574059719287506960290078066485484398598071581567206957445817816931933437829764406682600945605821064895780443081033429323192089424008484552738759982217900778672562405213152957356583992877722500343880493989401694787179395628766529524597338889271618938436181926889473469816574369049200235025203573166856753704917821493239082668683614394222363911046853060095986645512812393067493690557538354655137430041990226083471892758342140382177111175270856099393468699841105804660972753455776143921252119041032876199443349468880196193812496457120418468333363800291032270646772151992535572153932401102734954909626871228249515478921887556044133515445367453530093973379455533922848575964981931429806795349111315780020504903770574409943672246783801669909714112784572792777373796369375569511517401538549375714215020733493351867498147005638717514889622157878979323308481354581946843874569536945368842893102484672905551685727484750395349845318983105520300798438216164467573408553522832585180285343747849144712927107263034243345916828701150812164058366624630421443667659260692773179492097770671927492405003437756364106475291193227065987935261618141518440310171396960143277426476832599755489594297232238612775221069706122635159277123962656504405959083693142278133604836451961424322742774665848631958918228025959072158808279608563091086649801488506085253423521945227704570742811261129557527868896213190181802108948488880741453355191484133518066807397232039383334214719792134177227960423247218310902778726459242735907855512191111626879407142139721573402733926046732935247357509361529075492231464108336513722556966985235702583691307270197069373786932193484680140724143986788119181673134088754356607847962443328916790428144068577131522888893712336496814265404811679354084187808090340587965408177608331355777323569576791259664855892562048697311416580948861650028715254159622367594170354544512680534621724554976421164746426488891227211425428309219289747766831514479314316942314340928490439447716788226136424981563379654763350504349335973181798963238149435713737918604989019803406351513079465285677719302187014973665507058332283767077405882799808075660527895110612709789924829336325503151337830944053928526615323260771154272464877152441981496690218848297463187477002469396514813076057413731391219189805889379622257493695800418434254644326610709526207951496197388140887197535990116297564375068030036983274865329744116738228459020962082933317977521331024374870111253914045780566288719113967628537415655528286753558117168407073457993882258218523763720651704392170300673919786734012659196100489232790163798057583076422318180027394048562438348153965562318286195715662055670342569774080848093515151766174267043736105092481780198972539021968715805710597433418404894516908917195178791124922822151849667113476894990026434601198679661264519761043899355248107241014146011326193690861625387178818912169353675243089802145963222945084259373404201932407603906364961064766019130220557476750810848095507446305404570506881646898715092678912476586603831916083897614612326168341537450992540081031332171254878106645946922832323036244815236648399017763926434706056312726252568408310777473935384301526149965244599754283731824281088102079398307878161434043041685614762796856334274488628810569698583339090598013385597041880125624649820100730143151597353516908130763195577250011979980870480614811555622134286017719886243855786918037709414115170083225477395161101644435872335021468020835729492773053662974747218164102144672163684155...", unsigned int bufsize = 0x18)+0x9ec [d:/firefox/2.0/debug/js/src/../../../mozilla/js/src/jsdtoa.c @ 2395]
0012d7e0 10090df0 js3250!JS_dtostr(char * buffer = 0x0012d810 "", unsigned int bufferSize = 0x1a, int mode = 0, int precision = 0, double d = 1162151305724)+0xf2 [d:/firefox/2.0/debug/js/src/../../../mozilla/js/src/jsdtoa.c @ 2770]
0012d82c 31343534 js3250!js_NumberToString(struct JSContext * cx = 0x30333832, double d = 9.5063883087075125e-043)+0x70 [d:/firefox/2.0/debug/js/src/../../../mozilla/js/src/jsnum.c @ 713]
WARNING: Frame IP not in any known module. Following frames may be wrong.
0012d830 30333832 0x31343534


Here's what another developer got from a completely different build:

0:000> kp
ChildEBP RetAddr
0013eec0 100123f6 js3250!js_dtoa(double d = 1.2731934271933927e-313, int mode = 0, int biasUp = 0, int ndigits = 0, int * decpt = 0x0013eefc, int * sign = 0x00000035, char ** rve = 0x0013ef10, char * buf = 0x0013ef2e "1162144122972014314504375288231001817378836874275411375765123384366800004771020841168981362787052150928230916882702814797452292774343411518371471152149098464430563918937387606113617404159989369188922988560841891188099531356458897754597256051777635206358184644274181920335873304954207080915988791948054979986323075697490198799524016427515971752589967034519121412518398576837921278857730453792237986251248251105644753119025959596446812915363211515605851997888552820286747528804750101748572609538686005147459672127770429706446645143189346088794988925431743304825756487314163850224222249878412043708055685153703942645690965434392007771390202071081789651885009367453530093973379455533922848575964981931429806795349111315780020504903770574409943672246783801669909714112784572792777373796369375569511517401538549375714215020733493351867498147005638717514889622157878979323308481354581946843874569536945368842893102484672905551685727484750395349845318983105520300798438216164467573408553522832585!
1802853437478491447129271072630342433459168287011508121640583666246304214436676592606927731794920977706719274924050034377563641064752911932270659879352616181415184403101713969601432774264768325997554895942972322386127752210697061226351592771239626565044059590836931422781336048364519614243227427746658486319589182280259590721588082796085630910866498014885060852534235219452277045707428112611295575278688962131901818021089484888807414533551914841335180668073972320393833342147197921341772279604232472183109027787264592427359078555121911116268794071421397215734027339260467329352473575093615290754922314641083365137225569669852357025836913072701970693737869321934846801407241439867881191816731340887543566078479624433289167904281440685771315228888937123364968142654048116793540841878080903405879654081776083313557773235695767912596648558925620486973114165809488616500287152541596223675941703545445126805346217245549764211647464264888912272114254283092192897477668315144793143169423143409284!
9043944771678822613642498156337965476335050434933597318179896323814943
57137379186049890198034063515130794652856777193021870149736655070583322837670774058827998080756605278951106127097899248293363255031513378309440539285266153232607711542724648771524419814966902188482974631874770024693965148130760574137313912191898058893796222574936958004184342546443266107095262079514961973881408871975359901162975643750680300369832748653297441167382284590209620829333179775213310243748701112539140457805662887191139676285374156555282867535581171684070734579938822582185237637206517043921703006739197867340126591961004892327901637980575830764223181800273940485624383481539655623182861957156620556703425697740808480935151517661742670437361050924817801989725390219687158057105974334184048945169089171951787911249228221518496671134768949900264346011986796612645197610438993552481072410141460113261936908616253871788189121693536752430898021459632229450842593734042019324076039063649610647660191302205574767508108480955074463054045705068816468987150926789124765866038319160838976!
14612326168341537450992540081031332171254878106645946922832323036244815236648399017763926434706056312726252568408310777473935384301526149965244599754283731824281088102079398307878161434043041685614762796856334274488628810569698583339090598013385597041880125624649820100730143151597353516908130763195577250011979980870480614811555622134286017719886243855786918037709414115170083225477395161101644435872335021468020835729492773053662974747218164102144672163684155983809114799149412645066538437492647224507986584171135733639890346931850076058462307197504532836786090186971588487808567442677331748469062648220654994299381819022479372355625987803141061785441852626719604176897658652518873581778285323300617034536947203851039490092893346278587250279366866327201844589383868988487860996241554162673722025338970536241774363649033930870181770651689974217267832003115712459180240779265007663517909521174786545006822035352870461111296577979296569800400084174438748054665413472...91721709403052693042!
0342662835181769078822508784917354876911269151163767318578355210882790
266034219228668211860063061359520739627521345704439670801321782488800420531406528407864084297908570116549222622122856539900255957405971928750696029007806648548439859807158156720695744581781693193343782976440668260094Actx ", unsigned int bufsize = 0x18)+0x60a [h:\mozilla\tree-main\mozilla\js\src\jsdtoa.c @ 2409]
0013ef00 1002d1b8 js3250!JS_dtostr(char * buffer = 0x0013ef2c "", unsigned int bufferSize = 0x1a, JSDToStrMode mode = DTOSTR_STANDARD (0), int precision = 0, double d = 1162144120812)+0x86 [h:\mozilla\tree-main\mozilla\js\src\jsdtoa.c @ 2770]
0013ef4c 33373138 js3250!js_NumberToString(struct JSContext * cx = 0x33383837, double d = 3.3798416364912624e-057)+0x85 [h:\mozilla\tree-main\mozilla\js\src\jsnum.c @ 713]
WARNING: Frame IP not in any known module. Following frames may be wrong.
0013ef5c 37333131 0x33373138
0013ef60 35363735 0x37333131
Comment 9 timeless 2006-10-30 18:53:57 PST
dv

also go open the locals and watch windows and poke around.
Comment 10 Keith Victor 2006-11-03 13:56:47 PST
Hey:

I found and fixed the bug!

THanks for all your help, Timeless and Ben!

There is some bad looping logic in jsdtoa.
It uses logic based on the floating point math to exit the loop.
There is enough floating point error introduced, and we never exit the loop, while we continue to append the output buffer.

Comment 11 Ben Bucksch (:BenB) 2006-11-03 15:24:23 PST
Created attachment 244626 [details] [diff] [review]
Fix, v1, by Keith Victor

Keith Victor writes:

Proposed fix for a Firefox bug:
Crash in jsdtoa after opening a new window.

There is some very bad logic in jsdtoa, for the case where the double has no fractional component.
 
File: jsdtoa.c
See line 2376
 
    /* Do we have a "small" integer? */
 
In that case there is a loop that converts the double to a string, one digit at a time.
In the loop, it looks at the most significant digit, adds that digit to the string, and then removes the digit from the double value.
Then, it multiples the value by 10 so shift the digits over to the left.
 
The problem is that the looping logic is dependent on no floating point error being introduced.
But, floating point error is introduced when two doubles of different magnitudes are subtracted, which is done here:
 
2388:          d -= L*ds;
 
 
The check to exit the loop looks like this:
 
2410:
            if (!(d *= 10.))
                break;
It compares the double value to zero.
Unfortunately, there is floating point error introduced, and the value of "d" never gets down to zero.
Therefore, the loop becomes infinite, and we keep appending characters to the output string, ignoring the specified size of the buffer.   ( The buffer size test was done previously, based on the number of digits that it planned to place in the buffer. ).
 
My proposed fix is rather simple.
Since we know up front how many digits we have to write out, we can use that number to specify the number of times that we loop, rather than depending on errorless floating point math.
 
See the diff between the original and modified files attached.
 
I changed the "for" line:
 
From:
        for(i = 1; ; i++) {
To:
        for(i = 1; i<=k+1; i++) {
And, I removed the check on the bottom of the for loop.
 
 
From:
            if (!(d *= 10.))
                break;
To:
            d *= 10.;

Furthermore, the logic that is there is very bad. [Description about other problem cut, will file another bug where necessary.]

Thanks for your consideration.
 
Keith Victor
Media Machines, Inc.

----

This bug is important for them, as it prevents their Mozilla plugin from being used. I would like to check into trunk and later ask for branch checkin.

Brendan, please review. Thanks.
Comment 12 Chris Thomas (CTho) [formerly cst@andrew.cmu.edu cst@yecc.com] 2006-11-03 15:28:55 PST
BenB / brendan, why did this not crash SeaMonkey the same way?
Comment 13 Brendan Eich [:brendan] 2006-11-03 16:49:09 PST
(In reply to comment #12)
> BenB / brendan, why did this not crash SeaMonkey the same way?

Who knows?  You'd have to debug to find out.

/be
Comment 14 Brendan Eich [:brendan] 2006-11-03 16:59:15 PST
I'll have to study the code to review effectively.  A question for Keith or whomever can answer it: why i<=k+1 for the for loop's condition, instead of a comparison against ilim or ilim1?

Cc'ing wtc, as the prdtoa.c fork of David M. Gay's code looks newer and yet again different (but it may have the same bug -- again I'll have to make time to study it harder).

/be
Comment 15 Keith Victor 2006-11-03 20:04:19 PST
(In reply to comment #14)
> A question for Keith or
> whomever can answer it: why i<=k+1 for the for loop's condition, instead of a
> comparison against ilim or ilim1?

Hi:

In the cases I am seeing, both iLim and iLim1 are both -1 everytime it gets into that code.

I'm very new to this code, and I am not familiar with the parameters used here.
If there are beter parameters to use, that's fine with me.  But, iLim and iLim1 do not seem to be valid.

But,
Looking at the code for that case, and how it uses "k" to loop through the digits, and generate a string, one can conclude that "k+1" is the correct number of times to loop.

In each loop, it lops off most significant digit using the following algorithm:

        for(i = 1; i<=k+1; i++) {
            L = (Long) (d / ds);    // get the char in the "kth" position
            d -= L*ds;              // subtract off the most significant digit ( and introduce error )

            *s++ = '0' + (char)L;   // append the char to the string
            d *= 10.;               // SHift all the digits to the Left
where:
        ds = tens[k];

Since it is using this "ds" to determine which digit to lop off, it must write out "k+1" digits so that they are in the correct location.

For example, if k = 3, then ds = 1000.
So, the first time through the loop, we lop off the digit that is four places to the left of the decimal.  Since that first, and most significan digit, and the first digit that we place on the string, then we MUST write a total of four digits ( k+1 ) to the string.  Otherwise, that first digit will not be in the correct place.

Well, thats my logic anyway.

Thanks for all your help on this!

Keith Victor
Media Machines, Inc.
Comment 16 timeless 2006-11-04 15:26:35 PST
Comment on attachment 244626 [details] [diff] [review]
Fix, v1, by Keith Victor

grumble. please use cvs diff -pU30

in general, at least use cvs diff, -p, and enough -U to actually show what you're changing. in this case, the goal of 30 is to show me the entire block that you're influencing. had you used a cvs diff, bugzilla would have interpolated the missing context for me, but you didn't.
Comment 17 Ben Bucksch (:BenB) 2006-11-04 16:51:30 PST
You'll want to look at the actual source file for this change anyways. The function is 700 (!) lines long.
For your convenience, here's the link: http://lxr.mozilla.org/seamonkey/source/js/src/jsdtoa.c#2388
Comment 18 Keith Victor 2006-11-04 17:13:49 PST
Hey:

The crux of the change is in the for line, near 2386.

-        for(i = 1;; i++) {

+        for(i = 1; i<=k+1; i++) {

So, the relevent code is in that for loop.

The other change just removes the now redundant ( and still faulty ) test to Break out of the loop, when "d" gets down to zero.

I hope that helps.

Comment 19 Ben Bucksch (:BenB) 2006-11-04 17:36:00 PST
Created attachment 244726 [details] [diff] [review]
Fix, v1, by Keith Victor

same as cvs diff -pU30, as requested by timeless
Comment 20 timeless 2006-11-04 22:24:16 PST
that's much easier to read, and now if i were so inclined, i could read it as https://bugzilla.mozilla.org/attachment.cgi?id=244726&action=diff&headers=1&context=file 

(I'm not, the loop which you can clearly see in its entirety in the attachment is sufficient for my interest).
Comment 21 Keith Victor 2006-11-05 21:10:55 PST
Hi:

Here is some more information about this bug.
Previously, I did not understand the relationship between the plugins, and the crash.
All I could see was that if I loaded the plugin, it would then later crash in that jsdtoa code.

I did not understand why the same code could be called with the same arguments, and work fine before the simple plugin is loaded, but would crash after the plugin is loaded.

The only thing that the plugin does beyond the basic, "hello world" boiler plate code is it creates a Direct3D device.
As it turns out, when you create a Direct3D device, the precision of double precision math is reduced ( be default ).
( Isn't that nice! )
See:
http://msdn.microsoft.com/library/default.asp?url=/archive/en-us/directx9_c_Aug_2005/directx/graphics/reference/d3d/constants/D3DCREATE.asp

Look for : D3DCREATE_FPU_PRESERVE


That reduction of precision caues the unexpected roundoff error, and a resulting crash in jsdtoa.

So, we are able to fix our plugin, by creating our Direct3D device with that flag.
WIth that, it does not crash in the jsdtao code.


This some what explains why a codebase that is as mature as Mozilla could have such a huge bug.  It seems that it does not crash if your math is done in full double precision.
But, I still think this code is very dangerous, and the fix should be put in.

Other 3D plugin developers might fall into this trap  in the future, when they create a default Direct3D device.

Furthermore, there is another related bug in the same code that is also fixed by my change.

If you attempt to convert a 12 digit number, that has zeros directly to the left of the decimal, those zeros will be omitted from the string.
ie:   jsdtoa( 1000000000000.0 ) will result in this string: "1".
js( 123456789000.0 ) will result in "123456789"
This bug is independent of the precision issues discussed above.

I can not figure out how to create a test case to prove this.  But, I tested this in the debugger, and it is true.
In the loop, it lops off the most significant digit. As soon as the number goes to Zero, it breaks, and forgets to write out the trailing zeros.

The logic in that loop is BOTH flawed and reckless, independently!

Keith


Comment 22 Ben Bucksch (:BenB) 2006-11-06 06:08:56 PST
As to why this is Firefox only, I can only guess that Firefox uses *some* double (incl. conversion to string) in its JavaScript that's run during new window openings, and SeaMonkey doesn't.
Comment 23 Ben Bucksch (:BenB) 2006-11-06 06:11:42 PST
Changing summary to reflect causes we found.
Comment 24 timeless 2006-11-07 06:04:54 PST
switching precisions is a no no, it's very bad behavior and dangerous. oh, and switching is expensive.

changing this won't actually make the world happy since spidermonkey will now be violating JS specs for math operations.

it will temporarily result in no crash/deadlock, but it really isn't satisfactory.

we could theoretically add wasted calls to set precision before each jump in/out of plugins, but unfortunately they end up in the system event loop, so we'd also need calls each time we have a heartbeat.

that really sucks.
Comment 25 Ben Bucksch (:BenB) 2006-11-07 06:31:30 PST
timeless, I think you're misunderstanding it. It's *Microsoft* doing the precision switch, and preventing them from doing so is a workaround for this bug. Nothing in this patch switches precision, please read it. It fixes a logic flaw which is just exposed by the precision switch (and, as Keith mentioned, can happen in other cases, we just haven't found testcases yet).
Comment 26 timeless 2006-11-07 07:00:32 PST
i'm not. i understand ms is switching. but afaik all of js engine math (not just jsdtoa) needs to be in this one mode.
Comment 27 Brian Crowder 2006-11-07 08:09:43 PST
I think the code fix here is worth adding; this loop seems to be made much more safe by the slight restructuring of the loop.  It would be interesting to see if applying this fix causes regressions in the lin-testsuite.  If it doesn't, we should land it, and it might be worth nominating for 1.8.1.1
Comment 28 Brian Crowder 2006-11-07 10:22:31 PST
I ran against the testsuite, and it doesn't introduce any new failures.
Comment 29 Brian Crowder 2006-11-07 11:51:23 PST
Comment on attachment 244726 [details] [diff] [review]
Fix, v1, by Keith Victor

Brendan is busy, perhaps Igor can review?
Comment 30 Igor Bukanov 2006-11-07 12:52:37 PST
(In reply to comment #29)
> (From update of attachment 244726 [details] [diff] [review] [edit])
> Brendan is busy, perhaps Igor can review?

Can this wait until my afternoon tomorrow around 12:00 UTS? jsdtoa is the most hard to grasp piece of code IMO in SpiderMonkey and one must be fresh to look at it!
 
Comment 31 Igor Bukanov 2006-11-07 12:53:03 PST
(In reply to comment #29)
> (From update of attachment 244726 [details] [diff] [review] [edit])
> Brendan is busy, perhaps Igor can review?

Can this wait until my afternoon tomorrow around 12:00 UTC? jsdtoa is the most hard to grasp piece of code IMO in SpiderMonkey and one must be fresh to look at it!
 
Comment 32 Igor Bukanov 2006-11-08 07:45:37 PST
Comment on attachment 244726 [details] [diff] [review]
Fix, v1, by Keith Victor

Single nit:

>         }
>-        for(i = 1;; i++) {
>+        /* krv:  Use true number of digits to limit looping  */
>+        for(i = 1; i<=k+1; i++) {

Add blank line before "/* krv" comment and add "." after looping to follow SM style. r=+ with this change.
Comment 33 Wan-Teh Chang 2006-11-08 21:01:41 PST
It seems that if floating point errors are introduced in
             d -= L*ds;
the last digit that we write to the string could be off by one.
Is there a way to avoid floating point errors even when the
floating point arithmetic is single-precision?
Comment 34 Brian Crowder 2006-11-08 22:28:58 PST
The rounding error you mention does not exist in the unpatched version?
Comment 35 Keith Victor 2006-11-09 05:32:09 PST
(In reply to comment #34)
> The rounding error you mention does not exist in the unpatched version?
> 


Brian:
Did you follow the directions to reproduce the "bug".
Were you able to reproduce the crash?  The Crash is caused by the reduction in precision.  It is 100% reproducible.
You need to first load the plugin by playing the attached content.
After the plugin is loaded, then the precision is reduced
Comment 36 Keith Victor 2006-11-09 05:52:54 PST
(In reply to comment #33)
> It seems that if floating point errors are introduced in
>              d -= L*ds;
> the last digit that we write to the string could be off by one.
> Is there a way to avoid floating point errors even when the
> floating point arithmetic is single-precision?
> 
Firstly, it's not just the last digit that gets messed up.  The problem is the loop is controlled by the floating point arithmetic.  Since that is flawed ( or rather becomes flawed after the precision is reduced ), the loop becomes infinite, and it keeps adding characters to the string until it crashes.


But, to answer your questions:  Yes.
There is a #define in that code:
#define Int_max 14

Which is the number of digits that it figures are kept by a double.

Then, in order to get into the loop that we have been discussing, it needs to pass this test:

    if (be >= 0 && k <= Int_max) {


If we bump "Int_max" down to the number of digits of precision for a float, that would avoid the problem.
But, that would not be a good solution, IMHO.  Users should be able to get 14 digits of precision into their string, and we should not modify the way the string is generated because of odd ball cases where the precision is reduced.

It is my contention that Firefox should not Crash in those odd ball cases, as it does now.  And, we can do that without changing the number of digits ( which would break all kinds of test cases ) by simply modifying the logic in the loop.




Comment 37 Brian Crowder 2006-11-09 07:49:40 PST
Keith:  I'm not trying to be convinced of the veracity of the bug (I believe there is a bug), I'm trying to understand if there is a new rounding error introduced by the patch, which seems to be what wtchang@redhat is suggesting.  If the rounding-error is the same either way, that's fine.  If it is not, then I think in some important cases, it will be better for us to crash than to yield incorrect results, and we should wait for a patch which does not introduce this rounding-error.  wtchang:  Do you have a suggestion for an alternative fix that prevents the infinite-loop without introducing a rounding-error?

-- crowder
Comment 38 Ben Bucksch (:BenB) 2006-11-09 07:52:57 PST
crowder, no rounding error is introduced by the patch. The line that wtc blamed is not touched by the patch. This rounding error makes the unpatched version crash/loop infinitely, the patch makes us cope with the situation.
Comment 39 Ben Bucksch (:BenB) 2006-11-09 08:00:36 PST
Comment on attachment 244726 [details] [diff] [review]
Fix, v1, by Keith Victor

Igor, what does "add "." after looping" mean?

shaver: Can you superreview this?
Comment 40 Brian Crowder 2006-11-09 08:12:32 PST
ben:  Igor's asking for punctuation in the comment.
Comment 41 Blake Kaplan (:mrbkap) 2006-11-09 08:17:34 PST
It's worth noting that, in general, we don't usually add comments with initials because CVS blame + bugzilla take care of that...
Comment 42 Ben Bucksch (:BenB) 2006-11-09 08:27:27 PST
OK,
+        /* krv:  Use true number of digits to limit looping  */
will be replaced with:
+
+        /* Use true number of digits to limit looping. */
Comment 43 Brian Crowder 2006-11-09 08:35:31 PST
mrbkap:  shaver isn't inclined to sr+ this (just e-mailed me), would you mind adding your r+ (assuming the changes just mentioned)?
Comment 44 Keith Victor 2006-11-09 08:46:00 PST
(In reply to comment #37)
> Keith:  ...  I'm trying to understand if there is a new rounding error
> introduced by the patch, which seems to be what wtchang@redhat is suggesting. 
> 

Thanks you for the clarification of wtchang's comment.  I obviously missed his point.

YES.  There is indeed rounding error introduced by the patch.  Or, rather, the patch prevents a crash, which allows the string with the rounding error to be returned.  The Patch does not cause the rounding error.  The plugin does that.


For example:
Before Loading the Plugin:
jsdtoa( 123456789012.0 ) will result in the correct string of ( "123456789012" )

After the plugin is loaded, but withOUT the Patch:
jsdtoa( 123456789012.0 ) will crash, because it keeps adding characters to the string till it does crash.

After the plugin is loaded, but WITH the Patch:
jsdtoa( 123456789012.0 ) will result in the an incorrect string. Something like this: ( "123456747474" )  Its not just the last digit.  The last 5 or so digits are wrong.  
I dont know how to fix that bug when the double precision has mystericaly been reduced.  You would have to implement some kind of double precision processor in software, built on single precision math functions.  Not a good idea!

It is my belief that in the odd ball case where the precision has been reduced, we are better off returning a string that is incorrect, rather than crash.

I think I am hearing you say the opposite.  You would rather crash than provide the wrong string.  If that is the case, then let it crash.

Although there is still the independent issue where the function returns an incorrect value for numbers like 123456789000.0.
Comment 45 Keith Victor 2006-11-09 08:51:32 PST
(In reply to comment #41)
> It's worth noting that, in general, we don't usually add comments with initials
> because CVS blame + bugzilla take care of that...
> 

Sorry.  I wanted to get my initials in the codebase.  :)
Comment 46 Brian Crowder 2006-11-09 08:55:37 PST
I guess my concern is that, if a user is doing (for example) banking -- yes, it's a very very wealthy user -- in one window, and happens to load an unfriendly D3D plugin in another; his precision changes and he accidentally submits a form containing 123456747474, when he had meant to submit 123456789012.0.  In this situation, it seems to me we are better off crashing.  Perhaps an alternative solution would be to enforce floating-point precision at the start of JS execution?
Comment 47 Keith Victor 2006-11-09 08:59:06 PST
(In reply to comment #34)
> The rounding error you mention does not exist in the unpatched version?
> 

OH:
I think I also misunderstood this question.  I thought you were saying that you could not reproduce the rounding error. ( you were telling me that it does not exsit, not asking )

I'll directly answer your question:
Correct, The rounding error I mention does not exist in the unpatched version.
Because. it crashes before it returns the string.
Or, perhaps one might say, the rounding error does exist in the unpatched version, and causes a crash, which then hides the fact that it can not correctly convert the double to a string.

Comment 48 Igor Bukanov 2006-11-09 08:59:58 PST
(In reply to comment #37)
> If the rounding-error is the same either way, that's fine.  If it is not, then
> I think in some important cases, it will be better for us to crash than to
> yield incorrect results, and we should wait for a patch which does not
> introduce this rounding-error.  

The idea of the patch is that we already know the number of digits the loop should produce. So the patch just limits the loop to exactly the given number of iterations avoiding overwriting memory with 0 which can theoretically result in an exploitable bug. Any attempts to prevent printing of the wrong result due to rounding errors should go to another bug IMO.   
Comment 49 Brian Crowder 2006-11-09 09:03:34 PST
(In reply to comment #44)
> Although there is still the independent issue where the function returns an
> incorrect value for numbers like 123456789000.0.

Is there already another bug reported for this?  I cannot reproduce it in the shell:

js> String( 123456789000.0 )
123456789000
js> String( 123456789000000.0 )
123456789000000
js> String( 123456789000000000.0 )
123456789000000000
js> String( 123456789000000000000.0 )
123456789000000000000
js> String( 123456789000000000000000.0 )
1.23456789e+23
js> String( 123456789000000000000000000.0 )
1.23456789e+26

Can you open another bug and provide a testcase?

-- crowder
Comment 50 Igor Bukanov 2006-11-09 09:06:23 PST
(In reply to comment #47)
> Or, perhaps one might say, the rounding error does exist in the unpatched
> version, and causes a crash, which then hides the fact that it can not
> correctly convert the double to a string.

Exactly! Effectively the patch is a good conservative fix for theoretical security problem, so lets take take just on the base of that. The fact that it does not change jsdtoa to generate the right answer is for another bug. 
Comment 51 Brian Crowder 2006-11-09 09:07:37 PST
Fair enough; if Igor's sold I'm sold.  :)  We really don't -need- an sr+ or even another r+, though I certainly wouldn't mind another set of eyes.
Comment 52 Keith Victor 2006-11-09 09:17:04 PST
(In reply to comment #49)
> (In reply to comment #44)
> > Although there is still the independent issue where the function returns an
> > incorrect value for numbers like 123456789000.0.

I did the same thing, and it seems to work correctly, from the user's POV.
But, put a breakpoint in the jsdtoa, and look at what it returns.
It does indeed return an incorrect value, with the zeros stripped off the end.
But, then, it prints the correct result to the console.  So, it appears that is is calling jsdtoa, but not using the string produced by that function.

Therefore, I dont know how to produce a testcase.  But, I assure you that bug is real, and that loop contains two indepedent bugs!
Comment 53 Brian Crowder 2006-11-09 09:30:23 PST
Keith:  Ah.  js_dtoa() is not the full story.  JS_dtostr() does more work in padding the result from jsdtoa() using the result returned in js_dtoa()'s "rve" parameter.  If you're using js_dtoa() directly you need to perform the same padding.  That isn't a bug.
Comment 54 Keith Victor 2006-11-09 09:44:27 PST
(In reply to comment #53)
> Keith:  Ah.  js_dtoa() is not the full story.  JS_dtostr() does more work in
> padding the result from jsdtoa() using the result returned in js_dtoa()'s "rve"
> parameter.  If you're using js_dtoa() directly you need to perform the same
> padding.  That isn't a bug.
> 

OK, sorry about the diversion.
Still seems like a goofy way to go about it, but I guess it works.
Comment 55 Brian Crowder 2006-11-09 09:45:24 PST
Comment on attachment 244726 [details] [diff] [review]
Fix, v1, by Keith Victor

r=me (deputized for this bug by shaver)
Comment 56 Ben Bucksch (:BenB) 2006-11-09 10:20:34 PST
Created attachment 245115 [details] [diff] [review]
Fix, v2, by Keith Victor, comment changed

Thanks Brian.

Attaching patch with comment changed, ready for checkin. I assume I can't do it myself, because js/src/ is "restricted". Can a member of the "JavaScript" module please check it in? I propose the following checking comment:

When running with reduced FPU precision, the rounding error introduced by |d -= L*ds;| will cause |if (!(d *= 10.)) break;| to never be true, causing an infinite loop and consequent crash. Given |k| is already known, we know how often the loop should run, so use that as stop condition, also avoiding overwriting memory with 0.
Bug 358569. Patch by Keith Victor of MediaMachines, r=igor, r=crowder
Comment 57 :Gavin Sharp [email: gavin@gavinsharp.com] 2006-11-09 10:54:50 PST
(In reply to comment #56)
> Can a member of the "JavaScript" module please check it in?

Done: mozilla/js/src/jsdtoa.c 	3.36
Comment 58 Keith Victor 2006-11-09 11:09:31 PST
Thanks!

Comment 59 Brian Crowder 2006-11-09 11:15:07 PST
Comment on attachment 245115 [details] [diff] [review]
Fix, v2, by Keith Victor, comment changed

Nominating for branches
Comment 60 Wan-Teh Chang 2006-11-09 19:27:08 PST
Keith, sorry I didn't explain why I asked that question.
I just wanted to confirm my understanding that even though
the crash is fixed, the result is incorrect.  I hope we can
make the function fail in that case.

I confirmed that the version of this function in NSPR
(mozilla/nsprpub/pr/src/misc/prdtoa.c) has the same problem.
Comment 61 Igor Bukanov 2006-11-10 04:35:31 PST
(In reply to comment #60)
> Keith, sorry I didn't explain why I asked that question.
> I just wanted to confirm my understanding that even though
> the crash is fixed, the result is incorrect.  I hope we can
> make the function fail in that case.
> 

As timeless pointed out, this loss-of-precision problem should affect all double calculations in SpiderMonkey and other components of the browser. Thus the only speciality of jsdtoa is that it is possible to detect the problem due to the knowledge extracted about the number. Now, with the formating code fixed not to crash, why should jsdtoa worry about  the precision loss while the rest of the code doesn't?

For this reason I suggest to file a bug to implement a generic workaround for that DirectX problem and probably another one to alter jsdtoa not to depend on floating point  at all for its formatting code.
Comment 62 Igor Bukanov 2006-11-10 04:45:32 PST
I filed an enhancement bug 360247 about avoiding floating-point during number->string conversion.
Comment 63 Brendan Eich [:brendan] 2006-11-10 08:00:09 PST
Plugins must save and restore their host's FPU settings, or they are bad guests.  We are not going to FIX_FPU() before every FPU opcode that might be issued after returning from plugin code.

While it might be nice if {pr,js}dtoa.c didn't depend on FPU DP settings, that change forks even further from David M. Gay's long-lived dtoa.c code.  Who will own that fork?  Not I!  I'd rather we unfork existing non-essential changes and feed the essential ones upstream.

/be
Comment 64 Wan-Teh Chang 2006-11-10 10:35:11 PST
Created attachment 245234 [details]
NSPR Windows test program that reproduces the crash

You can use this as a basis for a regression test for
JavaScript.
Comment 65 Wan-Teh Chang 2006-11-10 10:36:36 PST
Created attachment 245235 [details]
(correct) NSPR Windows test program that reproduces the crash

I attached the wrong file.  Sorry.
Comment 66 Wan-Teh Chang 2006-11-10 10:39:15 PST
Comment on attachment 245115 [details] [diff] [review]
Fix, v2, by Keith Victor, comment changed

It seems that this change isn't necessary.  The original
code probably intends to break out of the loop as soon as
all the remaining digits are 0.

>-            if (!(d *= 10.))
>-                break;
>+            d *= 10.;
Comment 67 Brendan Eich [:brendan] 2006-11-10 10:44:50 PST
(In reply to comment #66)
> (From update of attachment 245115 [details] [diff] [review] [edit])
> It seems that this change isn't necessary.  The original
> code probably intends to break out of the loop as soon as
> all the remaining digits are 0.

With wrong precision mode in the FPU, you might not get to 0 before overrunning the buffer.

/be
Comment 68 Wan-Teh Chang 2006-11-10 10:49:59 PST
Brendan, I meant that the i<=k+1 change alone is sufficient
to fix the crash.  It's possible that the "if (!d) break" code
was intended to terminate the loop as soon as possible.
Comment 69 Wan-Teh Chang 2006-11-10 13:14:56 PST
I found this comment at the beginning of js_dtoa:

    /*  Arguments ndigits, decpt, sign are similar to those
        of ecvt and fcvt; trailing zeros are suppressed from
        the returned string.  ...

This comment suggests that the original "if (!d) break" code
was intended not only to terminate the loop but also to
suppress trailing zeros from the returned string.
Comment 70 Keith Victor 2006-11-10 13:31:10 PST
If you feel that it is problematic, then leave that check in there "if (!d) break". ( in addition to the i<k+1 check).  I suppose it will be a few nano-seconds faster that way.

I don't think its a problem.
Comment 71 Wan-Teh Chang 2006-11-10 13:50:19 PST
Keith, I don't know if not suppressing trailing zeros is a problem.
The only problem I see is that the code and comment got out of sync.

I don't understand the dtoa.c code but have to maintain it (the copy
in NSPR), so I'm taking extra time to understand the bug and your fix.
Sometimes I was just thinking out loud.  Please bear with me.  Your
clear explanation has been very helpful.
Comment 72 timeless 2006-11-12 17:56:22 PST
keep in mind that the behavior for math and stringification is almost certainly specified in ecma (and defacto before that), so we really can't just randomly change behavior. the only acceptable behavior in my book (other than working correctly) was crashing.

(and everyone knows that i don't like crashing.)
Comment 73 Daniel Veditz [:dveditz] 2006-11-28 11:27:13 PST
The result is a buffer overrun which is potentially a security issue -- broken math is much better than *that* type of crash. Rating "moderate" rather than "critical" because a random malicious page isn't likely to be able to force the use of a plugin that sets up the potential for this (in fact the actual risk is pretty minimal, but technically exists).
Comment 74 Daniel Veditz [:dveditz] 2006-11-28 11:30:38 PST
WTC: The "fix v2" patch only fixes the JS dtoa. Would you prefer a separate NSPR bug, or would you like to fix that under this bug at the same time?

If you're going to fix NSPR at a later time (lower risk, web content can't directly call into it the way they can JS Math) then a separate bug is better. Please mention it in this bug's dependencies.
Comment 75 Daniel Veditz [:dveditz] 2006-11-28 11:33:36 PST
Comment on attachment 245115 [details] [diff] [review]
Fix, v2, by Keith Victor, comment changed

approved for 1.8/1.8.0 branches, a=dveditz for drivers
Comment 76 Wan-Teh Chang 2006-11-28 13:23:01 PST
I opened a separate NSPR bug 362134.  I'm going to fix NSPR at
a later time.
Comment 77 :Gavin Sharp [email: gavin@gavinsharp.com] 2006-11-29 12:31:05 PST
mozilla/js/src/jsdtoa.c 	3.33.2.3
mozilla/js/src/jsdtoa.c 	3.33.10.1
Comment 78 Daniel Veditz [:dveditz] 2007-05-23 13:11:29 PDT
*** Bug 345308 has been marked as a duplicate of this bug. ***

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