379056 - Assertion failure: x->cp >= (gData->cpbegin + cap->index) (ecma_3/RegExp/regress-346090.js)

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Bob Clary [:bc] (inactive)]

bug 375642 regressed ecma_3/RegExp/regress-346090.js

Assertion failure: x->cp >= (gData->cpbegin + cap->index), at jsregexp.c:2872

marking sensitive out of paranoia.

Comment 1

[Bob Clary [:bc] (inactive)]

Note identical assertion from js1_5/Regress/regress-330352.js although I haven't checked the regression range.

Comment 2

[Brian Crowder]

It seems likely that I have misunderstood some subtlety of the regexp code and these assertions are bogus.  I'll investigate.

Comment 3

[Bob Clary [:bc] (inactive)]

adding the two bugs|tests where this assert appears as dependencies to keep track of them.

Comment 4

[Bob Clary [:bc] (inactive)]

js1_5/Regress/regress-330352.js for search goodness

Comment 6

[Bob Clary [:bc] (inactive)]

Additional test from [e-mail]

Reduced testcase to trigger this assertion:
  "23".match(/(\d+){5,}/);

crowder, can we kill this assert? Its a pita.

Comment 7

[Brian Crowder]

Attachment: removing the problematic assertion

Please leave the bug open, however.  These assertions have merit: there is something wrong in jsregexp.c that they are allowed to happen.

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 268833 [details] [diff] [review]
removing the problematic assertion

How about wrapping these in

#if defined(DEBUG_crowder) || defined(DEBUG_mrbkap)

?

Comment 9

[Brian Crowder]

Attachment: annoy crowder and mrbkap only

Great idea.

Comment 10

[Bob Clary [:bc] (inactive)]

comment #2 implied these asserts were bogus. If that is not the case, then please leave them in.

Comment 11

[Brian Crowder]

bc:  I'm less inclined to believe comment #2 the more I investigate, but the issue is more systemic, I think.  Also, I don't believe this is exploitable.

Comment 12

[Brendan Eich [:brendan]]

js1_5/Regress/regress-330352.js has a testcase,

"AB".match(/(.*)*?B/)

which seems to botch the captured paren state when backtracking. A length of -1 stored as a size_t is just wrong and leads to an assertbotch in JS_malloc. But if one clamps length at 0, the result is wrong too: ["AB", ""]. IOW, nothing is captured but the whole target string is matched. This should provide a clue.

I think I saw a failure to backtrack to the LPAREN, but I might have been confused. If backtracking within an open paren, the left end (cap->index) does not need to be reset unless one backtracks x->cp behind it. Hmm, maybe that is a further fix beyond clamping cap->length at 0.

/be

Comment 13

[Brendan Eich [:brendan]]

(In reply to comment #12)
> If backtracking within an open paren, the left end (cap->index) does
> not need to be reset unless one backtracks x->cp behind it. Hmm, maybe that is
> a further fix beyond clamping cap->length at 0.

No, that's not it. It looks like when backtracking we do not "backtrack" the captured paren indexes and lengths. More in a bit.

/be

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: WIP debugging patch

I'm sticking this here for safe-keeping. This helps debug backtracking.

Comment 15

[Bob Clary [:bc] (inactive)]

although the assert has been removed and the tests "pass" now, this is still an issue according to comment 7.

Comment 16

[Brendan Eich [:brendan]]

Blake, Brian: what's the latest thinking on a fix, and is this the bug for it? If there is another bug, then this could be marked fixed based on its summary.

/be

Comment 17

[Brian Crowder]

Yeah, this is fixed by the wallpaper I provided in another bug.  A new bug should be opened regarding bogus state-management in the regexp machine, though.

Comment 18

[Bob Clary [:bc] (inactive)]

js1_5/Regress/regress-330352.js, ecma_3/RegExp/regress-346090.js verified fixed 1.9.0 linux/mac*/windows.

Comment 19

[Brian Crowder]

The new bug is bug 398310.