771976 - (CVE-2012-3960) Heap-use-after-free in mozSpellChecker::SetCurrentDictionary

Status: RESOLVED FIXED

(Core :: Spelling checker, defect)

Description

[Abhishek Arya]

Reproduced on trunk. It wasn't a reliable crash, will update bug if i get a reliable testcase. Looks like |this|(mozSpellChecker) is blown from underneath[see free stack] and then causes a crash on reading its member variable here
357       mSpellCheckingEngine->SetPersonalDictionary(personalDictionary.get());

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

=================================================================
==7193== ERROR: AddressSanitizer heap-use-after-free on address 0x7fd80c73caa8 at pc 0x7fd83187223d bp 0x7fff83f39930 sp 0x7fff83f39928
READ of size 8 at 0x7fd80c73caa8 thread T0
    #0 0x7fd83187223d in mozSpellChecker::SetCurrentDictionary(nsAString_internal const&) extensions/spellcheck/src/mozSpellChecker.cpp:357
    #1 0x7fd83133d55d in nsEditorSpellCheck::SetCurrentDictionary(nsAString_internal const&) editor/composer/src/nsEditorSpellCheck.cpp:523
    #2 0x7fd83133ede7 in nsEditorSpellCheck::UpdateCurrentDictionary() editor/composer/src/nsEditorSpellCheck.cpp:651
    #3 0x7fd83188b2d6 in mozInlineSpellChecker::UpdateCurrentDictionary() extensions/spellcheck/src/mozInlineSpellChecker.cpp:1783
    #4 0x7fd830558864 in nsEditorEventListener::Focus(nsIDOMEvent*) editor/libeditor/base/nsEditorEventListener.cpp:888
    #5 0x7fd83055392f in nsEditorEventListener::HandleEvent(nsIDOMEvent*) editor/libeditor/base/nsEditorEventListener.cpp:311
    #6 0x7fd82fd3939e in nsRefPtr<nsIDOMEventListener>::operator nsIDOMEventListener*() const content/events/src/nsEventListenerManager.cpp:818
    #7 0x7fd82fd9979d in nsEventTargetChainItem::HandleEvent(nsEventChainPostVisitor&, unsigned int, bool, nsCxPusher*) content/events/src/nsEventListenerManager.h:143
0x7fd80c73caa8 is located 40 bytes inside of 56-byte region [0x7fd80c73ca80,0x7fd80c73cab8)
freed by thread T0 here:
    #0 0x424442 in free 
    #1 0x7fd83186bc99 in mozSpellChecker::Release() extensions/spellcheck/src/mozSpellChecker.cpp:16
    #2 0x7fd83052a870 in nsCOMPtr<nsIInlineSpellChecker>::~nsCOMPtr() ../../../dist/include/nsCOMPtr.h:442
    #3 0x7fd83052a68d in non-virtual thunk to nsEditor::Observe(nsISupports*, char const*, unsigned short const*) ../../../dist/include/nsCOMPtr.h:757
    #4 0x7fd831ca14e5 in nsTHashtable<nsObserverList>::GetEntry(char const*) const ../../dist/include/nsTHashtable.h:147
previously allocated by thread T0 here:
    #0 0x424502 in __interceptor_malloc 
    #1 0x7fd8349d61a9 in moz_xmalloc memory/mozalloc/mozalloc.cpp:54
==7193== ABORTING
Stats: 186M malloced (225M for red zones) by 546881 calls
Stats: 36M realloced by 21270 calls
Stats: 147M freed by 250321 calls
Stats: 13M really freed by 49274 calls
Stats: 420M (107580 full pages) mmaped in 105 calls
  mmaps   by size class: 8:425958; 9:57337; 10:20475; 11:14329; 12:5120; 13:3072; 14:1792; 15:512; 16:704; 17:128; 18:224; 19:40; 20:16;
  mallocs by size class: 8:448516; 9:54348; 10:19102; 11:14230; 12:4418; 13:2919; 14:1824; 15:407; 16:723; 17:119; 18:221; 19:41; 20:13;
  frees   by size class: 8:177903; 9:37821; 10:14745; 11:11000; 12:3384; 13:2594; 14:1557; 15:344; 16:652; 17:98; 18:173; 19:39; 20:11;
  rfrees  by size class: 8:42301; 9:3888; 10:1120; 11:1186; 12:273; 13:185; 14:128; 15:52; 16:129; 17:7; 18:4; 19:1;
