55003 - nsOutputStream.close() doesn't check for NULL

Status: VERIFIED FIXED

(Core :: XPCOM, defect, P3)

Description

[A. Craig West]

As it says in the summary, nsOutputStream.close() doesn't check if it has a null 
stream before trying to close the underlying stream. In the case of 
nsOutputFileStream, this can happen if you try to create a file with an invalid 
filename. I originally filed this as bug#5444, but after a few brain-dead 
instances on my part, I figured out what was wrong. The fix should be a simple
matter of inserting:
if (mOutputStream)

before the call to:
mOutputStream->Close();

in nsFileStream.h line 246

To duplicate the crash reproducably, delete the directory which contains the 
folder which your user pref:
mail.identity.id1.fcc_folder
points to. Then (without starting the mail/News client, which I believe rebuilds 
these folders) send a page to somebody. When the browser tries to save a copy of 
the message into a folder in a non-existent directory, it will crash.
Admittedly, this is not a normal usage case, but it is a 1-line fix for a 
gp-fault, and therefore worthwhile...

stack trace of crash:
nsOutputStream::close() line 246 + 17 bytes
nsLocalMailCopyState::~nsLocalMailCopyState() line 382
nsLocalMailCopyState::`scalar deleting destructor'(unsigned int 0x00000001) + 15 
bytes
nsMsgLocalMailFolder::ClearCopyState() line 1843 + 36 bytes
nsMsgLocalMailFolder::EndCopy(nsMsgLocalMailFolder * const 0x0255bf74, int 
0x00000001) line 2439
nsMsgLocalMailFolder::CopyFileMessage(nsMsgLocalMailFolder * const 0x0255becc, 
nsIFileSpec * 0x03eb7d30, nsIMessage * 0x00000000, int 0x00000000, nsIMsgWindow 
* 0x00000000, nsIMsgCopyServiceListener * 0x03ebd4c0) line 2010 + 23 bytes
nsMsgCopyService::DoNextCopy() line 246 + 78 bytes
nsMsgCopyService::DoCopy(nsCopyRequest * 0x03ebd460) line 174 + 8 bytes
nsMsgCopyService::CopyFileMessage(nsMsgCopyService * const 0x03ebe100, 
nsIFileSpec * 0x03eb7d30, nsIMsgFolder * 0x0255becc, nsIMessage * 0x00000000, 
int 0x00000000, nsIMsgCopyServiceListener * 0x03ebd4c0, nsIMsgWindow * 
0x00000000) line 417 + 12 bytes
nsMsgCopy::DoCopy(nsIFileSpec * 0x03eb7d30, nsIMsgFolder * 0x0255becc, 
nsIMessage * 0x00000000, int 0x00000000, nsIMsgWindow * 0x00000000, 
nsMsgComposeAndSend * 0x03ed5b60) line 305 + 67 bytes
nsMsgCopy::StartCopyOperation(nsIMsgIdentity * 0x03cc9de0, nsIFileSpec * 
0x03eb7d30, int 0x00000000, nsMsgComposeAndSend * 0x03ed5b60, const char * 
0x0012f864, nsIMessage * 0x00000000) line 236 + 35 bytes
nsMsgComposeAndSend::StartMessageCopyOperation(nsIFileSpec * 0x03eb7d30, int 
0x00000000, char * 0x03eb55b0) line 4174 + 57 bytes
nsMsgComposeAndSend::MimeDoFCC(nsFileSpec * 0x03e9c240, int 0x00000000, const 
char * 0x03b6f90c, const char * 0x03ed5340, const char * 0x03ed5420) line 4142 + 
29 bytes
nsMsgComposeAndSend::DoFcc() line 3114 + 74 bytes
nsMsgComposeAndSend::DoDeliveryExitProcessing(nsIURI * 0x03ec9a84, unsigned int 
0x00000000, int 0x00000000) line 3055 + 8 bytes
nsMsgComposeAndSend::DeliverAsMailExit(nsIURI * 0x03ec9a84, unsigned int 
0x00000000) line 3076
MailDeliveryCallback(nsIURI * 0x03ec9a84, unsigned int 0x00000000, void * 
0x03ed5b60) line 2639
nsMsgDeliveryListener::OnStopRunningUrl(nsMsgDeliveryListener * const 
0x03ecaea0, nsIURI * 0x03ec9a84, unsigned int 0x00000000) line 82 + 21 bytes
nsUrlListenerManager::BroadcastChange(nsIURI * 0x03ec9a84, nsUrlNotifyType 
nsUrlNotifyStopRunning, unsigned int 0x00000000) line 97
nsUrlListenerManager::OnStopRunningUrl(nsUrlListenerManager * const 0x03ec9970, 
nsIMsgMailNewsUrl * 0x03ec9a84, unsigned int 0x00000000) line 110 + 18 bytes
nsMsgMailNewsUrl::SetUrlState(nsMsgMailNewsUrl * const 0x03ec9a84, int 
0x00000000, unsigned int 0x00000000) line 107
nsSmtpProtocol::ProcessProtocolState(nsIURI * 0x03ec9a84, nsIInputStream * 
0x03ec8c30, unsigned int 0x000000cf, unsigned int 0x0000000b) line 1526
nsMsgProtocol::OnDataAvailable(nsMsgProtocol * const 0x03ec95a0, nsIChannel * 
0x03ec9264, nsISupports * 0x03ec9a84, nsIInputStream * 0x03ec8c30, unsigned int 
0x000000cf, unsigned int 0x0000000b) line 192 + 32 bytes
nsOnDataAvailableEvent::HandleEvent(nsOnDataAvailableEvent * const 0x03e9c070) 
line 400 + 47 bytes
nsStreamListenerEvent::HandlePLEvent(PLEvent * 0x03e9c470) line 97 + 12 bytes
PL_HandleEvent(PLEvent * 0x03e9c470) line 575 + 10 bytes
PL_ProcessPendingEvents(PLEventQueue * 0x00ac1b80) line 508 + 9 bytes
_md_EventReceiverProc(HWND__ * 0x002803dc, unsigned int 0x0000c0b7, unsigned int 
0x00000000, long 0x00ac1b80) line 1044 + 9 bytes
USER32! 77e71820()
00ac1b80()

Comment 1

[David :Bienvenu]

I can confirm this bug. I can also nominate it for RTM. Feel free to shoot it
down, because I can fix this in the mail/news code, but I think it would be a
good thing to protect against in the xpcom code as well.

Comment 2

[David :Bienvenu]

adding self to cc list.

Comment 3

[David :Bienvenu]

Attachment: proposed fix

Comment 4

[Warren Harris]

Edward: Welcome to xpcom!

Comment 5

[Warren Harris]

Once again... attempting to reassign from Ray to Edward.

Comment 6

[selmer (gone)]

Edward, we would like this fix on the N6 branch.  Could you get r=/sr= and then
put [rtm+] in the status whiteboard.  I suspect this will get approved easily
and then it could be checked in.

Comment 7

[selmer (gone)]

David, since you put in the patch can you usher this through reviews?  It's
languishing instead of getting into a candidate build like it should.

Comment 8

[David :Bienvenu]

Steve, I fixed this in the mail code a long time ago, so this doesn't have to be
fixed in the xpcom code (other than for correctness). I know of no other crashes
besides the one I fixed in the mail code. Reassigning back to
[e-mail] - if you know of an actual instance of this crash, I'd be
happy to usher it through the review process, but if not, we might as well leave
it for later.

Comment 9

[Chris Waterson]

The XPCOM patch looks fine [r,sr]=waterson, pick one.

Comment 10

[Edward Kandrot]

Sent it to super reviews.  r=kandrot, sr=waterson  Now it needs to be ++'ed.

Comment 11

[Jim Roskind]

Moving to rtm candidate limbo.
Please land on the *trunk* when possible (though I have no real fears of
regression).
Anticipate that on Monday, we'll probably mark this double-plus, and you should
try to get it landed on the branch *when* you see that happen.

Comment 12

[selmer (gone)]

This bug is in candidate limbo.  We will reconsider this fix once we have a
candidate in hand, but we can't take this fix before then.

Comment 13

[David :Bienvenu]

OK, fix checked in on trunk.

Comment 14

[Phil Peterson]

PDT marking [rtm++]. This bug is now out of limbo and approved for checkin to
the branch. Please check in ASAP.

Comment 15

[David :Bienvenu]

fix checked in.

Comment 16

[jefft]

Verified both on branch and trunk.