Closed
Bug 230216
Opened 21 years ago
Closed 20 years ago
No overflow check for regexp back reference and quantifier bounds
Categories
(Core :: JavaScript Engine, defect, P1)
Tracking
()
VERIFIED
FIXED
mozilla1.7final
People
(Reporter: igor, Assigned: brendan)
References
Details
(Keywords: fixed1.7, js1.5)
Attachments
(5 files, 3 obsolete files)
13.52 KB,
patch
|
igor
:
review+
shaver
:
superreview+
asa
:
approval1.7+
|
Details | Diff | Splinter Review |
14.23 KB,
patch
|
Details | Diff | Splinter Review | |
2.57 KB,
text/plain
|
Details | |
2.45 KB,
text/plain
|
Details | |
2.67 KB,
text/plain
|
Details |
Currently SpiderMonkey does not check against numerical overflow in regexps in back reference and bounds for {} quantifier. For example, the following code: /(a)\21474836481/.test("aa") instead of expected error about too big number gives true in js shell since 21474836481 overflows as 1. Similarly /a{21474836481}/.test("a") also produces true. More dangerous case is /a\21474836480/.test("") where 21474836480 is treated as 0 which leads to call to backrefMatcher in jsregexp.c with length set to 4294967295 or (uint)-1 and thus accessing REMatchState->parens[-1]. In principle this can lead to a crash although I was not able to trigger it.
Reporter | ||
Comment 1•21 years ago
|
||
CC Brendan to look at it
Assignee | ||
Updated•20 years ago
|
Assignee: general → brendan
Flags: blocking1.7+
Priority: -- → P1
Target Milestone: --- → mozilla1.7final
Reporter | ||
Comment 2•20 years ago
|
||
The patch also makes sure that parenIndex declared in various structs always uses uin16 type instead of the current mixture of uin16/intN/jsint
Reporter | ||
Comment 3•20 years ago
|
||
Attachment #146000 -
Attachment is obsolete: true
Assignee | ||
Comment 4•20 years ago
|
||
Fixed GetDecimalValue to check for overflow on first character, simplified strict check for invalid/bad backref, added invalid backref strict check/warning for \0. /be
Assignee | ||
Updated•20 years ago
|
Attachment #148389 -
Flags: superreview?(shaver)
Attachment #148389 -
Flags: review?(igor)
Assignee | ||
Comment 5•20 years ago
|
||
Comment on attachment 148389 [details] [diff] [review] proposed fix based on igor's patch Trying to get this in for RC2's build tomorrow. /be
Attachment #148389 -
Flags: approval1.7?
Comment on attachment 148389 [details] [diff] [review] proposed fix based on igor's patch sr=shaver
Attachment #148389 -
Flags: superreview?(shaver) → superreview+
Comment 7•20 years ago
|
||
Comment on attachment 148389 [details] [diff] [review] proposed fix based on igor's patch a=asa (on behalf of drivers) for checkin to 1.7
Attachment #148389 -
Flags: approval1.7? → approval1.7+
Reporter | ||
Updated•20 years ago
|
Attachment #148389 -
Flags: review?(igor) → review+
Assignee | ||
Comment 8•20 years ago
|
||
Given pattern = /((\3|b)\2(a)x)+/; string = 'aaxabxbaxbbx'; pattern(string) should return ["ax", "ax", "", "a"], per ECMA-262 Edition 3 15.10.2.9, first production. Not perl-ish, but that's the way it was standardized. This patch fixes the testsuite case that the last patch regressed, but avoids the longstanding hack of JS_MAX(9, state->parenCount) used to dodge the bullet (with one parsing pass, state->parenCount will be 2 when \3 is parsed). To minimize code footprint for this hard case, I added a lazy reparse of the whole regexp to get a total count of parentheticals. /be
Assignee | ||
Comment 9•20 years ago
|
||
Comment on attachment 148442 [details] [diff] [review] better fix, handles forward "backrefs" Sorry to re-request so soon, but I should have run the testsuite. I wasn't prepared to turn over this rock and find all the creepy-crawlies, but the patch isn't too bad, although JSREG_TEMP_STATE is a hack. I suppose if there's an error later in the regexp, the nested, lazy reparse will report it, but the error will be ignored and we'll re-report it. I'd like to fix that later, on the 1.8 trunk. I'll leave this bug open. /be
Attachment #148442 -
Flags: superreview?(shaver)
Attachment #148442 -
Flags: review?(igor)
Attachment #148442 -
Flags: approval1.7?
Assignee | ||
Updated•20 years ago
|
Attachment #148389 -
Attachment is obsolete: true
Attachment #148389 -
Flags: superreview+
Attachment #148389 -
Flags: review+
Attachment #148389 -
Flags: approval1.7+
Assignee | ||
Updated•20 years ago
|
Attachment #146001 -
Attachment is obsolete: true
Reporter | ||
Updated•20 years ago
|
Attachment #148442 -
Flags: review?(igor) → review+
Comment 10•20 years ago
|
||
Comment on attachment 148442 [details] [diff] [review] better fix, handles forward "backrefs" a=asa (on behalf of drivers) for checkin to 1.7
Attachment #148442 -
Flags: approval1.7? → approval1.7+
Assignee | ||
Comment 11•20 years ago
|
||
I added the error propagation with the least additional code possible. /be
Assignee | ||
Comment 12•20 years ago
|
||
Fixed on trunk and branch. /be
Attachment #148442 -
Flags: superreview?(shaver) → superreview+
Comment 13•19 years ago
|
||
Igor's test 1
Comment 14•19 years ago
|
||
Igor's test 2. Igor, with your permission these two tests will be included in the javascript test library.
Comment 15•19 years ago
|
||
thanks to be.
Comment 16•19 years ago
|
||
js1_5/Regress/regress-230216-1.js, js1_5/Regress/regress-230216-2.js, js1_5/Regress/regress-230216-3.js checked in.
Updated•19 years ago
|
Flags: testcase+
You need to log in
before you can comment on or make changes to this bug.
Description
•