1572725 - (coverity) Uninitialized scalar variable: mailnews/imap/src/nsImapMailFolder.cpp: uninitialized value rv.

Status: RESOLVED FIXED

(MailNews Core :: Networking: IMAP, defect)

Description

[Wayne Mery (:wsmwk)]

8940 for (uint32_t keyIndex = 0; keyIndex < aNumKeys; keyIndex++)
8930NS_IMETHODIMP
8931nsImapMailFolder::StoreCustomKeywords(nsIMsgWindow *aMsgWindow, const nsACString& aFlagsToAdd,
8932 const nsACString& aFlagsToSubtract, nsMsgKey *aKeysToStore, uint32_t aNumKeys, nsIURI **_retval)
8933{

1. var_decl: Declaring variable rv without initializer.
   8934 nsresult rv;
   2. Condition WeAreOffline(), taking true branch.
   8935 if (WeAreOffline())
   8936 {
   8937 GetDatabase();
   3. Condition this->mDatabase.operator bool(), taking true branch.
   8938 if (mDatabase)
   8939 {
   4. Condition keyIndex < aNumKeys, taking false branch.
   8940 for (uint32_t keyIndex = 0; keyIndex < aNumKeys; keyIndex++)
   8941 {
   8942 nsCOMPtr <nsIMsgOfflineImapOperation> op;
   8943 rv = mDatabase->GetOfflineOpForKey(aKeysToStore[keyIndex], true, getter_AddRefs(op));
   8944 SetFlag(nsMsgFolderFlags::OfflineEvents);
   8945 if (NS_SUCCEEDED(rv) && op)
   8946 {
   8947 if (!aFlagsToAdd.IsEmpty())
   8948 op->AddKeywordToAdd(PromiseFlatCString(aFlagsToAdd).get());
   8949 if (!aFlagsToSubtract.IsEmpty())
   8950 op->AddKeywordToRemove(PromiseFlatCString(aFlagsToSubtract).get());
   8951 }
   8952 }
   8953 mDatabase->Commit(nsMsgDBCommitType::kLargeCommit); // flush offline ops
   CID 1417619 (#1 of 1): Uninitialized scalar variable (UNINIT)

Comment 1

[ISHIKAWA, Chiaki]

Wayne,

IMAP source code is something I wish I could study in depth some day.
(10-15 years ago, imap implementations still have some serious bugs and I did not trust to use the servers. But GMAIL now is used by a lot of people [I know google has a special proprietary extension of the protocol which sometimes cause problems for TB].
So maybe I should try thinking of using IMAP instead of POP3, but I have no intention of switching in the foreseeable future given the bugs reported by IMAP users.
I think this and other hitherto unfound bugs may explain the buggy symptoms reported by imap users.

But right now I am busy trying to resync my patches for buffered write for TB against the change(s0 that took place in the early summer of 2018 and afterward, and the latest clang-format source file reformatting.
I have managed to resync the bulk of the changes and can compile and submit tryserver jobs. Great.
But I think I hit on a window-specific bug again (an opened file, that is a file with open descriptor pointing to it cannot be deleted or renamed until it is closed under windows. Under linux or OSX, it can. This causes a problem.)
Since I develop patches under linux locally, I have to rely on the log on the try server to figure the Window problem, and it is taking longer than I thought.

Once I fix it, maybe I can look at this bugzilla entry.
But given the relative simplicity after a quick look, somebody else can beat me to it easily... :-)

Comment 2

[Jorg K (CEST = GMT+2)]

Attachment: 1572725-init-rv.patch

Not really an issue since it only happens if aNumKeys is passed in as zero.

First reviewer wins ;-)

Comment 3

[Jorg K (CEST = GMT+2)]

Comment on attachment 9084486 [details] [diff] [review]
1572725-init-rv.patch

Looking at this again, well, the status returned after the loop if the one from the last iteration. So if something went wrong in a prior iteration, we still return OK.

So the question it: Should we always return OK, or should we keep a bad status and return it, à la:
```
nsresult rv2 = NS_OK;
...
if (NS_FAILED(rv)) rv2 = rv;
...
return rv2;
```

Opinions?

Comment 4

[Jorg K (CEST = GMT+2)]

Attachment: 1572725-init-rv.patch - alternative

This is the alternative.

Comment 5

[:aceman]

The function is strange. In case of !mDatabase it falls through to the online version of the code?
What about just returning at the top of the function when aNumKeys is 0 ?

Comment 6

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #5)

The function is strange. In case of !mDatabase it falls through to the online version of the code?

We're fixing that?

What about just returning at the top of the function when aNumKeys is 0 ?

Yes, could be.

Comment 7

[Jorg K (CEST = GMT+2)]

Attachment: 1572725-init-rv.patch (v3)

As per our IRC discussion:

Returning early if there's nothing to do.
Not falling into the online code from the offline code, return error if no DB.
If an error happened in the loop, return it.

Uff, so many changes to fix an uninitialised variable.

Comment 8

[:aceman]

Comment on attachment 9084488 [details] [diff] [review]
1572725-init-rv.patch (v3)

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

Yes I like this, thanks.
I would just switch the rv2 around with rv. Use rv2 for the local result and set rv if it fails and then return rv.

Comment 9

[Jorg K (CEST = GMT+2)]

Attachment: 1572725-init-rv.patch (vb)

Like that then?

Comment 10

[Jorg K (CEST = GMT+2)]

Attachment: 1572725-init-rv.patch (v3c)

Oops, unwanted hunk in there.

Comment 11

[:aceman]

Comment on attachment 9084493 [details] [diff] [review]
1572725-init-rv.patch (v3c)

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

Yes, thanks.

Comment 12

[Jorg K (CEST = GMT+2)]

Uff, I've come back to this again. In the online case we call AllocateUidStringFromKeys() and that returns a bad status when called with zero:
https://searchfox.org/comm-central/rev/993b0902886414e447f4d6ddfac1ecea9cf2b16e/mailnews/imap/src/nsImapMailFolder.cpp#1925

So should we remove the NS_OK return on zero keys or turn that into an error? Coverity complained exactly about that: Offline and no keys returned an undefined value in rv.

Comment 13

[Jorg K (CEST = GMT+2)]

OK, Aceman pointed out that the status of AllocateUidStringFromKeys() isn't checked, so we run nsImapService::StoreCustomKeywords() with an empty list of UIDs:
https://searchfox.org/comm-central/rev/993b0902886414e447f4d6ddfac1ecea9cf2b16e/mailnews/imap/src/nsImapService.cpp#2969

So we do some IMAP traffic for no good reason. OK, let's land the patch as-is. If no keys, it's a no-op. I wonder what that will break :-(

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/9b6c99491c64
(coverity) initalise rv in nsImapMailFolder::StoreCustomKeywords(). r=aceman