Open Bug 1281353 Opened 4 years ago

(coverity) uninitialized scalar value: mailnews/imap/src/nsImapMailFolder.cpp: there are execution paths where |rv| is not set before returned by |return rv|

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 1137536)

Coverity found this:
There is an execution path where|rv| is not initialized before being returned |return rv| 

3927NS_IMETHODIMP nsImapMailFolder::StoreImapFlags(int32_t flags, bool addFlags,
3928                                               nsMsgKey *keys, uint32_t numKeys,
3929                                               nsIUrlListener *aUrlListener)
3930{
    1. var_decl: Declaring variable rv without initializer.
3931  nsresult rv;
    2. Condition !WeAreOffline(), taking false branch
3932  if (!WeAreOffline())
3933  {
3934    nsCOMPtr<nsIImapService> imapService = do_GetService(NS_IMAPSERVICE_CONTRACTID, &rv);
3935    NS_ENSURE_SUCCESS(rv, rv);
3936    nsAutoCString msgIds;
3937    AllocateUidStringFromKeys(keys, numKeys, msgIds);
3938    if (addFlags)
3939      imapService->AddMessageFlags(this, aUrlListener ? aUrlListener : this,
3940                                   nullptr, msgIds, flags, true);
3941    else
3942      imapService->SubtractMessageFlags(this, aUrlListener ? aUrlListener : this,
3943                                        nullptr, msgIds, flags, true);
3944  }
3945  else
3946  {
3947    GetDatabase();
    3. Condition this->mDatabase.operator bool(), taking true branch
3948    if (mDatabase)
3949    {
3950      uint32_t total = numKeys;
    4. Condition keyIndex < total, taking false branch
3951      for (uint32_t keyIndex = 0; keyIndex < total; keyIndex++)
3952      {
3953        nsCOMPtr <nsIMsgOfflineImapOperation> op;
3954        rv = mDatabase->GetOfflineOpForKey(keys[keyIndex], true, getter_AddRefs(op));
3955        SetFlag(nsMsgFolderFlags::OfflineEvents);
3956        if (NS_SUCCEEDED(rv) && op)
3957        {
3958          imapMessageFlagsType newFlags;
3959          op->GetNewFlags(&newFlags);
3960          op->SetFlagOperation(addFlags ? newFlags | flags : newFlags & ~flags);
3961        }
3962      }
3963      mDatabase->Commit(nsMsgDBCommitType::kLargeCommit); // flush offline flags
3964    }
3965  }
    CID 1137536 (#1 of 1): Uninitialized scalar variable (UNINIT)5. uninit_use: Using uninitialized value rv.
3966  return rv;

Observation:

I think we can set |rv| to NS_OK at the beginning.

I am not entirely sure how an error within the for loop in the latter half should be handled. There is clearly a problem. if for a multiple-count loop, if, in the LAST loop, mDatabase->GetOfflineOpForKey() failed, then that value would have been returned. 

However, if, in the SECOND LAST loop, the same function returns failure, but in the LAST loop, it returns NS_OK, then the function would have returned NS_OK. So the intent is to return what? 

Superficially, the value in the for-loop is not something that should be returned (?). In any case, |rv| in the top-most if() is set only when (!WeAreOffline()) OR WeAreOffline() AND mDatabase is non-null (after the call to GetDatabase). If both condition fail, |rv| is never set.

We need better comment, etc.
You need to log in before you can comment on or make changes to this bug.