Last Comment Bug 771873 - (CVE-2012-3961) Heap-use-after-free in RangeData::~RangeData
(CVE-2012-3961)
: Heap-use-after-free in RangeData::~RangeData
Status: VERIFIED FIXED
[asan][advisory-tracking+][qa-]
: crash, sec-critical, testcase
Product: Core
Classification: Components
Component: Selection (show other bugs)
: Trunk
: All All
: -- critical (vote)
: mozilla16
Assigned To: :Aryeh Gregor (away until August 15)
:
Mentors:
: 759858 (view as bug list)
Depends on: 772387
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-07 23:12 PDT by Abhishek Arya
Modified: 2014-07-24 13:44 PDT (History)
12 users (show)
rforbes: sec‑bounty+
ehsan: in‑testsuite?
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
wontfix
+
fixed
+
verified
unaffected


Attachments
Testcase (993 bytes, text/html)
2012-07-07 23:12 PDT, Abhishek Arya
no flags Details
Patch part 1, adds a check to avoid subtraction underflow (2.84 KB, patch)
2012-07-09 01:13 PDT, :Aryeh Gregor (away until August 15)
no flags Details | Diff | Review
Patch part 2, add extra assert to nsTArray::RemoveElementsAt (1.17 KB, patch)
2012-07-09 01:16 PDT, :Aryeh Gregor (away until August 15)
benjamin: review+
Details | Diff | Review
Patch part 1 -- Resume spell checking from a script runner (1.06 KB, patch)
2012-07-10 03:47 PDT, :Aryeh Gregor (away until August 15)
no flags Details | Diff | Review
Patch part 1, v2 -- Bail out if indices are reversed (8.55 KB, patch)
2012-07-11 06:27 PDT, :Aryeh Gregor (away until August 15)
ehsan: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta+
Details | Diff | Review
ASAN Log (6.23 KB, text/plain)
2012-08-17 14:47 PDT, Anthony Hughes (:ashughes) [GFX][QA][Mentor]
no flags Details

Description Abhishek Arya 2012-07-07 23:12:39 PDT
Created attachment 640025 [details]
Testcase

Reproduces on trunk
20120707214206
http://hg.mozilla.org/mozilla-central/rev/9533b40ff28b