Stats: malloc large: 394 small slow: 2548
Shadow byte and word:
  0x1ffb018e7955: fd
  0x1ffb018e7950: fd fd fd fd fd fd fd fd
More shadow bytes:
  0x1ffb018e7930: fd fd fd fd fd fd fd fd
  0x1ffb018e7938: fd fd fd fd fd fd fd fd
  0x1ffb018e7940: fa fa fa fa fa fa fa fa
  0x1ffb018e7948: fa fa fa fa fa fa fa fa
=>0x1ffb018e7950: fd fd fd fd fd fd fd fd
  0x1ffb018e7958: fd fd fd fd fd fd fd fd
  0x1ffb018e7960: fa fa fa fa fa fa fa fa
  0x1ffb018e7968: fa fa fa fa fa fa fa fa
  0x1ffb018e7970: 00 00 00 00 00 00 00 00

Comment 1

[(no longer active)]

Hmm, this is really weird.  All of this stuff gets held on to with strong references, and the free stack is too short to figure out what's happened here.  :(

This is probably timing dependent, but we really need a testcase to figure out what's happening here...

Comment 2

[Abhishek Arya]

it seems pretty timing dependant. it got hit again, but flakily. here is a better stack (also from an optimized build, so some frames might be missing from in-between).

=================================================================
==3319== ERROR: AddressSanitizer heap-use-after-free on address 0x7f7714f67ea8 at pc 0x7f774648f904 bp 0x7fff3e0e97a0 sp 0x7fff3e0e9798
READ of size 8 at 0x7f7714f67ea8 thread T0
    #0 0x7f774648f904 in nsCOMPtr<mozISpellCheckingEngine>::get() const firefox/src/modules/zlib/src/inffast.c:0
    #1 0x7f774611a84c in nsEditorSpellCheck::SetCurrentDictionary(nsAString_internal const&) firefox/src/editor/composer/src/nsEditorSpellCheck.cpp:523
    #2 0x7f774611b1a9 in nsEditorSpellCheck::UpdateCurrentDictionary() firefox/src/editor/composer/src/nsEditorSpellCheck.cpp:651
    #3 0x7f774649ddea in mozInlineSpellChecker::UpdateCurrentDictionary() firefox/src/extensions/spellcheck/src/mozInlineSpellChecker.cpp:1782
    #4 0x7f77457c9b56 in nsEditorEventListener::Focus(nsIDOMEvent*) firefox/src/editor/libeditor/base/nsEditorEventListener.cpp:890
    #5 0x7f77457c5c51 in nsEditorEventListener::HandleEvent(nsIDOMEvent*) firefox/src/editor/libeditor/base/nsEditorEventListener.cpp:295
    #6 0x7f774524bb8b in nsEventListenerManager::HandleEventSubType(nsListenerStruct*, nsIDOMEventListener*, nsIDOMEvent*, nsIDOMEventTarget*, unsigned int, nsCxPusher*) firefox/src/content/events/src/nsEventListenerManager.cpp:820
    #7 0x7f774524bfd2 in nsEventListenerManager::HandleEventInternal(nsPresContext*, nsEvent*, nsIDOMEvent**, nsIDOMEventTarget*, unsigned int, nsEventStatus*, nsCxPusher*) firefox/src/content/events/src/nsEventListenerManager.cpp:893
    #8 0x7f774528ccbd in nsEventTargetChainItem::HandleEvent(nsEventChainPostVisitor&, unsigned int, bool, nsCxPusher*) firefox/src/content/events/src/nsEventDispatcher.cpp:182
