Open Bug 1280495 Opened 4 years ago Updated 4 years ago

(coverity) uninitialized scalar variable: mailnews/imap/src/nsImapMailFolder.cpp: |rv| is not set properly (either uninitialized and some error values are lost).

Categories

(MailNews Core :: Networking: IMAP, defect)

defect
Not set

Tracking

(Not tracked)

People

(Reporter: ishikawa, Unassigned)

References

(Blocks 1 open bug, )

Details

(Keywords: coverity, Whiteboard: CID 1137546)

Coverity found this:


9041NS_IMETHODIMP
9042nsImapMailFolder::StoreCustomKeywords(nsIMsgWindow *aMsgWindow, const nsACString& aFlagsToAdd,
9043                                      const nsACString& aFlagsToSubtract, nsMsgKey *aKeysToStore, uint32_t aNumKeys, nsIURI **_retval)
9044{
    1. var_decl: Declaring variable rv without initializer.
9045  nsresult rv;
    2. Condition WeAreOffline(), taking true branch
9046  if (WeAreOffline())
9047  {
9048    GetDatabase();
    3. Condition this->mDatabase.operator bool(), taking true branch
9049    if (mDatabase)
9050    {
    4. Condition keyIndex < aNumKeys, taking false branch
9051      for (uint32_t keyIndex = 0; keyIndex < aNumKeys; keyIndex++)
9052      {
9053        nsCOMPtr <nsIMsgOfflineImapOperation> op;
9054        rv = mDatabase->GetOfflineOpForKey(aKeysToStore[keyIndex], true, getter_AddRefs(op));
9055        SetFlag(nsMsgFolderFlags::OfflineEvents);
9056        if (NS_SUCCEEDED(rv) && op)
9057        {
9058          if (!aFlagsToAdd.IsEmpty())
9059            op->AddKeywordToAdd(PromiseFlatCString(aFlagsToAdd).get());
9060          if (!aFlagsToSubtract.IsEmpty())
9061            op->AddKeywordToRemove(PromiseFlatCString(aFlagsToSubtract).get());
9062        }
9063      }
9064      mDatabase->Commit(nsMsgDBCommitType::kLargeCommit); // flush offline ops
    CID 1137546 (#1 of 1): Uninitialized scalar variable (UNINIT)5. uninit_use: Using uninitialized value rv.
9065      return rv;
9066    }
9067  }

Observation:

When the for-loop is not executed once, then |rv| is not set at all. (This suggests a bug of wider scope. What if for-loop is not executed at all (?).)
(I think we should put in NS_ASSERT_STATE(aNumKeys > 0) or something similar to check for the sanity of the internal state and this code.)

It also suggests that error in one early loop of for-statement is not recorded or reported at all. *if* the subsequent rv = mDatabase->GetOfflineOpForKey(aKeysToStore[keyIndex], true, getter_AddRefs(op)); succeeds, the earlier error is forgotten. This is not right, I think.

This short piece of code leaves so much for room for appropriate comment.
What if mDatabase is null on 9049?
Shouldn't we do some error processing?
Or falling through to 9068 is OK?

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