513348 - JSVAL_INT overflow in regexp matching on 64-bit systems

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Luke Wagner [:luke]]

While jsval is 64 bits on 64-bit systems, JSVAL_INTs are still 31-bit integers.  
Since, on 64-bit systems, Strings can be > 32 bits, assignment of match indices to jsvals can overflow.  Two instances I've found are in str_search and js_ExecuteRegExp in jsregexp.cpp, but I suspect there are others.

Comment 1

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: proposed patch

This is a problem for the x64 JIT too because we want indexes to be 32-bit integers. As a compromise to solve this and that situation, this patch limits string lengths to 30 bits, since an unsigned 30-bit integer can still fit in a 31-bit jsval.

I would have liked to make this based on JSVAL_INT_BITS but I couldn't use that from jsstr.h so I made a static assert instead.

Comment 2

[Luke Wagner [:luke]]

I just talked to David about this, and there is an argument for changing the type returned by JSString::length() (and taken by getCharsAndLength) to reflect the fact that the length can necessarily fit in a JSVAL_INT.  Perhaps jsuint?  This would probably require a rather large patch, so maybe something to do later.

Comment 3

[Brendan Eich [:brendan]]

Comment on attachment 398062 [details] [diff] [review]
proposed patch

>+JS_STATIC_ASSERT(size_t(JSString::MAX_LENGTH) <= UINT_MAX &&
>+                 INT_FITS_IN_JSVAL(JSString::MAX_LENGTH));

Two separate static assertions in a row beats one using && so you can tell what botched.

I don't see the connection with UINT_MAX, which depends on the target C++ type model. Are we using unsigned/uintN/uint anywhere relevant? If you mean jsuint, then

$ grep jsuint *.h | grep typedef
jspubtd.h:typedef uint32    jsuint;

suggests using JSUINT_MAX and defining that appropriately in jspubtd.h. ILP64 targets will have an overlarge UINT_MAX but jsuint will still be uint32.

r=me once I understand the <limits.h>/UINT_MAX dependency.

/be

Comment 4

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: v2

Removed limits.h dependency. Also lowered bits to 29 from 30, so waiting for OOM in some tests doesn't take forever and slow your system to a crawl.

Alternately I could raise it back up to 30, and shortcut in ConcatStrings etc that the resulting string will fit in a JSString.

Comment 5

[Brendan Eich [:brendan]]

Comment on attachment 401066 [details] [diff] [review]
v2

>+#if JS_BITS_PER_WORD > 32
>+        LENGTH_BITS =   29,
>+#else
>         LENGTH_BITS =   JS_BITS_PER_WORD - 4,
>+#endif


In the interest of inter-operation, why not use 28 for both 32- and 64-bit targets?

r=me with that.

/be

Comment 6

[David Anderson [:dvander] - inactive, e-mail if emergency]

pushed with change from comment #5

http://hg.mozilla.org/tracemonkey/rev/126a52305332

Comment 7

[Brendan Eich [:brendan]]

Might be better to avoid the ifdef and always use 28, elsewhere static-asserting that JS_BITS_PER_WORD - 28 >= 4 (the number of bits reserved for flags). But the static assert should live near the reserved flags' definitions, hrm. Maybe this is the way to go.

/be

Comment 8

[Brendan Eich [:brendan]]

(In reply to comment #7)
> Maybe this is the way to go.

In case this was confusing, I mean let's go with what landed ;-).

/be

Comment 9

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/126a52305332

Comment 10

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/9673dbfbc913