0x7f7714f67ea8 is located 40 bytes inside of 56-byte region [0x7f7714f67e80,0x7f7714f67eb8)
freed by thread T0 here:
    #0 0x425a42 in free ??:0
    #1 0x7f774648ab53 in mozSpellChecker::Release() firefox/src/extensions/spellcheck/src/mozSpellChecker.cpp:16
    #2 0x7f77457a3dae in nsEditor::SyncRealTimeSpell() firefox/src/editor/libeditor/base/nsEditor.cpp:1315
    #3 0x7f77457a3923 in nsEditor::Observe(nsISupports*, char const*, unsigned short const*) firefox/src/editor/libeditor/base/nsEditor.cpp:1284
    #4 0x7f77457a3c2a in non-virtual thunk to nsEditor::Observe(nsISupports*, char const*, unsigned short const*) firefox/src/modules/zlib/src/inffast.c:0
    #5 0x7f7746799f59 in nsObserverService::NotifyObservers(nsISupports*, char const*, unsigned short const*) firefox/src/xpcom/ds/nsObserverService.cpp:152
    #6 0x7f774648ee8b in mozSpellChecker::SetCurrentDictionary(nsAString_internal const&) firefox/src/extensions/spellcheck/src/mozSpellChecker.cpp:353
    #7 0x7f774611a84c in nsEditorSpellCheck::SetCurrentDictionary(nsAString_internal const&) firefox/src/editor/composer/src/nsEditorSpellCheck.cpp:523
    #8 0x7f774611b1a9 in nsEditorSpellCheck::UpdateCurrentDictionary() firefox/src/editor/composer/src/nsEditorSpellCheck.cpp:651
    #9 0x7f774649ddea in mozInlineSpellChecker::UpdateCurrentDictionary() firefox/src/extensions/spellcheck/src/mozInlineSpellChecker.cpp:1782
    #10 0x7f77457c9b56 in nsEditorEventListener::Focus(nsIDOMEvent*) firefox/src/editor/libeditor/base/nsEditorEventListener.cpp:890
    #11 0x7f77457c5c51 in nsEditorEventListener::HandleEvent(nsIDOMEvent*) firefox/src/editor/libeditor/base/nsEditorEventListener.cpp:295
    #12 0x7f774524bb8b in nsEventListenerManager::HandleEventSubType(nsListenerStruct*, nsIDOMEventListener*, nsIDOMEvent*, nsIDOMEventTarget*, unsigned int, nsCxPusher*) firefox/src/content/events/src/nsEventListenerManager.cpp:820
    #13 0x7f774524bfd2 in nsEventListenerManager::HandleEventInternal(nsPresContext*, nsEvent*, nsIDOMEvent**, nsIDOMEventTarget*, unsigned int, nsEventStatus*, nsCxPusher*) firefox/src/content/events/src/nsEventListenerManager.cpp:893
    #14 0x7f774528ccbd in nsEventTargetChainItem::HandleEvent(nsEventChainPostVisitor&, unsigned int, bool, nsCxPusher*) firefox/src/content/events/src/nsEventDispatcher.cpp:182
previously allocated by thread T0 here:
    #0 0x425b02 in __interceptor_malloc ??:0
    #1 0x7f7749cba3f0 in moz_xmalloc firefox/src/memory/mozalloc/mozalloc.cpp:54
==3319== ABORTING
Stats: 213M malloced (259M for red zones) by 652310 calls
Stats: 51M realloced by 39848 calls
Stats: 155M freed by 373084 calls
Stats: 26M really freed by 96104 calls
Stats: 460M (117822 full pages) mmaped in 115 calls
  mmaps   by size class: 8:458724; 9:73719; 10:24570; 11:20470; 12:5120; 13:3584; 14:1792; 15:512; 16:704; 17:160; 18:208; 19:48; 20:16;
  mallocs by size class: 8:526562; 9:67635; 10:24057; 11:22797; 12:4445; 13:3300; 14:1902; 15:453; 16:705; 17:173; 18:225; 19:43; 20:13;
  frees   by size class: 8:272813; 9:53499; 10:19518; 11:18851; 12:3211; 13:2295; 14:1652; 15:386; 16:585; 17:150; 18:75; 19:39; 20:10;
  rfrees  by size class: 8:85661; 9:3745; 10:2325; 11:3388; 12:237; 13:242; 14:153; 15:64; 16:243; 17:27; 18:18; 19:1;