=================================================================
==25564== ERROR: AddressSanitizer heap-use-after-free on address 0x7f1cf1264780 at pc 0x7f1d2220e700 bp 0x7fffcdd81470 sp 0x7fffcdd81468
READ of size 8 at 0x7f1cf1264780 thread T0
    #0 0x7f1d2220e700 in ~nsRefPtr firefox/src/../../dist/include/nsAutoPtr.h:874
    #1 0x7f1d221a42b3 in ~nsRefPtr firefox/src/../../dist/include/nsAutoPtr.h:875
    #2 0x7f1d2288b4d3 in RangeData::~RangeData() firefox/src/../../dist/include/mozilla/Selection.h:24
    #3 0x7f1d227ea3b3 in RangeData::~RangeData() firefox/src/../../dist/include/mozilla/Selection.h:24
    #4 0x7f1d228838e3 in nsTArrayElementTraits<RangeData>::Destruct(RangeData*) firefox/src/../../dist/include/nsTArray.h:349
    #5 0x7f1d228823be in nsTArray<RangeData, nsTArrayDefaultAllocator>::DestructRange(unsigned int, unsigned int) firefox/src/../../dist/include/nsTArray.h:1210
    #6 0x7f1d22852ab7 in nsTArray<RangeData, nsTArrayDefaultAllocator>::RemoveElementsAt(unsigned int, unsigned int) firefox/src/../../dist/include/nsTArray.h:932
    #7 0x7f1d2283db0d in mozilla::Selection::AddItem(nsRange*, int*) firefox/src/layout/generic/nsSelection.cpp:3535
    #8 0x7f1d228674fe in mozilla::Selection::AddRange(nsIDOMRange*) firefox/src/layout/generic/nsSelection.cpp:4414
    #9 0x7f1d2b8c58db in mozInlineSpellChecker::AddRange(nsISelection*, nsIDOMRange*) firefox/src/extensions/spellcheck/src/mozInlineSpellChecker.cpp:1600
    #10 0x7f1d2b8c2d1d in mozInlineSpellChecker::DoSpellCheck(mozInlineSpellWordUtil&, nsISelection*, mozInlineSpellStatus*, bool*) firefox/src/extensions/spellcheck/src/mozInlineSpellChecker.cpp:1399
    #11 0x7f1d2b8c70f1 in mozInlineSpellChecker::ResumeCheck(mozInlineSpellStatus*) firefox/src/extensions/spellcheck/src/mozInlineSpellChecker.cpp:1488
    #12 0x7f1d2b8cf9dc in mozInlineSpellResume::Run() firefox/src/extensions/spellcheck/src/mozInlineSpellChecker.cpp:462
    #13 0x7f1d2d045ccd in nsThread::ProcessNextEvent(bool, bool*) firefox/src/xpcom/threads/nsThread.cpp:625
    #14 0x7f1d2ccd501d in NS_ProcessNextEvent_P(nsIThread*, bool) firefox/src/objdir-ff-asan-sym/xpcom/build/nsThreadUtils.cpp:217
    #15 0x7f1d2bd76226 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) firefox/src/ipc/glue/MessagePump.cpp:82
    #16 0x7f1d2d2f984a in MessageLoop::RunInternal() firefox/src/ipc/chromium/src/base/message_loop.cc:209
    #17 0x7f1d2d2f9693 in MessageLoop::RunHandler() firefox/src/ipc/chromium/src/base/message_loop.cc:202
    #18 0x7f1d2d2f9578 in MessageLoop::Run() firefox/src/ipc/chromium/src/base/message_loop.cc:176
    #19 0x7f1d2b2ae42e in nsBaseAppShell::Run() firefox/src/widget/xpwidgets/nsBaseAppShell.cpp:165
    #20 0x7f1d29ef9668 in nsAppStartup::Run() firefox/src/toolkit/components/startup/nsAppStartup.cpp:257
    #21 0x7f1d20736280 in XREMain::XRE_mainRun() firefox/src/toolkit/xre/nsAppRunner.cpp:3787
    #22 0x7f1d2073cc22 in XREMain::XRE_main(int, char**, nsXREAppData const*) firefox/src/toolkit/xre/nsAppRunner.cpp:3864
    #23 0x7f1d207400f2 in XRE_main firefox/src/toolkit/xre/nsAppRunner.cpp:3940
    #24 0x40c28f in do_main(int, char**) firefox/src/browser/app/nsBrowserApp.cpp:160
    #25 0x409cbd in main firefox/src/browser/app/nsBrowserApp.cpp:330
    #26 0x7f1d3b388c4d in ?? ??:0
