154737 - String(NaN) crashes JS Engine if JS_THREADSAFE is set

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[timeless]

w23k cvs trunk build from today (opt rofile)
jesse confirms it


js> NaN
<crash>

NTDLL! 77f821e1()
js_NumberToString(JSContext * 0x00af0838, double 1.#QNAN00000000) line 618 + 23 
bytes
js_ValueToString(JSContext * 0x00af0838, long 11004706) line 2577 + 15 bytes
Process(JSContext * 0x004016fb my_ErrorReporter(JSContext *, const char *, 
JSErrorReport *), JSObject * 0x00a7f8b8, char * 0x00000002, _iobuf * 
0x7803bb48) line 522
ProcessArgs(JSContext * 0x00af0838, JSObject * 0x00a7f8b8, char * * 0x00384ef4, 
int 0) line 655 + 28 bytes
main(int 1, char * * 0x00384ef0) line 936
XPCSHELL! mainCRTStartup + 227 bytes
KERNEL32! 77e8d326()

Comment 1

[timeless]

this might be a dupe, it seems familiar

PR_Lock(PRLock * 0x00000000) line 236
JS_dtostr(char * 0x0012ee50, unsigned int 25, int 0, int 0, double 
1.#QNAN00000000) line 2765
js_NumberToString(JSContext * 0x00af0bf8, double 1.#QNAN00000000) line 618 + 23 
bytes
js_ValueToString(JSContext * 0x00af0bf8, long 11005730) line 2577 + 15 bytes
Process(JSContext * 0x004016fb my_ErrorReporter(JSContext *, const char *, 
JSErrorReport *), JSObject * 0x00a7fcb8, char * 0x00000002, _iobuf * 
0x7803bb48) line 522
ProcessArgs(JSContext * 0x00af0bf8, JSObject * 0x00a7fcb8, char * * 0x00384ef4, 
int 0) line 655 + 28 bytes
main(int 1, char * * 0x00384ef0) line 936
XPCSHELL! mainCRTStartup + 227 bytes
KERNEL32! 77e8d326()

the function call comes from     ACQUIRE_DTOA_LOCK();

Comment 2

[timeless]

 Bug 120992   17 May 2002 23:21
i don't understand how this ever works.

anyway, here's what my research shows:
2764                   ACQUIRE_DTOA_LOCK();
2765                   dtoaRet = js_dtoa(d, dtoaModes[mode], mode >= 
DTOSTR_FIXED, precision, &decPt, &sign, &numEnd, numBegin, bufferSize-2);

js_dtoa calls
InitDtoa which does:
1176     freelist_lock = PR_NewLock();

nothing else assigns to the freelist_lock.
ACQUIRE_DTOA_LOCK tries to use the lock ...

Comment 3

[Steven Cole]

InitDtoA is also called by JS_strtod.  My guess is the following:  we crash
converting the double NaN to a string UNLESS we have earlier converted a string
to a double.  Normally, this is the case, because scripts are more likely to do
"for (i=0; i<10; i++)" than to emit NaN.  (Note the "0" and "10" cause InitDtoA
to be called in JS_strtod.)

I think all we need to do is call the init function in JS_dtostr (and remove it
from js_dtoa); I'll try that in the morning when I get back to a good
development machine.

--scole

Comment 4

[timeless]

Attachment: fix

I'm not sure about correct style for defines in mozilla/js, but imo this is the
correct thing to do.

Comment 5

[Steven Cole]

Let me add to my prior comment 4 to make things more clear:  we will crash (in
THREADSAFE) if a script contains no numbers execpt the "word" numbers (Infinity;
NaN), and tries to convert said numbers into strings.

If any numeric number exists in a script, then the DtoA code is initialized
properly during script compilation.

Note that the THREADSAFE requirement means that this will crash xpcshell but not
jsshell.

--scole

Comment 6

[Steven Cole]

Comment on attachment 89509 [details] [diff] [review]
fix

You've got the right idea; now use JS_BEGIN_MACRO and JS_END_MACRO for code
safety.  (See examples in jsinterp.h.)

--scole

Comment 7

[timeless]

Attachment: fix in js eng style

Comment 8

[Steven Cole]

Looks great, timeless.  I'd give it an r=, except that I figure I at least ought
to compile it, but I don't have all the baggage around to actually compile the
xpcshell; and this fix has no real impact on the jsshell.

Kenton?

--scole

Comment 9

[timeless]

while i'm rummaging through js1.5 bugs, i should cross reference bug 148171.

Perhaps we should always call the init function, just like we'll always call 
clenaup in !JS_THREADSAFE? -- well it's an interesting idea, but I can't figure 
out where I'd put that, so I'll leave that for someone else to consider in the 
future (I'll file a bug if someone requests).

Comment 10

[Phil Schwartau]

cc'ing dbradley: could you possibly test this patch in xpcshell
for possible r= on this patch? Thx -

cc'ing jband for possible sr=

Comment 11

[Kenton Hanson (gone)]

The Mac does not exhibit this problem.  The patch looks good but I'd rather
defer the r= to dbradley, I will r= if necessay.

Phil, can you write an automated test for this? 

Comment 12

[Phil Schwartau]

Kenton: we already have an automated testcase for this. The bug
does not manifest itself in the JS shell unless it has been built
with the JS_THREADSAFE flag set. That explains the references to 
the xpcshell above.

Perhaps that is why you are not seeing the crash -

Comment 13

[Steven Cole]

Actually, Phil, we don't have a testcase for this. 

Because the testcase *cannot* contain any numbers.  So the script 

a = "" + NaN; 

would kill xpcshell, whereas the script

a = "" + NaN;
b = 3;

would run just fine.

And since js/tests/ecma/shell.js has the number "0.0000001" on line 152, running
the NaN tests even in xpcshell won't crash.

[This is why this bug is never seen in mozilla-the-browser:  There's so much JS
that's run before a page is ever loaded that everything has been initialized
just fine.]

--scole

Comment 14

[Steven Cole]

Of course, then it occurs to me that jsshell and xpcshell compile by lines, not
by files, so the script 

a = "" + NaN;
b = 3;

would still crash in xpcshell.  But the opposite:

b = 3;
a = "" + NaN;

won't crash.  So my comment about line 152 in shell.js is still accurate.  (Sigh.)

--scole

Comment 15

[Brendan Eich [:brendan]]

Comment on attachment 89512 [details] [diff] [review]
fix in js eng style

I say this is good, sr=[e-mail].  Who's with me?  This should go
into the 1.0 branch when it's ready, too.

/be

Comment 16

[David Bradley]

Comment on attachment 89512 [details] [diff] [review]
fix in js eng style

r=dbradley

Looks fine to me, I compiled and ran as well and everything looks ok.

Comment 17

[timeless]

fixed on trunk

Comment 18

[chris hofmann]

Comment on attachment 89512 [details] [diff] [review]
fix in js eng style

a=chofman for 1.0.1.  add fixed1.0.1 after checking in.

Comment 19

[timeless]

committed fix to branch.
Note that the commit wasn't identical due to differences between branch and trunk.