319384 - Assertion failure and crash when converting a string to an integer

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Boris Zbarsky [:bzbarsky]]

Steps to reproduce are to load the page in the URL field in a debug build.

This is fallout from bug 215173.  Basically, we end up hitting an error converting some random string to an integer, which means we try to report the error listed in js.msg as:

117 MSG_DEF(JSMSG_CANT_CONVERT,            35, 1, JSEXN_NONE, "can't convert {0} to an integer")

Note the JSEXN_NONE there.  We end up in js_ErrorToException, which assigns JSEXN_NONE to |exn|, but thsi JSEXN_NONE has made a trip through the exnType member of JSErrorFormatString, which is a uint16.  So the value assigned to EXN is 65535 (-1 cast to uint16, then cast back to int).  Then we get:

#1  0xb7dfeb8f in js_ErrorToException (cx=0xb3fb6860, 
    message=0xb3efc7c0 "can't convert cfunc to an integer", reportp=0xbfffced4)
    at ../../../mozilla/js/src/jsexn.c:955
955         JS_ASSERT(exn < JSEXN_LIMIT);

And that assertion of course fails (since JSEXN_LIMIT <= 65535).

Then we go on and see:

968     if (exn == JSEXN_NONE)
969         return JS_FALSE;

which probably doesn't return since (int)65535 != (int)-1.

I assume that may be why we crash in opt builds on this site...

Comment 1

[Boris Zbarsky [:bzbarsky]]

Yeah, if I comment out the assert we get to:

992     ok = js_GetClassPrototype(cx, exceptions[exn].name, &errProto);

And looking at it in the debugger:

(gdb) p exceptions[exn].name
$4 = 0xc0840fc0 <Address 0xc0840fc0 out of bounds>

Comment 2

[Boris Zbarsky [:bzbarsky]]

For what it's worth, the page just does:

  clearTimeout('ViewClassic') 

(and any string that doesn't convert to integer would do).

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Fix both issues

There are two issues here:
1) The can't convert message should be some sort of exception (I'm using JSEXN_ERR here, though JSEXN_TYPEERR might be appropriate, comments welcome) so that it can be caught: try { somethingThatConvertsToInt32("foo"); } catch(e) { workAround(); }

2) Use the correct type for the exception struct.

Comment 4

[Michael Daumling]

Comment on attachment 205203 [details] [diff] [review]
Fix both issues

Could there be alignment issues on some systems, where an enum is stored as 32 bits (after the uint16 argcount)?

Comment 5

[Brendan Eich [:brendan]]

Comment on attachment 205203 [details] [diff] [review]
Fix both issues

> typedef struct JSErrorFormatString {
>     const char *format;  /* the error message (may be UTF-8 if compiled with JS_STRINGS_ARE_UTF8) */
>     uint16 argCount;     /* the number of arguments to convert in the error message */
>-    uint16 exnType;      /* One of the JSExnType constants above */
>+    JSExnType exnType;   /* One of the JSExnType constants above */
> } JSErrorFormatString;

I prefer Michael's version, for both packing/alignment and for better backward compatibility -- the JSErrorFormatString struct used to end with uintN argCount, and I'd rather not increase the size of the struct.  I don't remember why we didn't worry more about backward compatibility last time.  Michael, can you remind me?  Thanks,

sr=me with this hunk reverted.

/be

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #5)
> sr=me with this hunk reverted.

That hunk is needed for other, non-exceptionized, errors that might pass through js_ErrorToException. Perhaps it should be an int16 instead of uint16?

Comment 7

[Brendan Eich [:brendan]]

(In reply to comment #6)
> Perhaps it should be an int16 instead of uint16?

Sure, lots of widening to int, so signed narrower type is best.

/be

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into trunk (with the JSExnType -> int16 change discussed above).

Comment 9

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED on trunk using 2005-12-07-19 of SeaMonkey on Windows XP.

Comment 10

[Bob Clary [:bc] (inactive)]

Obvious string conversions didn't crash, so this test case is for the browser only and uses bz's clearTimeout example.

Checking in regress-319384.js;
/cvsroot/mozilla/js/tests/js1_5/Regress/regress-319384.js,v  <--  regress-31938
.js
initial revision: 1.1

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

*** Bug 319596 has been marked as a duplicate of this bug. ***

Comment 12

[Bob Clary [:bc] (inactive)]

I updated the testcase js1_5/Regress/regress-319384.js to attempt to catch the conversion error when clearTimeout('foo') is called. I think catchable JSEXN_TYPEERR would be a good change.

Comment 13

[Bob Clary [:bc] (inactive)]

fixed by Bug 336373 on the 1.8.1 branch. 
verified fixed 1.8.1 with windows/macppc/linux 20060707