0x7f1cf1264780 is located 0 bytes inside of 64-byte region [0x7f1cf1264780,0x7f1cf12647c0)
freed by thread T0 here:
    #0 0x4a4392 in free ??:0
    #1 0x7f1d382165c3 in moz_free firefox/src/memory/mozalloc/mozalloc.cpp:49
    #2 0x7f1d23d7e6c6 in ~nsRange firefox/src/content/base/src/nsRange.cpp:215
    #3 0x7f1d23d82997 in nsRange::Release() firefox/src/content/base/src/nsRange.cpp:258
    #4 0x7f1d2220e6a0 in ~nsRefPtr firefox/src/../../../dist/include/nsAutoPtr.h:874
    #5 0x7f1d221a42b3 in ~nsRefPtr firefox/src/../../../dist/include/nsAutoPtr.h:875
    #6 0x7f1d2288b4d3 in RangeData::~RangeData() firefox/src/../../dist/include/mozilla/Selection.h:24
    #7 0x7f1d227ea3b3 in RangeData::~RangeData() firefox/src/../../dist/include/mozilla/Selection.h:24
    #8 0x7f1d228838e3 in nsTArrayElementTraits<RangeData>::Destruct(RangeData*) firefox/src/../../dist/include/nsTArray.h:349
    #9 0x7f1d228823be in nsTArray<RangeData, nsTArrayDefaultAllocator>::DestructRange(unsigned int, unsigned int) firefox/src/../../dist/include/nsTArray.h:1210
    #10 0x7f1d22852ab7 in nsTArray<RangeData, nsTArrayDefaultAllocator>::RemoveElementsAt(unsigned int, unsigned int) firefox/src/../../dist/include/nsTArray.h:932
    #11 0x7f1d2283db0d in mozilla::Selection::AddItem(nsRange*, int*) firefox/src/layout/generic/nsSelection.cpp:3535
    #12 0x7f1d228674fe in mozilla::Selection::AddRange(nsIDOMRange*) firefox/src/layout/generic/nsSelection.cpp:4414
    #13 0x7f1d2b8c58db in mozInlineSpellChecker::AddRange(nsISelection*, nsIDOMRange*) firefox/src/extensions/spellcheck/src/mozInlineSpellChecker.cpp:1600
    #14 0x7f1d2b8c2d1d in mozInlineSpellChecker::DoSpellCheck(mozInlineSpellWordUtil&, nsISelection*, mozInlineSpellStatus*, bool*) firefox/src/extensions/spellcheck/src/mozInlineSpellChecker.cpp:1399
    #15 0x7f1d2b8c70f1 in mozInlineSpellChecker::ResumeCheck(mozInlineSpellStatus*) firefox/src/extensions/spellcheck/src/mozInlineSpellChecker.cpp:1488
    #16 0x7f1d2b8cf9dc in mozInlineSpellResume::Run() firefox/src/extensions/spellcheck/src/mozInlineSpellChecker.cpp:462
    #17 0x7f1d2d045ccd in nsThread::ProcessNextEvent(bool, bool*) firefox/src/xpcom/threads/nsThread.cpp:625
    #18 0x7f1d2ccd501d in NS_ProcessNextEvent_P(nsIThread*, bool) firefox/src/objdir-ff-asan-sym/xpcom/build/nsThreadUtils.cpp:217
    #19 0x7f1d2bd76226 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) firefox/src/ipc/glue/MessagePump.cpp:82
    #20 0x7f1d2d2f984a in MessageLoop::RunInternal() firefox/src/ipc/chromium/src/base/message_loop.cc:209
    #21 0x7f1d2d2f9693 in MessageLoop::RunHandler() firefox/src/ipc/chromium/src/base/message_loop.cc:202
    #22 0x7f1d2d2f9578 in MessageLoop::Run() firefox/src/ipc/chromium/src/base/message_loop.cc:176
    #23 0x7f1d2b2ae42e in nsBaseAppShell::Run() firefox/src/widget/xpwidgets/nsBaseAppShell.cpp:165
    #24 0x7f1d29ef9668 in nsAppStartup::Run() firefox/src/toolkit/components/startup/nsAppStartup.cpp:257
    #25 0x7f1d20736280 in XREMain::XRE_mainRun() firefox/src/toolkit/xre/nsAppRunner.cpp:3787
    #26 0x7f1d2073cc22 in XREMain::XRE_main(int, char**, nsXREAppData const*) firefox/src/toolkit/xre/nsAppRunner.cpp:3864
    #27 0x7f1d207400f2 in XRE_main firefox/src/toolkit/xre/nsAppRunner.cpp:3940
    #28 0x40c28f in do_main(int, char**) firefox/src/browser/app/nsBrowserApp.cpp:160
    #29 0x409cbd in main firefox/src/browser/app/nsBrowserApp.cpp:330
