1282087 - (coverity) dereference before null check: mailnews/imap/src/nsImapMailFolder.cpp: dereferencing of |msgIdString| occurs before null check

Status: RESOLVED FIXED

(MailNews Core :: Networking: IMAP, defect)

Description

[ISHIKAWA, Chiaki]

Coverity found this:

*msgIdString is done before the null check.

4971NS_IMETHODIMP
4972nsImapMailFolder::NotifyMessageDeleted(const char * onlineFolderName, bool deleteAllMsgs, const char * msgIdString)
4973{
4974  if (deleteAllMsgs)
4975    return NS_OK;
4976
4977  nsTArray<nsMsgKey> affectedMessages;
    deref_ptr_in_call: Dereferencing pointer msgIdString. [show details]
4978  ParseUidString(msgIdString, affectedMessages);
4979
    CID 1137515 (#1 of 1): Dereference before null check (REVERSE_INULL)check_after_deref: Null-checking msgIdString suggests that it may be null, but it has already been dereferenced on all paths leading to the check.
4980  if (msgIdString && !ShowDeletedMessages())
4981  {
4982    GetDatabase();
4983    NS_ENSURE_TRUE(mDatabase, NS_OK);
4984    if (!ShowDeletedMessages())
4985    {
4986      if (!affectedMessages.IsEmpty()) // perhaps Search deleted these messages
4987      {
4988        DeleteStoreMessages(affectedMessages);
4989        mDatabase->DeleteMessages(affectedMessages.Length(), affectedMessages.Elements(), nullptr);
4990      }
4991    }
4992    else // && !imapDeleteIsMoveToTrash
4993      SetIMAPDeletedFlag(mDatabase, affectedMessages, false);
4994  }
4995  return NS_OK;
4996}
4997

We can probably place at the beginning of a mozilla-equivalent of assert(msgIdString)  and remove the null check.

Comment 1

[:aceman]

What about adding a null check to ParseUidString() ? Yes, it seems to *uidString without checking it. It could exit cleanly (no keys added) with a null pointer.

Also here we could add "if (!msgIdString) return NS_OK;" into line 4976.

Also it looks like we get value of ShowDeletedMessages() twice. Or does it have sideeffects so that we have to call it twice?

Comment 2

[ISHIKAWA, Chiaki]

(In reply to :aceman from comment #1)
> What about adding a null check to ParseUidString() ? Yes, it seems to
> *uidString without checking it. It could exit cleanly (no keys added) with a
> null pointer.
> 
> Also here we could add "if (!msgIdString) return NS_OK;" into line 4976.
> 
> Also it looks like we get value of ShowDeletedMessages() twice. Or does it
> have sideeffects so that we have to call it twice?

Thank you very much for your comment.

aceman, would you like to create a patch?

I hoped that by posting the trivial bugs from coverity reports will encourage people to post patches in droves. 
Well, obviously it did not happen :-) 
Not that I expected it to happen very much.

I am tied up this week with my day time office work, and so you have to wait for my patch.

Comment 3

[:aceman]

Attachment: patch

So this contains my suggestions. I move SetIMAPDeletedFlag so another place which may change behaviour. But otherwise I do not understand when SetIMAPDeletedFlag would be called. It relied on ShowDeletedMessages() being true when we already are in a block that says ShowDeletedMessages() is false. I do not think GetDatabase() may change the result of ShowDeletedMessages().

Comment 4

[Kent James (:rkent)]

Comment on attachment 8771671 [details] [diff] [review]
patch

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

I know you've heard this from me before, and you are probably tired of hearing about it, but it does take time to review all of the extra changes that you are doing here that have nothing to do with the actual bug. The original bug is quite trivial, all you need to do is move the null check earlier. In fact this is almost certainly not a real issue, as we would be getting crashes in ParseUidString if it was. So we are doing this mostly to keep Coverity happy. I think we would be better served by trying to make these Coverity fixes as basic as possible so that they can be done easily.

If you want to fix the underlying issues in this part of the code, you'll need to figure out when SetIMAPDeletedFlag is really supposed to be called, and fix that. But IMHO that is out of scope of this bug.

::: mailnews/imap/src/nsImapMailFolder.cpp
@@ +4991,5 @@
> +    DeleteStoreMessages(affectedMessages);
> +    mDatabase->DeleteMessages(affectedMessages.Length(), affectedMessages.Elements(), nullptr);
> +  }
> +  else // && !imapDeleteIsMoveToTrash
> +    SetIMAPDeletedFlag(mDatabase, affectedMessages, false);

Doesn't this change behavior? Previously, this line was never executed, since !ShowDeletedMessages() needed to be true to get into the loop that contained it, yet the else clause is only executed if !ShowDeletedMessages() is false.

With the change, this is executed when ShowDeletedMessages() is true.

Clearly the original code has an issue, but whatever you are "fixing" is not really the subject of this bug, and may or may not be an improvement.

::: mailnews/imap/src/nsImapUtils.cpp
@@ +334,5 @@
>  
>  void ParseUidString(const char *uidString, nsTArray<nsMsgKey> &keys)
>  {
>    // This is in the form <id>,<id>, or <id1>:<id2>
> +  if (!uidString)

This particular change is OK, in keeping with our usual very conservative checking style. There were callers where I would have to trace through more than two levels of function calls to convince myself that this null check is not needed, so it is good to add it.

Comment 5

[:aceman]

Attachment: patch v2

Sure, no problem. Thanks.

Comment 6

[Kent James (:rkent)]

Comment on attachment 8772534 [details] [diff] [review]
patch v2

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

Thanks.

Comment 7

[:aceman]

https://hg.mozilla.org/comm-central/rev/080e848565e59f79204a81b3a5261b390821e226