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

Status: RESOLVED FIXED

(MailNews Core :: Networking: IMAP, defect)

Description

[ISHIKAWA, Chiaki]

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.

Comment 2

[Wayne Mery (:wsmwk)]

Should we do this before moving on to Bug 1565517 - crash in nsImapProtocol::GetMessageSize via nsIMAPBodyShell::Generate ?

Comment 3

[ISHIKAWA, Chiaki]

From a third party observer (well I actually reported this in the first place), why don't we land the patch Ben created provided that it does not cause regressions on the tryserver?

We need whatever incremental improvement to the codebase IMHO.

Comment 4

[Ben Campbell]

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

A reworked version of my patch, with the benefit of a little more knowledge of the IMAP code.

It highlights a little ambiguity about UID vs sequence number which I'm still confused by, but the patch retains the same functionality, so won't make things any worse.
I'm still wading through the IMAP code, so I'll revisit this if I have any further insights.

Try build here looks OK (Linux64 only. I did just kick off a cross platform one too, but I'm not sure it'll complete):
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=cecfa56985003174c74d2d3e8a7a8622deddb2bd

Comment 5

[ISHIKAWA, Chiaki]

It looks fine to me, but I am not expert on IMAP code.
I would be more comfortable to get an opinion from someone else like, say, Magnus.

CC'ing him.

Comment 6

[Ben Campbell]

Hmm. I ran a more comprehensive try build...
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=96a7b1a11bc5c299ff0421f03596810aa560001c
there are a couple of fails, but I'm not sure if they are related - linux64 opt was fine last time, but now it fails a mochitest :-(

Comment 7

[Magnus Melin [:mkmelin]]

Comment on attachment 9134032 [details] [diff] [review]
1280662-fix-uninitialised-size-in-nsimapprotocol-2.patch

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

Looks good I think. The try failure is just an intermittent.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/3528b15b3c00
Fix uninitialised var in nsImapProtocol::GetMessageSize(). r=mkmelin DONTBUILD

Comment 9

[Wayne Mery (:wsmwk)]

Please nominate for uplift to esr when you are ready. Perhaps best not to land and ship at same time as other imap patches.