previously allocated by thread T0 here:
    #0 0x4a4452 in __interceptor_malloc ??:0
    #1 0x7f1d38216717 in moz_xmalloc firefox/src/memory/mozalloc/mozalloc.cpp:54
    #2 0x7f1d2b8dc0e3 in mozInlineSpellWordUtil::MakeRange(mozInlineSpellWordUtil::NodeOffset, mozInlineSpellWordUtil::NodeOffset, nsRange**) firefox/src/extensions/spellcheck/src/mozInlineSpellWordUtil.cpp:322
    #3 0x7f1d2b8d9fc6 in mozInlineSpellWordUtil::MakeRangeForWord(mozInlineSpellWordUtil::RealWord const&, nsRange**) firefox/src/extensions/spellcheck/src/mozInlineSpellWordUtil.cpp:226
    #4 0x7f1d2b8dee62 in mozInlineSpellWordUtil::GetNextWord(nsAString_internal&, nsRange**, bool*) firefox/src/extensions/spellcheck/src/mozInlineSpellWordUtil.cpp:297
    #5 0x7f1d2b8c1906 in mozInlineSpellChecker::DoSpellCheck(mozInlineSpellWordUtil&, nsISelection*, mozInlineSpellStatus*, bool*) firefox/src/extensions/spellcheck/src/mozInlineSpellChecker.cpp:1317
    #6 0x7f1d2b8c70f1 in mozInlineSpellChecker::ResumeCheck(mozInlineSpellStatus*) firefox/src/extensions/spellcheck/src/mozInlineSpellChecker.cpp:1488
    #7 0x7f1d2b8cf9dc in mozInlineSpellResume::Run() firefox/src/extensions/spellcheck/src/mozInlineSpellChecker.cpp:462
    #8 0x7f1d2d045ccd in nsThread::ProcessNextEvent(bool, bool*) firefox/src/xpcom/threads/nsThread.cpp:625
    #9 0x7f1d2ccd501d in NS_ProcessNextEvent_P(nsIThread*, bool) firefox/src/objdir-ff-asan-sym/xpcom/build/nsThreadUtils.cpp:217
    #10 0x7f1d2bd76226 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) firefox/src/ipc/glue/MessagePump.cpp:82
    #11 0x7f1d2d2f984a in MessageLoop::RunInternal() firefox/src/ipc/chromium/src/base/message_loop.cc:209
    #12 0x7f1d2d2f9693 in MessageLoop::RunHandler() firefox/src/ipc/chromium/src/base/message_loop.cc:202
    #13 0x7f1d2d2f9578 in MessageLoop::Run() firefox/src/ipc/chromium/src/base/message_loop.cc:176
    #14 0x7f1d2b2ae42e in nsBaseAppShell::Run() firefox/src/widget/xpwidgets/nsBaseAppShell.cpp:165
    #15 0x7f1d29ef9668 in nsAppStartup::Run() firefox/src/toolkit/components/startup/nsAppStartup.cpp:257
    #16 0x7f1d20736280 in XREMain::XRE_mainRun() firefox/src/toolkit/xre/nsAppRunner.cpp:3787
    #17 0x7f1d2073cc22 in XREMain::XRE_main(int, char**, nsXREAppData const*) firefox/src/toolkit/xre/nsAppRunner.cpp:3864
    #18 0x7f1d207400f2 in XRE_main firefox/src/toolkit/xre/nsAppRunner.cpp:3940
    #19 0x40c28f in do_main(int, char**) firefox/src/browser/app/nsBrowserApp.cpp:160
    #20 0x409cbd in main firefox/src/browser/app/nsBrowserApp.cpp:330
    #21 0x7f1d3b388c4d in ?? ??:0