Stats: malloc large: 454 small slow: 2937
Shadow byte and word:
  0x1feee29ecfd5: fd
  0x1feee29ecfd0: fd fd fd fd fd fd fd fd
More shadow bytes:
  0x1feee29ecfb0: 00 00 00 00 00 00 00 00
  0x1feee29ecfb8: fb fb fb fb fb fb fb fb
  0x1feee29ecfc0: fa fa fa fa fa fa fa fa
  0x1feee29ecfc8: fa fa fa fa fa fa fa fa
=>0x1feee29ecfd0: fd fd fd fd fd fd fd fd
  0x1feee29ecfd8: fd fd fd fd fd fd fd fd
  0x1feee29ecfe0: fa fa fa fa fa fa fa fa
  0x1feee29ecfe8: fa fa fa fa fa fa fa fa
  0x1feee29ecff0: fd fd fd fd fd fd fd fd

Comment 3

[(no longer active)]

OK, I think I may know what's happening now.

mozSpellChecker::SetCurrentDictionary gets called, and then mozHunspell::SetDictionary gets called <http://mxr.mozilla.org/mozilla-central/source/extensions/spellcheck/src/mozSpellChecker.cpp#353> (which is inlined), which in turn calls into the notification service: <http://mxr.mozilla.org/mozilla-central/source/extensions/spellcheck/hunspell/src/mozHunspell.cpp#176>.  The editor then catches that notification and calls nsEditor::SyncRealTimeSpell, which can potentially lead into mInlineSpellChecker to get set to null <http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/base/nsEditor.cpp#1251>, which in turn releases its mSpellChecker member <http://mxr.mozilla.org/mozilla-central/source/editor/composer/src/nsEditorSpellCheck.h#43>, which is a mozSpellChecker which we see on the 1st frame of the freeing call stack.

Then, all of this stuff returns, and when we get back to the mozSpellChecker::SetCurrentDictionary frame, *this is dead, so any attempt to call it (such as calling Release on it) will dereference freed memory.

Now, I _think_ that you can't put arbitrary stuff on the stack between the time that the mozSpellChecker object dies and the time that mozSpellChecker::SetCurrentDictionary returns, but if I'm wrong, and you could do that, then this gives you a very nice remote exploit, because the offset of Release in the vtable is pretty well known...

Comment 4

[(no longer active)]

Attachment: Patch (v1)

Abhishek, can you please test this patch and see if you can still reproduce the bug?  I think that my analysis should be correct, but this is all code inspection through a few layers of abstraction, so I'm not 100% sure that I got everything right.  Thanks!

Comment 5

[Abhishek Arya]

