225578 - crash [@ nsAbView::GetSelectedAddresses(nsIArray**)] does not check for NULL pointer

Status: RESOLVED FIXED

(MailNews Core :: Address Book, defect)

Description

[Stevie-O]

User-Agent:       Opera/7.20 (Windows NT 5.0; U)  [en]
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.5) Gecko/20031007

nsAbView::GetSelectedAddresses calls GetSelectedCards() to get a list of the 
selected address cards.  However, if none are selected, GetSelectedCards() 
returns a NULL pointer while reporting success. GetSelectedAddresses then tries 
to dereference selectedCards at nsAbView.cpp:1300, without first ensuring that 
selectedCards is not NULL:

selectedCards->Count(&count);


Reproducible: Always

Steps to Reproduce:
1. Open the Address Book.
Make sure you don't have a broken install -- you should have at least two books 
(Personal Address Book and Collected Addresses).
2. Click on one of the address books.
3. Open Venkman.
3a. If necessary, uncheck 'Exclude Browser Files' and check 'Show/Hide>Loaded 
Scripts".
4. Go to the SetStatusText function in addressbook.js (easily accessed via the 
Loaded Scripts pane)
5. Place a breakpoint on the first statement in that function (it's an "if" 
statement).
6. Go back to the Address Books window and click on a different address book.  
Venkman should pop up, halted at the line breakpointed in step #5.
7. In the 'Interactive Session' pane at the bottom, simply type 'gAbView' and 
press Enter.

Actual Results:  
Mozilla crashed with a NULL-pointer dereference.

Expected Results:  
I did not know what to expect (I was experimenting).  I can assure you that I 
was not expecting Mozilla to crash.

Comment 1

[Stevie-O]

Attachment: modifies nsAbView::GetSelectedAddresses to test for a NULL object before dying

Comment 2

[Stevie-O]

Attachment: modifies nsAbView::GetSelectedAddresses to test for a NULL object before dying, without using tabs

ewps

Comment 3

[Stevie-O]

Attachment: modifies nsAbView::GetSelectedAddresses to test for a NULL object before dying, without using tables, AND fixes another bug

gah. OK, that's enough revisions.

This version adds a bugfix timeless asked for (it checks for failure to
allocate the 'addresses' object).

Comment 4

[Joe Infla]

confirming crash

Comment 5

[Wayne Mery (:wsmwk)]

patch going on 5yr old and never reviewed - obsolete?

Comment 6

[Mark Banner (:standard8)]

(In reply to comment #5)
> patch going on 5yr old and never reviewed - obsolete?
> 
The bug is still valid, though the patch obsolete. I've a feeling we don't actually hit this code (currently), which is why we never have a problem with crashes.

Comment 7

[Wayne Mery (:wsmwk)]

assuming nsAbView::GetSelectedAddresses(nsIArray**) is the same, rare = 8 crashes in 10 months for 3.0x.  various OS, so at least 3-4 people

3.0b4 variation
bp-c2ccfadc-1195-4e3d-925b-2c7712091018
Frame	Module	Signature [Expand]	Source
0	thunderbird.exe	nsAbView::GetSelectedAddresses	mailnews/addrbook/src/nsAbView.cpp:1314
1	xpcom_core.dll	NS_InvokeByIndex_P	xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp:101
2	xpcom_core.dll	xptiInterfaceInfo::GetMethodInfo	xpcom/reflect/xptinfo/src/xptiprivate.h:706

Comment 8

[Wayne Mery (:wsmwk)]

add missing ] so bug# shows in crash-stats

Comment 9

[Mark Banner (:standard8)]

Attachment: The fix

Proposed fix - as GetSelectedAddresses is an internal function, make the argument an nsCOMPtr<nsIMutableArray> and pass that directly. This skips the need for pointers.

As a result had to change one function to bail out early if there's nothing selected. Tested various functions including selecting ABs, composing emails with addresses selected and such.

Venkman is failing in debug mode, so I'm currently doing a release build to fully verify the fix.

Comment 10

[David :Bienvenu]

Comment on attachment 428727 [details] [diff] [review]
The fix

seems fine...tested on a windows debug build.

Comment 11

[Mark Banner (:standard8)]

Checked in: http://hg.mozilla.org/comm-central/rev/aeb07d0521aa

and to the 3.1a1 branch for purposes of the test firedrill: http://hg.mozilla.org/comm-central/rev/c1bb767189c1