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.

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?

(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.

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 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.

Sure, no problem. Thanks.

Comment on attachment 8772534 [details] [diff] [review] patch v2 Review of attachment 8772534 [details] [diff] [review]: ----------------------------------------------------------------- Thanks.