1.14 KB, text/html
1.25 KB, patch
|Details | Diff | Splinter Review|
59 bytes, text/plain
1.39 KB, patch
Mike Schroepfer: approval1.8.1+
|Details | Diff | Splinter Review|
User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:18.104.22.168) Gecko/20060608 Ubuntu/dapper-security Firefox/22.214.171.124 Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:126.96.36.199) Gecko/20060608 Ubuntu/dapper-security Firefox/188.8.131.52 Will post example file... Reproducible: Always Steps to Reproduce: 1.open the submitted file 2.press the popup buttons 3.observe the crash Also happened with Epiphany on Ubuntu Dapper
confirm crash in 1.8 (TB21498432E), trunk (TB21498498G) with todays builds on winxp.
In a debug build I hit the "JS_ASSERT(nbytes != 0);" in JS_Malloc, called from js_NewStringCopyN as in the talkback crash. In a non-debug build nbytes gets reset to 1 and a small bit allocated, success! Back in js_NewCopyStringN() the reason it was trying to allocate 0 bytes is because the string length passed in from the RegExp is -1 -- oops. With malloc successful it looks like js_strncpy tries to copy 0xffffffff characters into a 1 character buffer.
(sorry, nuked too much there).
FWIW, this is another minimal quantifier bug.
Brendan: any chance you could help out with a few well-placed RegExp error checks here? I assume we can't afford to check for -1 inside js_NewStringCopyN itself, but RegExp maybe could in the places that matter
Created attachment 235336 [details] [diff] [review] two line fix, return an empty string match instead of backtracking I think this is not really The Right Thing to Do here; probably the way these matches are compiled should be modified to detect potentially empty matches and generate a specific bytecode that does nothing but capture an empty string.
Created attachment 235339 [details] [diff] [review] Adding a comment and fixing a couple typos resulting from rename
Created attachment 235358 [details] A smaller js-shell usable test case This should go into a test suite somewhere somehow.
Brian and I want to try to avoid the condition that causes this negative length, rather than just protect against it occurring, but the existing patch is minimal-risk and probably a good candidate for 184.108.40.206 because of that.
Comment on attachment 235339 [details] [diff] [review] Adding a comment and fixing a couple typos resulting from rename r=shaver as per the previous comment in case you want it ASAP for 220.127.116.11, though we'd like something more fundamental for 1.8.1 and trunk, IMO.
Comment on attachment 235339 [details] [diff] [review] Adding a comment and fixing a couple typos resulting from rename approved for 1.8.0 branch, a=dveditz for drivers Please land this weekend. If you get a more robust fix for 1.8.1 we'll look at doing that in 18.104.22.168 instead of this.
Created attachment 235710 [details] [diff] [review] patch that I landed on the 1.8.0 branch Mainly style policing (80th column, major comment, no extra parens for + vs. ==), but also a FIXME in the comment citing the bug. /be
https://bugzilla.mozilla.org/attachment.cgi?id=230886&action=view should not crash. Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:22.214.171.124pre) Gecko/20060828 Firefox/126.96.36.199pre verified 188.8.131.52
Suitable for 1.8 branch? If so please nom as we'd like it there...
Comment on attachment 235710 [details] [diff] [review] patch that I landed on the 1.8.0 branch Should be pretty safe and seems to resolve the problem. I think the only reason this bug remains open (or should) is to remind me that there is a Better Fix to be done.
Comment on attachment 235710 [details] [diff] [review] patch that I landed on the 1.8.0 branch a=schrep
The only complication is that I still do not have privileges to land this. Can you do it again, Brendan?
Checking in jsregexp.c; /cvsroot/mozilla/js/src/jsregexp.c,v <-- jsregexp.c new revision: 184.108.40.206; previous revision: 220.127.116.11 done
How does this bug become fixed/verified? If there's a Schoolhouse Rocks song about it, that'd be helpful. Also can the attachment from comment #11 be made into a test-case?
Checking in regress-346090.js; /cvsroot/mozilla/js/tests/ecma_3/RegExp/regress-346090.js,v <-- regress-346090.js initial revision: 1.1 done
verified fixed 20061010 1.8 windows/mac*/linux, 1.9 windows/linux