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

VERIFIED FIXED

Status

()

Core
JavaScript Engine
VERIFIED FIXED
11 years ago
3 years ago

People

(Reporter: bc, Assigned: Brian Crowder)

Tracking

({regression})

Trunk
x86
Linux
regression
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

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

Comment 1

11 years ago
Note identical assertion from js1_5/Regress/regress-330352.js although I haven't checked the regression range.
(Assignee)

Comment 2

11 years ago
It seems likely that I have misunderstood some subtlety of the regexp code and these assertions are bogus.  I'll investigate.
(Reporter)

Comment 3

11 years ago
adding the two bugs|tests where this assert appears as dependencies to keep track of them.
Depends on: 330352, 346090
(Reporter)

Comment 4

11 years ago
js1_5/Regress/regress-330352.js for search goodness
(Reporter)

Updated

11 years ago
Group: security
(Reporter)

Updated

11 years ago
Duplicate of this bug: 381346
(Reporter)

Comment 6

11 years ago
Additional test from Seno.Aiko@gmail.com

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

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

Comment 7

11 years ago
Created attachment 268833 [details] [diff] [review]
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.
Assignee: general → crowder
Status: NEW → ASSIGNED
Attachment #268833 - Flags: review?(mrbkap)
Comment on attachment 268833 [details] [diff] [review]
removing the problematic assertion

How about wrapping these in

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

?
(Assignee)

Comment 9

11 years ago
Created attachment 268834 [details] [diff] [review]
annoy crowder and mrbkap only

Great idea.
Attachment #268833 - Attachment is obsolete: true
Attachment #268834 - Flags: review?(mrbkap)
Attachment #268833 - Flags: review?(mrbkap)
(Reporter)

Comment 10

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

Updated

11 years ago
Attachment #268834 - Flags: review?(mrbkap) → review+
(Assignee)

Comment 11

11 years ago
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.
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
(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
(Assignee)

Updated

11 years ago
Blocks: 375651
Created attachment 276854 [details] [diff] [review]
WIP debugging patch

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

Comment 15

11 years ago
although the assert has been removed and the tests "pass" now, this is still an issue according to comment 7.
Flags: in-testsuite+
Flags: blocking1.9?
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
(Assignee)

Comment 17

11 years ago
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
(Reporter)

Comment 18

11 years ago
js1_5/Regress/regress-330352.js, ecma_3/RegExp/regress-346090.js verified fixed 1.9.0 linux/mac*/windows.
Status: RESOLVED → VERIFIED
(Assignee)

Comment 19

11 years ago
The new bug is bug 398310.
(Reporter)

Updated

3 years ago
Flags: blocking1.9?
You need to log in before you can comment on or make changes to this bug.