Last Comment Bug 413544 - add null-arg checks in nsMsgMailSession.cpp, nsMimeBaseEmitter.cpp, nsMsgCompose.cpp and nsMsgAccount.cpp
: add null-arg checks in nsMsgMailSession.cpp, nsMimeBaseEmitter.cpp, nsMsgComp...
Status: RESOLVED FIXED
: crash
Product: MailNews Core
Classification: Components
Component: Backend (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Thunderbird 16.0
Assigned To: :aceman
:
Mentors:
Depends on:
Blocks: 412109
  Show dependency treegraph
 
Reported: 2008-01-22 13:04 PST by Joey Minta
Modified: 2012-06-11 16:14 PDT (History)
6 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch v1 (5.01 KB, patch)
2008-01-22 13:04 PST, Joey Minta
standard8: review-
Details | Diff | Review
updated patch with comment 1 addressed and other enhancements (18.17 KB, patch)
2011-12-02 15:34 PST, :aceman
no flags Details | Diff | Review
v2, updated patch with comment 1 addressed and other enhancements (23.14 KB, patch)
2011-12-06 13:48 PST, :aceman
standard8: review-
Details | Diff | Review
v3, with fixes per comment 6 (22.47 KB, patch)
2012-01-03 14:24 PST, :aceman
standard8: review+
Details | Diff | Review
patch v4 (23.39 KB, patch)
2012-06-11 13:13 PDT, :aceman
acelists: review+
Details | Diff | Review

Description Joey Minta 2008-01-22 13:04:19 PST
Created attachment 298527 [details] [diff] [review]
patch v1

A bunch more null-arg checks.  I've run into some weird crashes in nsMimeBaseEmitter that i've started marking as known failures.
Comment 1 Mark Banner (:standard8) 2008-01-24 04:58:29 PST
Comment on attachment 298527 [details] [diff] [review]
patch v1

   PRInt32 numTasks = mShutdownTasks.Count();
-  mMsgProgress->OnProgressChange(nsnull, nsnull, 0, 0, mTaskIndex, numTasks);
+  if (mMsgProgress)
+    mMsgProgress->OnProgressChange(nsnull, nsnull, 0, 0, mTaskIndex, numTasks);

There's not really much point in getting numTasks if we've not got a mMsgProgress.

   nsString statusString(inStatusString);
-  mMsgProgress->OnStatusChange(nsnull, nsnull, NS_OK, statusString.get());
+  if (mMsgProgress)
+    mMsgProgress->OnStatusChange(nsnull, nsnull, NS_OK, statusString.get());

Not you fault, but let's inline this whilst were here:

nsString(statusString).get()

+  NS_ENSURE_ARG_POINTER(buf);
   PRInt32     tmpLen = strlen(buf);

nit: blank line here please.

nsresult rv;
 
-  rv = mOutStream->Write(buf, count, countWritten);
+  if (mOutStream)
+    rv = mOutStream->Write(buf, count, countWritten);
+  else
+    rv = NS_ERROR_NOT_INITIALIZED;
+

This would be much better as:

nsresult rv = mOutStream ? mOutStream->Write(...) : NS_ERROR...;

+  NS_ENSURE_ARG_POINTER(outCharset);
   mDocHeader = rootMailHeader;

nit: blank line please.
Comment 2 Gary Kwong [:gkw] [:nth10sd] 2010-08-06 08:44:01 PDT
Comment on attachment 298527 [details] [diff] [review]
patch v1

Patch has obsoleted (along with r-):

$ patch -p1 --dry-run < ~/Desktop/p413544.diff 
patching file mailnews/base/src/nsMsgAccount.cpp
Hunk #1 succeeded at 325 (offset 34 lines).
patching file mailnews/base/src/nsMsgMailSession.cpp
Hunk #1 succeeded at 518 (offset 104 lines).
Hunk #2 succeeded at 603 (offset 116 lines).
Hunk #3 succeeded at 769 (offset 172 lines).
Hunk #4 succeeded at 797 (offset 173 lines).
patching file mailnews/compose/src/nsMsgCompose.cpp
Hunk #1 succeeded at 889 with fuzz 2 (offset 19 lines).
Hunk #2 succeeded at 941 with fuzz 1 (offset 2 lines).
Hunk #3 succeeded at 3035 (offset 186 lines).
Hunk #4 succeeded at 4437 with fuzz 2 (offset 249 lines).
patching file mailnews/mime/emitters/src/nsMimeBaseEmitter.cpp
Hunk #1 succeeded at 378 (offset 1 line).
Hunk #2 FAILED at 435.
Hunk #3 succeeded at 519 (offset 7 lines).
Hunk #4 succeeded at 579 (offset 7 lines).
1 out of 4 hunks FAILED -- saving rejects to file mailnews/mime/emitters/src/nsMimeBaseEmitter.cpp.rej
Comment 3 :aceman 2011-12-02 05:26:13 PST
Joey, can I pick up this patch?
Comment 4 :aceman 2011-12-02 15:34:27 PST
Created attachment 578754 [details] [diff] [review]
updated patch with comment 1 addressed and other enhancements
Comment 5 :aceman 2011-12-06 13:48:01 PST
Created attachment 579446 [details] [diff] [review]
v2, updated patch with comment 1 addressed and other enhancements

Sorry, the previous version broke tests.
Comment 6 Mark Banner (:standard8) 2012-01-03 12:52:48 PST
Comment on attachment 579446 [details] [diff] [review]
v2, updated patch with comment 1 addressed and other enhancements

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

I've looked at some of this in a bit more detail this time and spotted a few issues. You'll also need to updated the patch for the PR_FALSE -> false (etc) transition.

::: mailnews/base/src/nsMsgMailSession.cpp
@@ +469,4 @@
>  
> +  bool baseDirExists = false;
> +  nsresult rv = defaultsDir->Exists(&baseDirExists);
> +  NS_ENSURE_SUCCESS(rv,rv);

nit: as you're touching this line, please put a space after the comma, ditto throughout the rest of the function.

::: mailnews/compose/src/nsMsgCompose.cpp
@@ +965,5 @@
>  
>  nsresult nsMsgCompose::_SendMsg(MSG_DeliverMode deliverMode, nsIMsgIdentity *identity, 
>                                  const char *accountKey, bool entityConversionDone)
>  {
> +  NS_ENSURE_TRUE(m_compFields, NS_ERROR_NOT_INITIALIZED);

The more I look at this, the more I don't quite like this change.

_SendMsg is an internal function to this class, so we can choose to take null values if we want.

When you look at the if statement this was in:

if (m_compFields && identity)

then if either of those are null, rv is set to NS_ERROR_NOT_INITIALIZED, but there's then an extra notification that gets done at the end of the function - so there may be an expectation of that notification, which doing this NS_ENSURE_TRUE would skip.

However, skipping up the stack a level, the only caller is SendMsg. AFAICT in SendMsg if m_compFields is null, then nothing really happens - you just end up prompting the user that the message send failed, but not telling why.

It also looks like (in SendMsg again) that if identity is null, the same thing happens as if m_compFields is null.

Therefore, I think the right place to check for m_compFields and identity being null is at the start of nsMsgCompose::SendMsg - the xpcom interface. We should also update the idl documentation to state that the nsIMsgCompose instance should have been initialized (with the Initialized call), and that identity must be specified.

::: mailnews/mime/emitters/src/nsMimeBaseEmitter.cpp
@@ +376,5 @@
>  
>  NS_IMETHODIMP
>  nsMimeBaseEmitter::GetOutputListener(nsIStreamListener **listener)
>  {
> +  if (*listener)

Nope, this should just be listener - we're just assigning the value to it.

I think we should probably switch this to NS_ENSURE_ARG_POINTER.

@@ +518,5 @@
>  
>  nsresult
>  nsMimeBaseEmitter::WriteHelper(const char *buf, PRUint32 count, PRUint32 *countWritten)
>  {
> +  nsresult rv = mOutStream ? mOutStream->Write(buf, count, countWritten) : NS_ERROR_NOT_INITIALIZED;

Ok, now I've seen the full part of this function I've changed my mind.

Lets go with ensuring mOutStream and the result of the Write going directly to nsresult rv.
Comment 7 :aceman 2012-01-03 14:24:54 PST
Created attachment 585556 [details] [diff] [review]
v3, with fixes per comment 6

I tried to fix according to your comments.
There were some changes of 'if (!cond) return ERR' into 'NS_ENSURE_TRUE()' (in the original file but also after the patch you reviewed). Could you please thoroughly check the changes do not change the meaning (like negated condition)? I think there were some wrong ones in my previous patch that I now fixed.

I've reverted the changes in _SendMsg.
Comment 8 Mark Banner (:standard8) 2012-01-19 05:53:15 PST
Comment on attachment 585556 [details] [diff] [review]
v3, with fixes per comment 6

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

r=me with the comments below fixed.

::: mailnews/base/src/nsMsgMailSession.cpp
@@ +414,5 @@
>  }
>  
>  NS_IMETHODIMP nsMsgMailSession::IsFolderOpenInWindow(nsIMsgFolder *folder, bool *aResult)
>  {
> +  NS_ENSURE_TRUE(aResult, NS_ERROR_NULL_POINTER);

This can just be NS_ENSURE_ARG_POINTER

::: mailnews/mime/emitters/src/nsMimeBaseEmitter.cpp
@@ +379,5 @@
>  {
> +  NS_ENSURE_ARG_POINTER (listener);
> +
> +  *listener = mOutListener;
> +  NS_IF_ADDREF(*listener);

nit: we can make these lines:

NS_IF_ADDREF(*listener = mOutListener);
Comment 9 :aceman 2012-01-19 06:35:43 PST
(In reply to Mark Banner (:standard8) from comment #8)
> Comment on attachment 585556 [details] [diff] [review]
> > +  NS_ENSURE_TRUE(aResult, NS_ERROR_NULL_POINTER);
> 
> This can just be NS_ENSURE_ARG_POINTER

That would return NS_ERROR_INVALID_POINTER in case of failure. Would the callers be happy with that change?

This is a general question. Do any callers check for the exact error value returned from these interface functions and then act on it in different ways? For now I assumed this is the case therefore I generally try not to touch the returned values.

Or do the callers always just ignore any error and proceed or return the error/trow exception to the parent caller? So that the exact error value is not relevant?
Comment 10 Mark Banner (:standard8) 2012-01-19 12:20:31 PST
(In reply to :aceman from comment #9)
> (In reply to Mark Banner (:standard8) from comment #8)
> > Comment on attachment 585556 [details] [diff] [review]
> > > +  NS_ENSURE_TRUE(aResult, NS_ERROR_NULL_POINTER);
> > 
> > This can just be NS_ENSURE_ARG_POINTER
> 
> That would return NS_ERROR_INVALID_POINTER in case of failure. Would the
> callers be happy with that change?

Yes that should be fine. We don't generally make promises about which errors will be returned unless they are specific to something (e.g. file not found, or file already open).

> This is a general question. Do any callers check for the exact error value
> returned from these interface functions and then act on it in different
> ways? For now I assumed this is the case therefore I generally try not to
> touch the returned values.

For the majority of callers, most of them just care if it is success or failure (some don't check). However we do have calls where we check for specifics, e.g. here's one:

http://mxr.mozilla.org/comm-central/ident?i=NS_MSG_ERROR_FOLDER_SUMMARY_OUT_OF_DATE

> Or do the callers always just ignore any error and proceed or return the
> error/trow exception to the parent caller? So that the exact error value is
> not relevant?

For things that are really true error conditions, e.g. null pointers & unintialised, we probably don't generally care. Its the less generic, maybe expected ones that we do care about a bit more. Generally doing a quick search in mxr for the callers should give you an idea.
Comment 11 :aceman 2012-01-20 13:35:49 PST
(In reply to Mark Banner (:standard8) from comment #6)
> ::: mailnews/mime/emitters/src/nsMimeBaseEmitter.cpp
> @@ +376,5 @@
> >  
> >  NS_IMETHODIMP
> >  nsMimeBaseEmitter::GetOutputListener(nsIStreamListener **listener)
> >  {
> > +  if (*listener)
> 
> Nope, this should just be listener - we're just assigning the value to it.
> 
> I think we should probably switch this to NS_ENSURE_ARG_POINTER.

Now when I look at that function, adding NS_ENSURE_ARG_POINTER(listener) would change the behavior of that function. It was returning NS_OK even if listener was null, it just didn't assign anything to it. With the change it would return error. Is it really OK?
Comment 12 :aceman 2012-04-10 15:30:22 PDT
standard8?
Comment 13 :aceman 2012-06-10 13:41:04 PDT
This got forgotten by me. Standard8, can we finish it in some way?
Comment 14 Mark Banner (:standard8) 2012-06-11 07:11:25 PDT
(In reply to :aceman from comment #11)
> (In reply to Mark Banner (:standard8) from comment #6)
> > ::: mailnews/mime/emitters/src/nsMimeBaseEmitter.cpp
> > @@ +376,5 @@
> > >  
> > >  NS_IMETHODIMP
> > >  nsMimeBaseEmitter::GetOutputListener(nsIStreamListener **listener)
> > >  {
> > > +  if (*listener)
> > 
> > Nope, this should just be listener - we're just assigning the value to it.
> > 
> > I think we should probably switch this to NS_ENSURE_ARG_POINTER.
> 
> Now when I look at that function, adding NS_ENSURE_ARG_POINTER(listener)
> would change the behavior of that function. It was returning NS_OK even if
> listener was null, it just didn't assign anything to it. With the change it
> would return error. Is it really OK?

Yes, I think that is fine. We don't use it from c++ internally afaict, but if someone is calling the function to get a value, but not giving us a pointer to get that value, I think they deserve an error back.
Comment 15 :aceman 2012-06-11 13:13:08 PDT
Created attachment 631999 [details] [diff] [review]
patch v4

Fixes from comment 8 + bitrot.
Comment 16 Ryan VanderMeulen [:RyanVM] 2012-06-11 16:14:49 PDT
https://hg.mozilla.org/comm-central/rev/b7d246591716

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