436700 - Assertion failure: 1 <= num && num <= 0x10000, at jsregexp.cpp:1266

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Sylvain Pasche]

Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.1a1pre) Gecko/2008053112 Minefield/3.1a1pre

This is from http://trac.webkit.org/browser/trunk/LayoutTests/fast/js/resources/regexp-no-extensions.js. There's no crash with a non debug build.

Comment 1

[Brian Crowder]

I don't understand the apparent webkit behavior here, of interpreting the \214 as an octal escape.  It doesn't agree with my reading of the spec which seems to suggest that the number after the \ should reference a match index, and if the number of parens in the pattern is less than that index, we should "return failure" (ECMA-262 15.10.2.9)

Comment 2

[Brian Crowder]

It does seem to be a bug that we're not falling into the OVERFLOW_VALUE condition on line ~1237, however.

Comment 3

[Brian Crowder]

Ah, ok.  In FindParenCount, we recur into parsing the regexp again, to figure out the paren count.  In the recursion, we return OVERFLOW_VALUE as the max.

Comment 4

[Brian Crowder]

Attachment: FindParenCount should use 0x10000 as its max "fallout" value

mrbkap:  Please help me double-check my reading of the spec for this.  Is WebKit wrong?

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 326931 [details] [diff] [review]
FindParenCount should use 0x10000 as its max "fallout" value

