375876 - "Assertion failure: (c2 <= cs->length) && (c1 <= c2)" with /[\[-h]/i

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Jesse Ruderman]

js> /[\[-h]/i.exec("")
Assertion failure: (c2 <= cs->length) && (c1 <= c2), at jsregexp.c:2233

Security-sensitive for now because the code involved is playing with bit arrays.

Comment 1

[Jesse Ruderman]

WFM with this testcase.  New testcase coming soon.

Comment 2

[Jesse Ruderman]

js> /[\d-z-x]/.exec("")
Assertion failure: (c2 <= cs->length) && (c1 <= c2), at jsregexp.c:2242

I can make that a new bug and mark this one as WFM if desired.

Comment 3

[Jesse Ruderman]

I ran the regular expression fuzzer for over 10 hours and didn't hit anything other than this bug :)

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

Brian, the problem here is that CalculateBitmapSize and ProcessCharSet don't agree as to whether \d can start a character range (ain't code duplication fun?). I think the fix is to make CalculateBitmapSize follow suit, but I don't have time to try it out right now.

Comment 5

[Brian Crowder]

The equivalent for us, to what perl does, would be to accept the \d as 0-9, then read the - as a lone '-' character in the set, not the start of the range.  Finally, we would throw an exception at the z-x (invalid range).

crowdermac:src crowder$ perl -e 'use re 'debug'; s/[\d-z-c]/a/'
Compiling REx `[\d-z-c]'
Invalid [] range "z-c" in regex; marked by <-- HERE in m/[\d-z-c <-- HERE ]/ at -e line 1.

crowdermac:src crowder$ perl -e 'use re 'debug'; "" =~ s/[\d-z]/a/'
Compiling REx `[\d-z]'
size 13 Got 108 bytes for offset annotations.
first at 1
   1: ANYOF[\-0-9z+utf8::IsDigit](13)
  13: END(0)
stclass "ANYOF[\-0-9z+utf8::IsDigit]" minlen 1 
Offsets: [13]
	1[6] 0[0] 0[0] 0[0] 0[0] 0[0] 0[0] 0[0] 0[0] 0[0] 0[0] 0[0] 7[0] 

Comment 6

[Mike Schroepfer]

Marking blocking on the concern that this is a latent security issue.  Please adjust if wrong...

Comment 7

[Brian Crowder]

Attachment: Treat "-" after character-set (\w, \W, \d, \D) as '-', not part of a range

This gives us parity with what perl does.

Comment 8

[Brendan Eich [:brendan]]

I forget, too busy to look (and my eyes would bleed anyway): what does ES3 spec say to do with such hyphens?

/be

Comment 9

[Brian Crowder]

The spec seems to think [\d-z] should specify a range, and not the group \d, '-', 'z':

ClassAtom::
    -
    ClassAtomNoDash

ClassAtomNoDash::
    SourceCharacter  (but not one of \ ] -)
    \ ClassEscape

ClassEscape::
    DecimalEscape
    b
    CharacterEscape
    CharacterClassEscape

This, however, seems like a bug in the spec.  We should not treat 
'\' CharacterClassEscape '-' ClassAtom
as a range.  I haven't worked out what the proper grammar should look like, though.  This is definitely -not- what perl does.  Also, our grammar seems to allow the nonsensical /[a-g-z]/ to become [/a-z/], while perl interprets it as /[-a-gz]/

Comment 10

[Brian Crowder]

Webkit seems to adhere more closely to the perl behavior here.

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 291941 [details] [diff] [review]
Treat "-" after character-set (\w, \W, \d, \D) as '-', not part of a range

>+        JSBool canStartRange = JS_TRUE;
>         uintN localMax = 0;
...
>           default:
>             localMax = *src++;
>+            canStartRange = JS_TRUE;
>             break;
>         }

To make sure I'm following correctly: this assignment isn't actually necessary, right?

Comment 12

[Brian Crowder]

Attachment: removing redundant assignment

Yes, mrbkap, that assignment in the default: case is redundant given the assignment at the top of the loop.  Thanks.

Comment 13

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 292534 [details] [diff] [review]
removing redundant assignment

While I'm not sure I disagree with this patch entirely, I'm not sure it's the right one for this bug. ;-)

Comment 14

[Brian Crowder]

Comment on attachment 292534 [details] [diff] [review]
removing redundant assignment

Woops, here comes the real patch shortly.

Comment 15

[Brian Crowder]

Attachment: one more try

As much as attachment 292534 [details] [diff] [review] delights me (and Blake), -this- is the right patch.  For now.

Comment 16

[Brian Crowder]

jsregexp.c: 3.169

Comment 17

[Bob Clary [:bc] (inactive)]

Attachment: ecma_3/RegExp/regress-375876-01.js

Comment 18

[Bob Clary [:bc] (inactive)]

Attachment: ecma_3/RegExp/regress-375876-02.js

Comment 20

[Bob Clary [:bc] (inactive)]

verified fixed 1.9.0

Comment 21

[Bob Clary [:bc] (inactive)]

when this bug is opened, the test should be checked in.