JSVAL_INT overflow in regexp matching on 64-bit systems

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: luke, Unassigned)

Tracking

unspecified
x86
Linux
Points:
---
Dependency tree / graph
Bug Flags:
wanted1.9.2 +

Firefox Tracking Flags

(status1.9.2 beta1-fixed)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

8 years ago
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.
Created attachment 398062 [details] [diff] [review]
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.
Attachment #398062 - Flags: review?(brendan)
(Reporter)

Comment 2

8 years ago
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 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
(Reporter)

Updated

8 years ago
Blocks: 511777
Blocks: 457879
Created attachment 401066 [details] [diff] [review]
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.
Attachment #398062 - Attachment is obsolete: true
Attachment #401066 - Flags: review?(brendan)
Attachment #398062 - Flags: review?(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
Attachment #401066 - Flags: review?(brendan) → review+
Blocks: 518013
pushed with change from comment #5

http://hg.mozilla.org/tracemonkey/rev/126a52305332
Whiteboard: fixed-in-tracemonkey
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
(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

8 years ago
http://hg.mozilla.org/mozilla-central/rev/126a52305332
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED

Comment 10

8 years ago
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/9673dbfbc913
status1.9.2: --- → beta1-fixed
Flags: wanted1.9.2+
You need to log in before you can comment on or make changes to this bug.