625 bytes, text/html
12.70 KB, text/plain
3.84 KB, text/html
981 bytes, patch
|Details | Diff | Splinter Review|
Created attachment 657365 [details] Repro-file Reproducible with built from https://people.mozilla.com/~choller/firefox/asan/20120831-mozilla-central-linux64-debug-fcc533f691e9+asan.html OS: Ubuntu 12.04 x86_64 The reprofile is not stable and it might take few tries to reproduce. ASAN-report: ASAN:SIGSEGV ==11689== ERROR: AddressSanitizer crashed on unknown address 0x7f00dca7af80 (pc 0x7efff2512520 sp 0x7fffdf81b7d8 bp 0x7fffdf81b820 T0) AddressSanitizer can not provide additional info. ABORTING #0 0x7efff2512520 in ?? /build/buildd/eglibc-2.15/string/../sysdeps/x86_64/multiarch/memcpy-ssse3-back.S:2577 #1 0x7effe7e9c1bf in nsTArray_base<nsTArrayDefaultAllocator>::ShiftData(unsigned int, unsigned int, unsigned int, unsigned int, unsigned long) /builds/slave/try-lnx64-dbg/build/../../../dist/include/nsTArray-inl.h:247 #2 0x7effe8cb2b47 in nsRefPtr<nsHTMLOptionElement>* nsTArray<nsRefPtr<nsHTMLOptionElement>, nsTArrayDefaultAllocator>::ReplaceElementsAt<nsHTMLOptionElement*>(unsigned int, unsigned int, nsHTMLOptionElement* const*, unsigned int) /builds/slave/try-lnx64-dbg/build/../../../../dist/include/nsTArray.h:724 #3 0x7effe8ca6367 in nsHTMLOptionCollection::InsertOptionAt(nsHTMLOptionElement*, unsigned int) /builds/slave/try-lnx64-dbg/build/content/html/content/src/nsHTMLSelectElement.h:68 #4 0x7effe8ca5392 in nsHTMLSelectElement::InsertOptionsIntoListRecurse(nsIContent*, int*, int) /builds/slave/try-lnx64-dbg/build/content/html/content/src/nsHTMLSelectElement.cpp:338 #5 0x7effe8ca4d8e in nsHTMLSelectElement::InsertOptionsIntoList(nsIContent*, int, int, bool) /builds/slave/try-lnx64-dbg/build/content/html/content/src/nsHTMLSelectElement.cpp:215 #6 0x7effe8ca65fb in nsHTMLSelectElement::WillAddOptions(nsIContent*, nsIContent*, int, bool) /builds/slave/try-lnx64-dbg/build/content/html/content/src/nsHTMLSelectElement.cpp:455 #7 0x7effe8ca3881 in nsSafeOptionListMutation /builds/slave/try-lnx64-dbg/build/content/html/content/src/nsHTMLSelectElement.cpp:73 #8 0x7effe8ca4b51 in nsHTMLSelectElement::InsertChildAt(nsIContent*, unsigned int, bool) /builds/slave/try-lnx64-dbg/build/content/html/content/src/nsHTMLSelectElement.cpp:191 . . .
Created attachment 657372 [details] Whole asan-report Also before that ASAN-report there is following assertion ###!!! ASSERTION: insPos too small: 'insPos >= 1', file /builds/slave/try-lnx64-dbg/build/content/base/src/nsINode.cpp, line 1798 There is also few warnings you can see in the attached full ASAN-report.
It's really hard to reproduce this actually. I've been able to do it once but no more in a few dozen tries. I'm not really familiar with the involved code too. I don't know who is though.
Created attachment 657541 [details] More reliable repro-file I think that this repro-file is much more reliable than the original one.
I can reproduce the crash using the first testcase but not using the latter one.
For my laptop that latter file is almost 100% sure. There might be some race-condition. You could try to change the amount of the duplicated line inside the crash-function in the testcase, or try to change the for-loop rounds in that function in the first testcase.
Created attachment 657601 [details] [diff] [review] patch This fixes the original testcase. No idea about the latter one. (I hate mutation events.)
(I could use mutationguard too, but not sure it is worth.)
The 2nd testcase crashes reliably for me in a Linux64 debug build (non-asan). It crashes in memmove() called from nsTArray_base<nsTArrayDefaultAllocator>::ShiftData: (gdb) list 240 start *= elemSize; 241 newLen *= elemSize; 242 oldLen *= elemSize; 243 num *= elemSize; 244 char *base = reinterpret_cast<char*>(mHdr + 1) + start; 245 memmove(base + newLen, base + oldLen, num); 246 } 247 } 248 (gdb) p base $1 = 0x8000ddbc2490 <Address 0x8000ddbc2490 out of bounds> (gdb) p start $2 = 4294967288 (gdb) p/x start $3 = 0xfffffff8 (gdb) If something happened to be allocated at |base + newLen| then it's potentially exploitable? The patch fixed it for me. There's about 2000 crash-stats reports in the past 4 weeks that match "memmove | nsTArray_base<nsTArrayDefaultAllocator>::ShiftData".
Comment on attachment 657601 [details] [diff] [review] patch r=me
This appears to affect ESR-10 as well from code inspection (don't have an esr-10 ASAN build) http://mxr.mozilla.org/mozilla-esr10/source/content/base/src/nsGenericElement.cpp#4077
Olli is this ready to be landed?
Yes, but this particular bug is so obvious that I'd like to land it as close to next release as possible.
Great, just making sure you were not landing it on purpose. :)
(In reply to Olli Pettay [:smaug] from comment #13) > Yes, but this particular bug is so obvious that I'd like to land it as close > to next release as possible. Now's the time to land and nominate for uplift without putting the FF16 release at risk.
Comment on attachment 657601 [details] [diff] [review] patch [Approval Request Comment] Bug caused by (feature/regressing bug #): NA User impact if declined: Possible security sensitive crash Testing completed (on m-c, etc.): tested only locally Risk to taking this patch (and alternatives if risky): The patch is effectively just a null check String or UUID changes made by this patch: NA
Comment on attachment 657601 [details] [diff] [review] patch [Triage Comment] Approving for Aurora in preparation for Beta consideration.
Comment on attachment 657601 [details] [diff] [review] patch [Triage Comment] We've now had the opportunity to discuss in triage - let's land on Beta 16 and ESR10 before Tuesday.
Thanks for landing
I see you landed also to beta. I assume you'll do also esr.
Yup. I'm just making sure it builds locally because esr10 is a little different... https://hg.mozilla.org/releases/mozilla-beta/rev/77f02bc310f4
Both testcases reliably crash Firefox 18.0a1 Nightly debug 2012-09-01 (non-asan) for me. Neither testcase crashes: * Firefox 18.0a1 Nightly debug (non-asan) 2012-09-21 * Firefox 17.0a2 Aurora debug (non-asan) 2012-09-23 * Firefox 16.0 Beta debug (non-asan) 2012-09-23 * Firefox 10.0.8esrpre esr10 debug (non-asan) 2012-09-23 Question, is that sufficient to call this verified, or does the verification require checking against ASan builds?
I never used ASAn builds with this. The problem is clear so verifying without asan should be enough.
Thanks :smaug, marking this verified across the board based on my results in comment 25.