==25564== ABORTING
Stats: 156M malloced (189M for red zones) by 464984 calls
Stats: 41M realloced by 20345 calls
Stats: 112M freed by 231687 calls
Stats: 0M really freed by 0 calls
Stats: 376M (96306 full pages) mmaped in 94 calls
  mmaps   by size class: 8:393192; 9:49146; 10:20475; 11:18423; 12:3072; 13:2048; 14:1536; 15:384; 16:576; 17:128; 18:176; 19:48; 20:16;
  mallocs by size class: 8:376983; 9:47309; 10:16409; 11:17335; 12:2458; 13:1841; 14:1416; 15:336; 16:562; 17:116; 18:165; 19:41; 20:13;
  frees   by size class: 8:163249; 9:36760; 10:12958; 11:14054; 12:1531; 13:948; 14:1228; 15:278; 16:474; 17:101; 18:58; 19:38; 20:10;
  rfrees  by size class:
Stats: malloc large: 335 small slow: 2084
Shadow byte and word:
  0x1fe39e24c8f0: fd
  0x1fe39e24c8f0: fd fd fd fd fd fd fd fd
More shadow bytes:
  0x1fe39e24c8d0: fd fd fd fd fd fd fd fd
  0x1fe39e24c8d8: fd fd fd fd fd fd fd fd
  0x1fe39e24c8e0: fa fa fa fa fa fa fa fa
  0x1fe39e24c8e8: fa fa fa fa fa fa fa fa
=>0x1fe39e24c8f0: fd fd fd fd fd fd fd fd
  0x1fe39e24c8f8: fd fd fd fd fd fd fd fd
  0x1fe39e24c900: fa fa fa fa fa fa fa fa
  0x1fe39e24c908: fa fa fa fa fa fa fa fa
  0x1fe39e24c910: fd fd fd fd fd fd fd fd
Comment 1 :Aryeh Gregor (away until August 15) 2012-07-09 01:13:19 PDT
Created attachment 640150 [details] [diff] [review]
Patch part 1, adds a check to avoid subtraction underflow

This doesn't address why startIndex and endIndex were reversed in the first place.  It does fix the crash.
Comment 2 :Aryeh Gregor (away until August 15) 2012-07-09 01:16:25 PDT
Created attachment 640152 [details] [diff] [review]
Patch part 2, add extra assert to nsTArray::RemoveElementsAt

This would have caught the bug more cleanly.  There's already an assert to check that start + count <= Length(), but in this case that passed because the addition overflows -- count is actually 2^32 - 1.
Comment 3 :Aryeh Gregor (away until August 15) 2012-07-09 01:17:36 PDT
I forgot to post my comment before posting patches:

(In reply to Abhishek Arya from comment #0)
>     #6 0x7f1d22852ab7 in nsTArray<RangeData,
> nsTArrayDefaultAllocator>::RemoveElementsAt(unsigned int, unsigned int)
> firefox/src/../../dist/include/nsTArray.h:932

The arguments are instructive when I reproduce locally:

#10 0xb491c287 in nsTArray<RangeData, nsTArrayDefaultAllocator>::RemoveElementsAt (this=0x98812ad4, 
    start=61, count=4294967295) at ../../dist/include/nsTArray.h:931

The call line is:

  mRanges.RemoveElementsAt(startIndex, endIndex - startIndex);

So looks like startIndex > endIndex here.  They're set by GetIndicesForInterval.  Specifically, sticking in some MOZ_ASSERTs and printf()s: FindInsertionPoint() sets endsBeforeIndex to 60, and beginsAfterIndex to 61.  mRanges.Length() is 65 (!).  aAllowAdjacent is false.

So the easy workaround is adding

  if (startIndex > endIndex) {
    PRInt32 tmp = startIndex;
    startIndex = endIndex;
    endIndex = tmp;
  }

and that fixes the crash/asserts.  Also, nsTArray should assert that count <= Length() to catch future cases like this more quickly.

