1280662 - (coverity) uninitialized scalar variable: mailnews/imap/src/nsImapProtocol.cpp: |size| is not initialized always.

Status: ASSIGNED

(MailNews Core :: Networking: IMAP, defect)

Description

[ISHIKAWA, Chiaki (may be slow to respond until Jan 4.)]

Coverity found this:
|size| is not initialized always, but is used.

// In 4.5, this posted an event back to libmsg and blocked until it got a response.
4450// We may still have to do this.It would be nice if we could preflight this value,
4451// but we may not always know when we'll need it.
4452uint32_t nsImapProtocol::GetMessageSize(const char * messageId,
4453                                        bool idsAreUids)
4454{
4455  const char *folderFromParser = GetServerStateParser().GetSelectedMailboxName();
    1. Condition folderFromParser, taking true branch
    2. Condition messageId, taking true branch
4456  if (folderFromParser && messageId)
4457  {
4458    char *id = (char *)PR_CALLOC(strlen(messageId) + 1);
4459    char *folderName;
    3. var_decl: Declaring variable size without initializer.
4460    uint32_t size;
4461
4462    PL_strcpy(id, messageId);
4463
4464    nsIMAPNamespace *nsForMailbox = nullptr;
4465        m_hostSessionList->GetNamespaceForMailboxForHost(GetImapServerKey(), folderFromParser,
4466            nsForMailbox);
4467
4468
    4. Condition nsForMailbox, taking true branch
4469    if (nsForMailbox)
    5. Falling through to end of if statement
4470          m_runningUrl->AllocateCanonicalPath(
4471              folderFromParser, nsForMailbox->GetDelimiter(),
4472              &folderName);
4473    else
4474       m_runningUrl->AllocateCanonicalPath(
4475          folderFromParser,kOnlineHierarchySeparatorUnknown,
4476          &folderName);
4477
    6. Condition id, taking true branch
    7. Condition folderName, taking true branch
4478    if (id && folderName)
4479    {
    8. Condition this->m_imapMessageSink.operator bool(), taking false branch
4480      if (m_imapMessageSink)
4481          m_imapMessageSink->GetMessageSizeFromDB(id, &size);
4482    }
    9. Condition id, taking true branch
4483    PR_FREEIF(id);
    10. Condition folderName, taking true branch
4484    PR_FREEIF(folderName);
4485
4486    uint32_t rv = 0;
    11. Condition !this->DeathSignalReceived(), taking true branch
4487    if (!DeathSignalReceived())
    CID 1137545 (#1 of 1): Uninitialized scalar variable (UNINIT)12. uninit_use: Using uninitialized value size.
4488      rv = size;
4489    return rv;
4490  }
4491  return 0;
4492}

Observation. 
IMHO, we should at least put NS_WARNING() to print the strange ignored situations where |size| is not set. (Error conditions silently ignored are bad symptoms of TB code.)

Setting size = 0 seems to be a reasonable choice since 0 is returned at the end of the function.

Comment 1

[Ben Campbell]

Attachment: 1280662-fix-uninitialised-size-in-nsimapprotocol.patch

Picking through nsImapProtocol::GetMessageSize(), it /seems/ like most of it isn't even needed.
This patch:

• strips out most of the code (I couldn't see any side effects in AllocateCanonicalPath() or GetNamespaceForMailboxForHost(), and couldn't see any reason why they should fail in normal operation.
• removes the now-unused idsAreUids param.
• tries to distinguish between stuff that should never ever happen (assert!) and stuff that might happen with bad data (NS_WARNING). Possibly some of the warning checks should be asserts, but I don't know the code well enough to say for sure.

It seems to pass the IMAP unit tests OK for me locally...
Will give it another sanity check with fresh eyes tomorrow before getting anyone to review it, but plonking it here in case someone has some other thoughts in the meantime.