See the comment after the strict warning (inside the (num == OVERFLOW_VALUE) block. Everybody deviates from the spec here to follow IE.

Comment 6

[Brian Crowder]

No, I get that part.  What I don't get is why WebKit is treating \214 as an octal sequence, instead of as a capture index.

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

That's what we do as well, isn't it?

js> /\214/.test("\214")
true

So any invalid backref is treated as octal, if possible.

Comment 8

[Brian Crowder]

Oh, then my fix probably isn't complete.  The testcase in the URL field here should succeed, should it not?  We should be interpreting \214 as a separate sequence of digits.

Comment 9

[Brian Crowder]

Comment on attachment 326931 [details] [diff] [review]
FindParenCount should use 0x10000 as its max "fallout" value

Canceling this review, since the patch seems to be inadequate.  Don't have time to work on this right now, though.

Comment 10

[Brian Crowder]

Note to self for later: octal values in regex shouldn't(?) exceed 255 (0377 occurs elsewhere in jsregexp.c, thanks to mrbkap for spotting).

Comment 11

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

/\2147483648/.exec(String.fromCharCode(140) + "7483648").toString()

On 1.9.0.x branch:
Assertion failure: 1 <= num && num <= 0x10000, at jsregexp.c:1266

With TM tip:
Assertion failure: 1 <= num && num <= 0x10000, at ../jsregexp.cpp:1269

Brian, still working on this?

Comment 12

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

This should be a regression of bug 230216, and also asserts in TM tip.

Seems to work as expected with cvs js shell checkout at 2004-05-13 17:09 PDT
Asserts with cvs js shell checkout at 2004-05-13 17:11 PDT

http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=mozilla%2Fjs%2Fsrc&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2004-05-13+17%3A09%3A00&maxdate=2004-05-13+17%3A11%3A00&cvsroot=%2Fcvsroot

Bonsai message:

My version of [e-mail]'s fix to check backref overflow (bug 230216, r/sr=igor/shaver).


=====

(gdb) bt
#0  JS_Assert (s=0x1a7f6e "1 <= num && num <= 0x10000", file=0x1a7f28 "../jsregexp.cpp", ln=1269) at ../jsutil.cpp:68
#1  0x000d8f84 in ParseTerm (state=0xbfffea5c) at ../jsregexp.cpp:1269
#2  0x000d978c in ParseRegExp (state=0xbfffea5c) at ../jsregexp.cpp:673
#3  0x000d9f41 in FindParenCount (state=0xbfffec80) at ../jsregexp.cpp:876
#4  0x000d7e08 in GetDecimalValue (c=50, max=0, findMax=0xd9e98 <FindParenCount>, state=0xbfffec80) at ../jsregexp.cpp:894
#5  0x000d8eae in ParseTerm (state=0xbfffec80) at ../jsregexp.cpp:1240
#6  0x000d978c in ParseRegExp (state=0xbfffec80) at ../jsregexp.cpp:673
#7  0x000daeb3 in CompileRegExpToAST (cx=0x30bc60, ts=0xbffff4b0, str=0x29e5c8, flags=0, state=@0xbfffec80) at ../jsregexp.cpp:1990
#8  0x000db0e2 in js_NewRegExp (cx=0x30bc60, ts=0xbffff4b0, str=0x29e5c8, flags=0, flat=0) at ../jsregexp.cpp:2530
#9  0x000e11fc in js_NewRegExpObject (cx=0x30bc60, ts=0xbffff4b0, chars=0x811a38, length=11, flags=0) at ../jsregexp.cpp:5041
#10 0x000d2784 in PrimaryExpr (cx=0x30bc60, ts=0xbffff4b0, tc=0xbffff434, tt=TOK_REGEXP, afterDot=0) at ../jsparse.cpp:5869
#11 0x000d419a in MemberExpr (cx=0x30bc60, ts=0xbffff4b0, tc=0xbffff434, allowCallSyntax=1) at ../jsparse.cpp:4513
#12 0x000c9d1d in UnaryExpr (cx=0x30bc60, ts=0xbffff4b0, tc=0xbffff434) at ../jsparse.cpp:4161
#13 0x000c9e4d in MulExpr (cx=0x30bc60, ts=0xbffff4b0, tc=0xbffff434) at ../jsparse.cpp:4000
#14 0x000c9f3d in AddExpr (cx=0x30bc60, ts=0xbffff4b0, tc=0xbffff434) at ../jsparse.cpp:3982
#15 0x000ca02c in ShiftExpr (cx=0x30bc60, ts=0xbffff4b0, tc=0xbffff434) at ../jsparse.cpp:3967
#16 0x000ca105 in RelExpr (cx=0x30bc60, ts=0xbffff4b0, tc=0xbffff434) at ../jsparse.cpp:3942
#17 0x000ca22a in EqExpr (cx=0x30bc60, ts=0xbffff4b0, tc=0xbffff434) at ../jsparse.cpp:3920
#18 0x000ca2e2 in BitAndExpr (cx=0x30bc60, ts=0xbffff4b0, tc=0xbffff434) at ../jsparse.cpp:3908
#19 0x000ca37c in BitXorExpr (cx=0x30bc60, ts=0xbffff4b0, tc=0xbffff434) at ../jsparse.cpp:3895
#20 0x000ca416 in BitOrExpr (cx=0x30bc60, ts=0xbffff4b0, tc=0xbffff434) at ../jsparse.cpp:3882
#21 0x000ca4b0 in AndExpr (cx=0x30bc60, ts=0xbffff4b0, tc=0xbffff434) at ../jsparse.cpp:3871
#22 0x000ca54a in OrExpr (cx=0x30bc60, ts=0xbffff4b0, tc=0xbffff434) at ../jsparse.cpp:3860
#23 0x000ca5e4 in CondExpr (cx=0x30bc60, ts=0xbffff4b0, tc=0xbffff434) at ../jsparse.cpp:3824
#24 0x000ca82a in AssignExpr (cx=0x30bc60, ts=0xbffff4b0, tc=0xbffff434) at ../jsparse.cpp:3750
#25 0x000cab1a in Expr (cx=0x30bc60, ts=0xbffff4b0, tc=0xbffff434) at ../jsparse.cpp:3704
#26 0x000cfea6 in Statement (cx=0x30bc60, ts=0xbffff4b0, tc=0xbffff434) at ../jsparse.cpp:3511
#27 0x000d026e in Statements (cx=0x30bc60, ts=0xbffff4b0, tc=0xbffff434) at ../jsparse.cpp:1496
#28 0x000d526f in js_ParseScript (cx=0x30bc60, chain=0x29c000, pc=0xbffff4b0) at ../jsparse.cpp:481
#29 0x00023431 in JS_BufferIsCompilableUnit (cx=0x30bc60, obj=0x29c000, bytes=0x30d8f0 "/\\2147483648/.exec(String.fromCharCode(140) + \"7483648\").toString()", length=67) at ../jsapi.cpp:4715
#30 0x000088e5 in Process (cx=0x30bc60, obj=0x29c000, filename=0x0, forceTTY=0) at ../../shell/js.cpp:412
#31 0x00009e56 in ProcessArgs (cx=0x30bc60, obj=0x29c000, argv=0xbffff7d0, argc=0) at ../../shell/js.cpp:782
#32 0x0000b0b7 in main (argc=0, argv=0xbffff7d0, envp=0xbffff7d4) at ../../shell/js.cpp:4634

Comment 13

[Brian Crowder]

Can't own this, sorry.

Comment 14

[Robert Sayre]

can't block if we're already living with it

Comment 15

[Igor Bukanov]

The assert indicates that REOP_BACKREF can have as a parameter parenIndex that exceeds the maximum possible number of reference groups. This number would be written as-is as a part of the bytecode in the REOP_BACKREF case of EmitREBytecode and later read during execution in SimpleMatch. Then the number is passed into BackrefMatcher where it will be used as as an index into REMatchState.parens array. 

So the violated assert means that the code may latter access the memory besides allocated data. Since BackrefMatcher only reads the context of REMatchState.parens[index] array, the worst consequence of this is that an evil script could use it to check if some arbitrary memory matches some script-supplied strings.

Comment 16

[Igor Bukanov]

Sorry, the previous comment is wrong: the assert is violated when GetDecimalValue calls the regexp parser recursively to find the number of parenthesis. But in this case the parsed tree is never converted into bytecode and simply is thrown away. So the assert is really harmless.

Comment 17

[Igor Bukanov]

Attachment: fix v1 [Checkin: Comment 18 & 20]

The fix relaxes the assert to ignore recursive invocations of regexp parser.

Comment 18

[Igor Bukanov]

landed to TM - http://hg.mozilla.org/tracemonkey/rev/a45eb680c66c

Comment 19

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

This has baked in TM for >2 weeks, setting checkin-needed for landing to the other branches.

Comment 20

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/a45eb680c66c

Comment 21

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/11591269864b

Comment 22

[Bob Clary [:bc] (inactive)]

ecma_3/RegExp/regress-436700.js	
http://hg.mozilla.org/tracemonkey/rev/86e0dc4f5352

Comment 23

[Bob Clary [:bc] (inactive)]

v 1.9.1, 1.9.2

Comment 24

[Bob Clary [:bc] (inactive)]

/cvsroot/mozilla/js/tests/ecma_3/RegExp/regress-436700.js,v  <--  regress-436700.js
initial revision: 1.1