That leaves the question of why GetIndicesForInterval ever returns a start index greater than the end index.  That can be fixed separately, though.
Comment 4 :Aryeh Gregor (away until August 15) 2012-07-09 01:32:39 PDT
Try for the two patches so far: https://tbpl.mozilla.org/?tree=Try&rev=206ebf4d367c
Comment 5 :Ehsan Akhgari (busy, don't ask for review please) 2012-07-09 13:58:14 PDT
Why did you choose to wallpaper over the crash and not fix GetIndicesForInterval itself?  It seems to me that if GetIndicesForInterval has a bug, we should fix that, instead of leaving all of the other callers affected by this kind of behavior.

Also, please don't include the test case when you're going to land your patches here, as it might be potentially helpful to attackers turning this bug into an exploit.
Comment 6 :Aryeh Gregor (away until August 15) 2012-07-10 00:21:38 PDT
(In reply to Ehsan Akhgari [:ehsan] from comment #5)
> Why did you choose to wallpaper over the crash and not fix
> GetIndicesForInterval itself?  It seems to me that if GetIndicesForInterval
> has a bug, we should fix that, instead of leaving all of the other callers
> affected by this kind of behavior.

Because it's a lot easier, and an immediate but incomplete fix seems valuable to me.  This I could do easily; actually figuring out what's wrong with GetIndicesForInterval is not as easy.  To fix other callers too, I could put the swapping in GetIndicesForInterval itself instead of in the caller.

> Also, please don't include the test case when you're going to land your
> patches here, as it might be potentially helpful to attackers turning this
> bug into an exploit.

Okay.
Comment 7 :Aryeh Gregor (away until August 15) 2012-07-10 03:47:49 PDT
Created attachment 640557 [details] [diff] [review]
Patch part 1 -- Resume spell checking from a script runner

Okay, so this took a lot of pounding my head against the wall, but I eventually discovered this:

(gdb) p mRanges[60].mRange->mEndParent->mFirstChild
$55 = (nsSVGSVGElement *) 0x9e1f3b00
(gdb) p mRanges[60].mRange->mEndParent->mFirstChild->mNextSibling
$56 = (nsTextNode *) 0xa1579fb0
(gdb) p mRanges[60].mRange->mEndParent->mFirstChild->mNextSibling->mNextSibling
$57 = (nsTextNode *) 0xa15909c0
(gdb) p mRanges[60].mRange->mEndOffset
$58 = 2
(gdb) p mRanges[60].mRange->mStartParent
$59 = {mRawPtr = 0xa15909c0}

The end offset of this range is 2.  The start parent is the third child of the end parent.  So the range's start is before its end.  Thus the end of the new range is before the start of ranges 0-60, but its start is after the end of ranges 0-61.  So the indexes are quite legitimate; it's the range that's messed up.  The reason is simple: once again, range endpoints have not yet been updated.

Fortunately, the fix is simple.  For the future, I'm starting to think we should hardcode range endpoint updating into the core methods that mutate the DOM, before absolutely anything else happens.  This kind of thing is much too likely to happen given the way we do things now.

https://tbpl.mozilla.org/?tree=Try&rev=89b14e21f047
Comment 8 :Aryeh Gregor (away until August 15) 2012-07-10 04:51:48 PDT
Comment on attachment 640557 [details] [diff] [review]
Patch part 1 -- Resume spell checking from a script runner

Okay, so judging by the try run, this seems to break spellcheck.  What do you suggest I do?
Comment 9 Benjamin Smedberg [:bsmedberg] 2012-07-10 07:08:37 PDT
Comment on attachment 640152 [details] [diff] [review]
Patch part 2, add extra assert to nsTArray::RemoveElementsAt

As I understand the most recent dev.platform discussions about underflow, this assert is defined behavior because the underflow of unsigned ints is defined behavior?
Comment 10 :Aryeh Gregor (away until August 15) 2012-07-10 07:32:14 PDT
Apparently, yes.  That's what 3.9.1.4 here seems to say:

http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2012/n3337.pdf
Comment 11 :Ehsan Akhgari (busy, don't ask for review please) 2012-07-10 12:26:49 PDT
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #9)
> Comment on attachment 640152 [details] [diff] [review]
> Patch part 2, add extra assert to nsTArray::RemoveElementsAt
> 
> As I understand the most recent dev.platform discussions about underflow,
> this assert is defined behavior because the underflow of unsigned ints is
> defined behavior?

Hmm yeah this should make spell checking happen in sync, which at best should be a perf hit and at worst should break assumptions that the spellcheck code makes.

How about just doing what you did before in GetIndicesForInterval itself?  I don't have a lot of ideas here, and I agree that this is a pain to debug. :(
Comment 12 :Aryeh Gregor (away until August 15) 2012-07-11 06:27:28 PDT
Created attachment 641030 [details] [diff] [review]
Patch part 1, v2 -- Bail out if indices are reversed

This fixes the crash, but things will be all kinds of broken when mRanges has ranges in it with their ends before their starts.  So I left an NS_ASSERTION.  I guess a while after the fix has been pushed out to users, we can commit the crashtest too, right?
Comment 13 :Aryeh Gregor (away until August 15) 2012-07-11 06:31:45 PDT
Try: https://tbpl.mozilla.org/?tree=Try&rev=e64a1793a92f
Comment 14 :Ehsan Akhgari (busy, don't ask for review please) 2012-07-11 10:35:05 PDT
Comment on attachment 641030 [details] [diff] [review]
Patch part 1, v2 -- Bail out if indices are reversed

Review of attachment 641030 [details] [diff] [review]:
-----------------------------------------------------------------

sigh....
Comment 15 :Ehsan Akhgari (busy, don't ask for review please) 2012-07-11 10:36:35 PDT
(In reply to :Aryeh Gregor from comment #12)
> Created attachment 641030 [details] [diff] [review]
> Patch part 1, v2 -- Bail out if indices are reversed
> 
> This fixes the crash, but things will be all kinds of broken when mRanges
> has ranges in it with their ends before their starts.  So I left an
> NS_ASSERTION.  I guess a while after the fix has been pushed out to users,
> we can commit the crashtest too, right?

Yes.  We usually wait until the bug gets opened up, which happens when the fix has shipped to users.
Comment 18 Alex Keybl [:akeybl] 2012-07-13 15:33:33 PDT
Until we have a regression window, it's unclear if this sg:crit bug should be considered for release in a FF14 chemspill. We'll wait to make that evaluation.
Comment 19 Alex Keybl [:akeybl] 2012-07-14 22:01:04 PDT
Just to be clear, this would not drive a chemspill but would be considered if we need to roll a 14.0.2.
Comment 20 juan becerra [:juanb] 2012-07-14 23:44:47 PDT
Narrowed it down to 12 and 13b1, which showed a spell checker uplift from mozilla-central:

https://bugzilla.mozilla.org/show_bug.cgi?id=743819

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=75c7378c87b6&tochange=cc5254f9825f

Builds prior to 4/25 don't crash.
Comment 21 Daniel Veditz [:dveditz] 2012-07-15 18:41:37 PDT
That should make ESR-10 unaffected.
Comment 22 Alex Keybl [:akeybl] 2012-07-16 06:53:02 PDT
Discussed with Dan - we'll track for FF14, but only consider as part of a 14.0.2 chemspill if one is required for other reasons.
Comment 23 Alex Keybl [:akeybl] 2012-07-26 17:32:53 PDT
(In reply to :Aryeh Gregor from comment #17)
> https://hg.mozilla.org/mozilla-central/rev/a0c8be30a28b
> https://hg.mozilla.org/mozilla-central/rev/3f00954a0301

Would you mind nominating for FF15 approval if deemed low risk? Thanks!
Comment 24 :Ehsan Akhgari (busy, don't ask for review please) 2012-07-26 19:38:49 PDT
Comment on attachment 641030 [details] [diff] [review]
Patch part 1, v2 -- Bail out if indices are reversed

[Approval Request Comment]
Bug caused by (feature/regressing bug #): n/a
User impact if declined: sec-critical
Testing completed (on m-c, etc.): on m-c
Risk to taking this patch (and alternatives if risky): the patch is sane and relatively low risk.  It's basically a wallpaper.
String or UUID changes made by this patch: none
Comment 25 Alex Keybl [:akeybl] 2012-07-27 16:48:28 PDT
Comment on attachment 641030 [details] [diff] [review]
Patch part 1, v2 -- Bail out if indices are reversed

[Triage Comment]
If landed before Tuesday, this will make it into beta 3 (which will give us time to react to any new regressions). Approved for branches.
Comment 26 :Ehsan Akhgari (busy, don't ask for review please) 2012-07-30 10:56:54 PDT
This had landed before the Aurora uplift, so no need to land it there...
Comment 27 :Ehsan Akhgari (busy, don't ask for review please) 2012-07-30 11:03:54 PDT
https://hg.mozilla.org/releases/mozilla-beta/rev/268e31db5949
Comment 28 :Aryeh Gregor (away until August 15) 2012-08-13 07:26:32 PDT
*** Bug 759858 has been marked as a duplicate of this bug. ***
Comment 29 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-08-17 14:47:07 PDT
Created attachment 652939 [details]
ASAN Log

Testcase still crashes for me using :decoder's AddressSanitizer build for mozilla-central at revision 89dcadd42ec4 (debug), built on 20120803. Based on the date of check-in this should not be happening, should it?
Comment 30 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-08-17 14:54:56 PDT
(In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #29)
> Created attachment 652939 [details]
> ASAN Log
> 
> Testcase still crashes for me using :decoder's AddressSanitizer build for
> mozilla-central at revision 89dcadd42ec4 (debug), built on 20120803. Based
> on the date of check-in this should not be happening, should it?

AddressSanitizer build for mozilla-aurora at revision 08e55f351f6d (debug) built on 20120817 does not reproduce a crash and displays "OTs@ tAar,t pf] |X# ]N2tKObhm?LAiAy )9.CDoic* J,F [NE(I[F;yI~aRni9 [[em4jOXgh uIVfj,}=j m[ MJ Ebc-dd%jWNDU7Hc,SNYXJk- mkd 5ejJ )E+R*WoNc W fEA ZXQ:O.l9q*QqMQ*NLEs~)m ^g%$Hz:4 $r8p{=#m@Pi- X" in content.
Comment 31 :Ehsan Akhgari (busy, don't ask for review please) 2012-08-21 14:13:19 PDT
(In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #29)
> Created attachment 652939 [details]
> ASAN Log
> 
> Testcase still crashes for me using :decoder's AddressSanitizer build for
> mozilla-central at revision 89dcadd42ec4 (debug), built on 20120803. Based
> on the date of check-in this should not be happening, should it?

Can you please get stacks for this crash?
Comment 32 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-08-21 16:21:37 PDT
(In reply to Ehsan Akhgari [:ehsan] from comment #31)
> (In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #29)
> Can you please get stacks for this crash?

I've never tried to retrieve a stack before -- can you please provide me some documentation/guidance?
Comment 33 :Ehsan Akhgari (busy, don't ask for review please) 2012-08-21 16:48:32 PDT
ASAN includes a script which symbolicates the call stacks: <http://code.google.com/p/address-sanitizer/wiki/AddressSanitizer#Call_stack>
Comment 34 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-08-23 16:06:40 PDT
I can't seem to reproduce this crash anymore. However with a try-server ASan build from decoder with changeset 9f3cc040e41a I can't reproduce the testcase. I've been trying to get my own builds set up but I've been running into technical hurdles that decoder has been helping me through. For the sake of time, given that we are days from release, would someone who can reproduce this please help with verification?
Comment 35 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-08-24 14:17:28 PDT
I managed to get my own try-server ASAN build built from http://hg.mozilla.org/mozilla-central/rev/9533b40ff28b as mentioned in comment 0 and am able to reproduce this bug now. Will now test to verify fixed.
Comment 36 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-08-24 14:27:02 PDT
Verified fixed with Firefox 16.0a2 2012-08-24 ASAN build. Marking [qa-] since I can't verify against Firefox 15 Beta.

Note You need to log in before you can comment on or make changes to this bug.