Closed
Bug 375715
Opened 17 years ago
Closed 17 years ago
"Assertion failure: (c2 <= cs->length) && (c1 <= c2)" with /[\Wb-G]/
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
VERIFIED
FIXED
People
(Reporter: jruderman, Assigned: crowderbt)
References
Details
(Keywords: assertion, testcase, Whiteboard: [sg:nse?] patch broke GMail)
Attachments
(5 files, 8 obsolete files)
js> /[\Wb-G]/.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•17 years ago
|
||
Although we've found our max when we process a \W, we still need to complete processing the character set to find invalid ranges.
Comment 2•17 years ago
|
||
FWIW, I don't think this is exploitable. We allocate the maximum bitmap size thanks the the \W and then we add an invalid character range, which I believe will cause incorrect results (it looks like we invert the range) but shouldn't crash. I'm not entirely sure though.
Assignee | ||
Updated•17 years ago
|
Attachment #260024 -
Flags: review?(crowder) → review+
Comment 3•17 years ago
|
||
Fix checked into trunk.
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Comment 4•17 years ago
|
||
Comment on attachment 260024 [details] [diff] [review] Fix This is a pretty safe correctness fix.
Attachment #260024 -
Flags: approval1.8.1.4?
Attachment #260024 -
Flags: approval1.8.0.12?
Comment 5•17 years ago
|
||
sounds like something that should go on the branch after a tiny bit of baking on the trunk
Flags: blocking1.8.1.4?
Flags: blocking1.8.0.12?
Comment 6•17 years ago
|
||
Bug has "crash" keyword, but comment 2 says it shouldn't crash. Changing keyword.
Reporter | ||
Comment 7•17 years ago
|
||
I've been using the "crash" keyword for all JavaScript Engine assertions, because those assertions are fatal.
Comment 8•17 years ago
|
||
(In reply to comment #7) > I've been using the "crash" keyword for all JavaScript Engine assertions, > because those assertions are fatal. DEBUG build only -- a release build may or may not crash, so I don't think this keyword is appropriate for assertbotches. /be
Comment 9•17 years ago
|
||
I've backed out this patch because it broke Gmail (bug 375931).
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Updated•17 years ago
|
Whiteboard: [sg:nse?] → [sg:nse?] patch broke GMail
Comment 10•17 years ago
|
||
Sigh, reduced testcase is: /[\s-:]/ With this patch \s sets localMax to be 65535, so when we ensure that the end of the range is >= the beginning, we throw an error.
Comment 11•17 years ago
|
||
This artifically sets localMax to 0 to ensure that whatever character is next in the range is valid (which I think is correct, there is no maximum character for any of the ranges).
Attachment #260024 -
Attachment is obsolete: true
Attachment #260173 -
Flags: review?(crowder)
Attachment #260024 -
Flags: approval1.8.1.4?
Attachment #260024 -
Flags: approval1.8.0.12?
Assignee | ||
Updated•17 years ago
|
Attachment #260173 -
Flags: review?(crowder) → review+
Comment 12•17 years ago
|
||
Please ask for branch approval after this has been on the trunk a few days, (but we probably won't hold the release for this one so not a 'blocker')
Flags: blocking1.8.1.4?
Flags: blocking1.8.0.12?
Reporter | ||
Comment 13•17 years ago
|
||
Even with the patch, I get the same assertion with some other testcases: js> /[_-t]/i.exec("") Assertion failure: (c2 <= cs->length) && (c1 <= c2), at jsregexp.c:2237 js> (new RegExp("[\xDF-\xC7]]", "i")).exec("") Assertion failure: (c2 <= cs->length) && (c1 <= c2), at jsregexp.c:2237
Assignee | ||
Comment 14•17 years ago
|
||
The oddly-named "fold" flag screws us up here because we end up potentially adding an invalid range here. Fold implies two ranges, 1) the [downcase(start), downcase(end)], and 2) the [upcase(start), upcase(end)] range. For characters that cannot be up/down-cased (ie., the up/down-cased value is the same) this can yield a range where the start value is higher than the end value; thus an assertion. I don't think this can be made to yield a crash in release, but I am not convinced that it might not yield invalid results, at least.
Comment 15•17 years ago
|
||
Comment on attachment 261759 [details] [diff] [review] better handling for fold flag on some ranges of characters Add braces around the multilined ifs and r=mrbkap
Attachment #261759 -
Flags: review?(mrbkap) → review+
Assignee | ||
Comment 16•17 years ago
|
||
Added the parens requested by mrbkap and landed: jsregexp.c: 3.149
Status: ASSIGNED → RESOLVED
Closed: 17 years ago → 17 years ago
Resolution: --- → FIXED
Comment 17•17 years ago
|
||
In Linux shell from today, with this test still get Assertion failure: (c2 <= cs->length) && (c1 <= c2), at jsregexp.c:2231 Trace/breakpoint trap
Comment 18•17 years ago
|
||
Comment 19•17 years ago
|
||
Comment 20•17 years ago
|
||
Updated•17 years ago
|
Status: RESOLVED → REOPENED
Flags: in-testsuite+
Resolution: FIXED → ---
Assignee | ||
Comment 21•17 years ago
|
||
Assignee | ||
Comment 22•17 years ago
|
||
Instead of asserting, just make this a runtime check in the most central location.
Attachment #263502 -
Flags: review?(mrbkap)
Assignee | ||
Comment 23•17 years ago
|
||
mrbkap: Of the last two patches, pick and r+ your fave. My fave is the one I asked for review on.
Assignee | ||
Comment 24•17 years ago
|
||
this one.
Attachment #263502 -
Attachment is obsolete: true
Attachment #263502 -
Flags: review?(mrbkap)
Assignee | ||
Updated•17 years ago
|
Attachment #263503 -
Flags: review?(mrbkap)
Assignee | ||
Comment 25•17 years ago
|
||
Comment on attachment 263503 [details] [diff] [review] whoops, alligator is eating the wrong c Ok, no. This whole thing is bogus. We must report an invalid character range like this and bail. A bug my patch introduces is that the first character of the range will still be added even if the range is invalid. A patch that does the right reporting coming up. Sorry for the bugspam.
Attachment #263503 -
Attachment is obsolete: true
Attachment #263503 -
Flags: review?(mrbkap)
Assignee | ||
Comment 26•17 years ago
|
||
Attachment #263501 -
Attachment is obsolete: true
Attachment #263686 -
Flags: review?(mrbkap)
Comment 27•17 years ago
|
||
Why can't we catch these cases in CalculateBitmapSize?
Comment 28•17 years ago
|
||
Comment on attachment 263686 [details] [diff] [review] emit error messages for bad ranges Brian and I talked about this and he's going to come up with a comprehensive patch.
Attachment #263686 -
Flags: review?(mrbkap)
Assignee | ||
Comment 29•17 years ago
|
||
This includes Blake's old patch and adds another range check that should catch the remaining cases in this bug.
Attachment #260173 -
Attachment is obsolete: true
Attachment #263686 -
Attachment is obsolete: true
Attachment #265703 -
Flags: review?(mrbkap)
Comment 30•17 years ago
|
||
Comment on attachment 265703 [details] [diff] [review] As suggested by mrbkap This is missing Perl compatibility that we discussed (or, at least, doesn't bring sanity to the /[\s-:]/ regexp).
Attachment #265703 -
Flags: review?(mrbkap) → review-
Comment 31•17 years ago
|
||
Comment on attachment 265703 [details] [diff] [review] As suggested by mrbkap Further investigation shows that this actually does match Perl's behavior.
Attachment #265703 -
Flags: review- → review+
Assignee | ||
Comment 32•17 years ago
|
||
Last patch introduced an off-by-one that caused all sorts of problems (found in extra testing before landing the patch). This fixes that.
Attachment #265836 -
Flags: review?(mrbkap)
Comment 33•17 years ago
|
||
Comment on attachment 265836 [details] [diff] [review] fixing an off-by-one >- if (rangeStart > localMax) { >+ if (rangeStart > localMax || rangeStart > localMax) { This change seems to be unnecessary.
Assignee | ||
Comment 34•17 years ago
|
||
Wow, not enough coffee today, huh? Odd; I wonder how your original patch never landed, mrbkap, since that seems to be all that's needed here.
Assignee | ||
Updated•17 years ago
|
Attachment #265836 -
Attachment is obsolete: true
Attachment #265836 -
Flags: review?(mrbkap)
Assignee | ||
Updated•17 years ago
|
Attachment #265703 -
Attachment is obsolete: true
Assignee | ||
Comment 35•17 years ago
|
||
jsregexp.c: 3.150
Status: REOPENED → RESOLVED
Closed: 17 years ago → 17 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 36•17 years ago
|
||
Comment on attachment 260173 [details] [diff] [review] Fixed fix Landed this one. jsregexp.c: 3.150
Attachment #260173 -
Attachment is obsolete: false
Comment 37•17 years ago
|
||
Consensus was that this was not an exploitable problem? Please remove the security flag if that is true.
Flags: blocking1.8.1.5?
Flags: blocking1.8.0.13?
Comment 38•17 years ago
|
||
I accidentally checked this test in rather than make it an attachment. Hopefully dveditz is correct and this is not a security issue. The test must be negative since the error can not be caught and still exhibit the assertion. /cvsroot/mozilla/js/tests/ecma_3/RegExp/regress-375715-01-n.js,v <-- regress-375715-01-n.js initial revision: 1.1
Updated•17 years ago
|
Attachment #263489 -
Attachment is obsolete: true
Updated•17 years ago
|
Group: security
Comment 39•17 years ago
|
||
Given comment 37 ("not exploitable") do we need this fix for the branches? I'm guessing not.
Flags: wanted1.8.1.x+
Flags: wanted1.8.0.x+
Flags: blocking1.8.1.5?
Flags: blocking1.8.1.5-
Flags: blocking1.8.0.13?
Flags: blocking1.8.0.13-
Comment 40•17 years ago
|
||
/cvsroot/mozilla/js/tests/ecma_3/RegExp/regress-375715-02.js,v <-- regress-375715-02.js initial revision: 1.1 /cvsroot/mozilla/js/tests/ecma_3/RegExp/regress-375715-03.js,v <-- regress-375715-03.js initial revision: 1.1 /cvsroot/mozilla/js/tests/ecma_3/RegExp/regress-375715-04.js,v <-- regress-375715-04.js initial revision: 1.1
You need to log in
before you can comment on or make changes to this bug.
Description
•