448165 - Mailnews crashes in [@ nsAbMDBDirectory::DeleteCards] if aCards is null

Status: VERIFIED FIXED

(MailNews Core :: Address Book, defect)

Description

[Josh Geenen (:pi)]

Attachment: Code to demonstrate the crash

nsAbMDBDirectory::DeleteCards doesn't check if aCards is null, so it is possible to crash Mailnews by passing null to the DeleteCards method of an MDB directory.

It crashes on line 499 (trunk):
http://mxr.mozilla.org/mailnews/source/mailnews/addrbook/src/nsAbMDBDirectory.cpp#499

Or line 476 for Thunderbird 2
http://mxr.mozilla.org/mozilla1.8.0/source/mailnews/addrbook/src/nsAbMDBDirectory.cpp#476

The attachment has a line of code that will crash Thunderbird 3 and another line for Thunderbird 2, if run in the Error Console.

Patch and unit test coming soon.

Comment 1

[Josh Geenen (:pi)]

Attachment: Patch with unit test

This is a possible patch for the trunk with a unit test.  I am not requesting review yet until I have a chance to try it in Windows.

The patch just adds NS_ENSURE_ARG_POINTER(aCards); to the DeleteCards method.

Comment 2

[Josh Geenen (:pi)]

I posted the wrong link under Thunderbird 2 in the first post (thanks sid0)
It should be:
http://mxr.mozilla.org/mozilla1.8/source/mailnews/addrbook/src/nsAbMDBDirectory.cpp#476
and the parameter is "cards" in Thunderbird 2.

Comment 3

[Josh Geenen (:pi)]

Comment on attachment 331460 [details] [diff] [review]
Patch with unit test

I tested the patch and unit test in Windows.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Why this was checked-in and never marked as fixed? Even no link to the changeset?

Comment 5

[Mark Banner (:standard8)]

(In reply to comment #4)
> Why this was checked-in and never marked as fixed? Even no link to the
> changeset?

Opps, must have forgotten it. The changeset was 46:427e30ec8458, pushed on the 30th July.

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Mark, any plans to get the patch into 1.8 branch? As I read the crash also happens on that branch.

Comment 7

[Mark Banner (:standard8)]

(In reply to comment #6)
> Mark, any plans to get the patch into 1.8 branch? As I read the crash also
> happens on that branch.

If there's crashes in talkback that point directly to this, then yes I would consider it. However I think it unlikely as this is more likely to be picked up in extension dev. Therefore I think unless we have proof its needed, its not worth spending the time on.

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Ok, so lets set the version to 1.8 to keep track.

Verified so far with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b1pre) Gecko/20080903025653 Shredder/3.0b1pre

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1a2pre) Gecko/20080823031107 Shredder/3.0b1pre