(In reply to Ehsan Akhgari [:ehsan] from comment #4)
> Created attachment 641142 [details] [diff] [review]
> Patch (v1)
> 
> Abhishek, can you please test this patch and see if you can still reproduce
> the bug?  I think that my analysis should be correct, but this is all code
> inspection through a few layers of abstraction, so I'm not 100% sure that I
> got everything right.  Thanks!

The crash hit twice in the fuzzing, but testcase is not reliable at all. If i get something reproducible, i will update the bug. But i did go through your analysis and that seems to be the way |this| can get blown away. I did predict the same in c#1, but didnt have a good stack to prove that how was that possible since everything was refed :) With the new stack and your analysis explains it that it can come back to set its member var to null in nsEditor::GetInlineSpellChecker.

Comment 6

[(no longer active)]

(In reply to Abhishek Arya from comment #5)
> (In reply to Ehsan Akhgari [:ehsan] from comment #4)
> > Created attachment 641142 [details] [diff] [review]
> > Patch (v1)
> > 
> > Abhishek, can you please test this patch and see if you can still reproduce
> > the bug?  I think that my analysis should be correct, but this is all code
> > inspection through a few layers of abstraction, so I'm not 100% sure that I
> > got everything right.  Thanks!
> 
> The crash hit twice in the fuzzing, but testcase is not reliable at all. If
> i get something reproducible, i will update the bug. But i did go through
> your analysis and that seems to be the way |this| can get blown away. I did
> predict the same in c#1, but didnt have a good stack to prove that how was
> that possible since everything was refed :) With the new stack and your
> analysis explains it that it can come back to set its member var to null in
> nsEditor::GetInlineSpellChecker.

Hmm, did you test again with my patch applied?  If not (I don't know if you build Firefox yourself or if you just use our asan builds) then maybe you can wait until my patch hits m-c (I'll land it in a sec anyways).

Comment 7

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/21c98ae5fc42

Comment 8

[(no longer active)]

CCing Aryeh since he might find this bug interesting (sorry, thought you're already CCed here!)

Comment 9

[Abhishek Arya]

(In reply to Ehsan Akhgari [:ehsan] from comment #6)
> (In reply to Abhishek Arya from comment #5)
> > (In reply to Ehsan Akhgari [:ehsan] from comment #4)
> > > Created attachment 641142 [details] [diff] [review]
> > > Patch (v1)
> > > 
> > > Abhishek, can you please test this patch and see if you can still reproduce
> > > the bug?  I think that my analysis should be correct, but this is all code
> > > inspection through a few layers of abstraction, so I'm not 100% sure that I
> > > got everything right.  Thanks!
> > 
> > The crash hit twice in the fuzzing, but testcase is not reliable at all. If
> > i get something reproducible, i will update the bug. But i did go through
> > your analysis and that seems to be the way |this| can get blown away. I did
> > predict the same in c#1, but didnt have a good stack to prove that how was
> > that possible since everything was refed :) With the new stack and your
> > analysis explains it that it can come back to set its member var to null in
> > nsEditor::GetInlineSpellChecker.
> 
> Hmm, did you test again with my patch applied?  If not (I don't know if you
> build Firefox yourself or if you just use our asan builds) then maybe you
> can wait until my patch hits m-c (I'll land it in a sec anyways).

For fuzzing, i use builds from https://people.mozilla.com/~choller/firefox/asan/ [I used to compile my own optimized build using steps from Christian's wiki - https://developer.mozilla.org/en/Building_Firefox_with_Address_Sanitizer, but his builds seem faster (some trick??), so i switched to using his's]. But in bug reports, i do compile a fully symbolized build for better stacktraces. Once you commit the patch, i can later update the build and see if the fuzzing hits again. I never had a good testcase for this crash, so that is why i cannot test your patch. My fuzzing has only hit it twice in a long time. What i can do is i will update the m-c build after a day or so, and see if this stack comes to haunt again.

Comment 10

[(no longer active)]

Yeah, makes total sense.  I have dealt with a lot of bugs which only happen intermittently, so I totally understand how much of a pain reproducing this entails.  :-)

I'd appreciate if you can keep an eye on this in your future tests.  I think the patch should land on mozilla-central some time tomorrow, which means that it will be in the nightly builds starting from tomorrow night.

Thanks!

Comment 11

[Abhishek Arya]

Sounds good!

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/21c98ae5fc42

Comment 13

[Abhishek Arya]

Sorry, but looks like the bug was not fixed by the guessed patch and the same stack hit again on trunk. Reopening. Like last time, not a reliable crash.

=================================================================
==4059== ERROR: AddressSanitizer heap-use-after-free on address 0x7fd2533941d0 at pc 0x7fd27601b5be bp 0x7ffffc130d50 sp 0x7ffffc130d48
READ of size 4 at 0x7fd2533941d0 thread T0
    #0 0x7fd27601b5be in nsString ../../../dist/include/nsTSubstring.h:618
    #1 0x7fd276594436 in mozInlineSpellChecker::UpdateCurrentDictionary() extensions/spellcheck/src/mozInlineSpellChecker.cpp:1782
    #2 0x7fd27516caef in nsEditorEventListener::Focus(nsIDOMEvent*) editor/libeditor/base/nsEditorEventListener.cpp:907
    #3 0x7fd2751672f8 in nsEditorEventListener::HandleEvent(nsIDOMEvent*) editor/libeditor/base/nsEditorEventListener.cpp:326
    #4 0x7fd2748de54e in nsRefPtr<nsIDOMEventListener>::operator nsIDOMEventListener*() const content/events/src/nsEventListenerManager.cpp:820
    #5 0x7fd27494581d in nsEventTargetChainItem::HandleEvent(nsEventChainPostVisitor&, unsigned int, bool, nsCxPusher*) content/events/src/nsEventListenerManager.h:142
0x7fd2533941d0 is located 80 bytes inside of 96-byte region [0x7fd253394180,0x7fd2533941e0)
freed by thread T0 here:
    #0 0x4248b2 in free 
    #1 0x7fd2760146e9 in nsEditorSpellCheck::Release() editor/composer/src/nsEditorSpellCheck.cpp:193
    #2 0x7fd275138f73 in nsEditor::SyncRealTimeSpell() editor/libeditor/base/nsEditor.cpp:1332
    #3 0x7fd275138d7d in non-virtual thunk to nsEditor::Observe(nsISupports*, char const*, unsigned short const*) ../../../dist/include/nsCOMPtr.h:762
    #4 0x7fd2769cd035 in nsTHashtable<nsObserverList>::GetEntry(char const*) const ../../dist/include/nsTHashtable.h:147
previously allocated by thread T0 here:
    #0 0x424972 in __interceptor_malloc 
    #1 0x7fd2797751a9 in moz_xmalloc memory/mozalloc/mozalloc.cpp:54
==4059== ABORTING
Stats: 197M malloced (188M for red zones) by 369879 calls
Stats: 88M realloced by 14808 calls
Stats: 152M freed by 142463 calls
Stats: 2M really freed by 8142 calls
Stats: 412M (105540 full pages) mmaped in 103 calls
  mmaps   by size class: 8:311277; 9:40955; 10:16380; 11:14329; 12:4096; 13:2048; 14:1536; 15:512; 16:1088; 17:480; 18:160; 19:40; 20:12;
  mallocs by size class: 8:300055; 9:35451; 10:13414; 11:11977; 12:3314; 13:1967; 14:1480; 15:502; 16:1047; 17:468; 18:154; 19:40; 20:10;
  frees   by size class: 8:93249; 9:23684; 10:9954; 11:8965; 12:2292; 13:1065; 14:1303; 15:453; 16:970; 17:454; 18:29; 19:37; 20:8;
  rfrees  by size class: 8:7062; 9:584; 10:268; 11:130; 12:25; 13:22; 14:28; 15:2; 16:17; 17:3; 19:1;
Stats: malloc large: 672 small slow: 2141
Shadow byte and word:
  0x1ffa4a67283a: fd
  0x1ffa4a672838: fd fd fd fd fd fd fd fd
More shadow bytes:
  0x1ffa4a672818: 00 00 00 00 00 00 00 fb
  0x1ffa4a672820: fa fa fa fa fa fa fa fa
  0x1ffa4a672828: fa fa fa fa fa fa fa fa
  0x1ffa4a672830: fd fd fd fd fd fd fd fd
=>0x1ffa4a672838: fd fd fd fd fd fd fd fd
  0x1ffa4a672840: fa fa fa fa fa fa fa fa
  0x1ffa4a672848: fa fa fa fa fa fa fa fa
  0x1ffa4a672850: 00 00 00 fb fb fb fb fb
  0x1ffa4a672858: fb fb fb fb fb fb fb fb

Comment 14

[Abhishek Arya]

Ah! the stack changed after the fix [see free stack #1 0x7fd2760146e9 in nsEditorSpellCheck::Release]. Looks like nsEditorSpellCheck |this| object is stale too.

Comment 15

[(no longer active)]

(In reply to Abhishek Arya from comment #14)
> Ah! the stack changed after the fix [see free stack #1 0x7fd2760146e9 in
> nsEditorSpellCheck::Release]. Looks like nsEditorSpellCheck |this| object is
> stale too.

Right...  Should've seen that coming!

Comment 16

[(no longer active)]

Attachment: Part 2

Comment 17

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/33bece9dee65

Comment 18

[Abhishek Arya]

as i am seeing the call stack in c#0, shouldn't nsEditorSpellCheck be protected in nsEditorSpellCheck::UpdateCurrentDictionary ? The recent fix in nsEditorSpellCheck::GetCurrentDictionary confuses me since it is in a get command that usually shouldn't do modifications.

    #0 0x7fd83187223d in mozSpellChecker::SetCurrentDictionary(nsAString_internal const&) extensions/spellcheck/src/mozSpellChecker.cpp:357
    #1 0x7fd83133d55d in nsEditorSpellCheck::SetCurrentDictionary(nsAString_internal const&) editor/composer/src/nsEditorSpellCheck.cpp:523
    #2 0x7fd83133ede7 in nsEditorSpellCheck::UpdateCurrentDictionary() editor/composer/src/nsEditorSpellCheck.cpp:651
    #3 0x7fd83188b2d6 in mozInlineSpellChecker::UpdateCurrentDictionary() extensions/spellcheck/src/mozInlineSpellChecker.cpp:1783

Comment 19

[(no longer active)]

Attachment: Part 3

Argh, you're completely right.  Although I'd prefer to do this both in SetCurrentDictionary and UpdateCurrentDictionary, as the former can be called without the latter being on the stack...

Comment 20

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1dfcb36d7c17

Comment 21

[Ed Morley [:emorley]]

Parts 2 + 3:
https://hg.mozilla.org/mozilla-central/rev/33bece9dee65
https://hg.mozilla.org/mozilla-central/rev/1dfcb36d7c17

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 641142 [details] [diff] [review]
Patch (v1)

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

We should take this security fix on Aurora and Beta.

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 642856 [details] [diff] [review]
Part 2

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

We should take this security fix on Aurora and Beta.

Comment 24

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 643240 [details] [diff] [review]
Part 3

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

We should take this security fix on Aurora and Beta.

Comment 25

[Daniel Veditz [:dveditz]]

I'll assume ESR-10 is affected as well.

Comment 26

[Alex Keybl [:akeybl]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #22)
> Comment on attachment 641142 [details] [diff] [review]
> Patch (v1)
> 
> Review of attachment 641142 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> We should take this security fix on Aurora and Beta.

Would you mind providing a risk assessment?

Comment 27

[(no longer active)]

(In reply to Alex Keybl [:akeybl] from comment #26)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #22)
> > Comment on attachment 641142 [details] [diff] [review]
> > Patch (v1)
> > 
> > Review of attachment 641142 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > We should take this security fix on Aurora and Beta.
> 
> Would you mind providing a risk assessment?

The risk is very very low.  We're just adding a bunch of more reference counts to a couple of variables.  This is almost as safe as a security fix can get!

Comment 28

[Alex Keybl [:akeybl]]

Comment on attachment 641142 [details] [diff] [review]
Patch (v1)

[Triage Comment]
Thanks Ehsan, approving for all branches in that case.

Comment 29

[(no longer active)]

http://hg.mozilla.org/releases/mozilla-aurora/rev/2ae7d777b815
http://hg.mozilla.org/releases/mozilla-beta/rev/12d5f596e431

Comment 30

[Abhishek Arya]

Just a fyi, i haven't seen this crash happen again.

Comment 31

[(no longer active)]

(In reply to Abhishek Arya from comment #30)
> Just a fyi, i haven't seen this crash happen again.

Great!  Hard to say if that actually means that this is fixed, but for now, \o/!

Comment 32

[(no longer active)]

Comment on attachment 641142 [details] [diff] [review]
Patch (v1)

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: sec-critical
Fix Landed on Version: Nightly, Aurora, Beta
Risk to taking this patch (and alternatives if risky): This has very minimal risk
String or UUID changes made by this patch: none

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 33

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 643240 [details] [diff] [review]
Part 3

Low risk, needed on ESR to match 15 release - approving.

Comment 34

[(no longer active)]

http://hg.mozilla.org/releases/mozilla-esr10/rev/a92f905cd755

Comment 35

[u279076]

qa- until testcase-wanted can be satisfied.