Closed Bug 1280662 Opened 4 years ago Closed 7 months ago

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

Categories

(MailNews Core :: Networking: IMAP, defect)

defect
Not set
normal

Tracking

(thunderbird_esr68+ affected, thunderbird75+ wontfix, thunderbird76 fixed)

RESOLVED FIXED
Thunderbird 76.0
Tracking Status
thunderbird_esr68 + affected
thunderbird75 + wontfix
thunderbird76 --- fixed

People

(Reporter: ishikawa, Assigned: benc)

References

(Blocks 1 open bug, )

Details

(Keywords: coverity, Whiteboard: CID 1137545)

Attachments

(1 file, 1 obsolete file)

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.

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.

Assignee: nobody → benc
Status: NEW → ASSIGNED

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

Flags: needinfo?(gds)
Flags: needinfo?(benc)

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.

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

Attachment #9060298 - Attachment is obsolete: true
Flags: needinfo?(benc)
Attachment #9134032 - Flags: review?(ishikawa)

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.

Flags: needinfo?(mkmelin+mozilla)

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 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.
Attachment #9134032 - Flags: review?(ishikawa) → review+
Flags: needinfo?(mkmelin+mozilla)
Target Milestone: --- → Thunderbird 76.0

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/3528b15b3c00
Fix uninitialised var in nsImapProtocol::GetMessageSize(). r=mkmelin DONTBUILD

Status: ASSIGNED → RESOLVED
Closed: 7 months ago
Resolution: --- → FIXED

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

Flags: needinfo?(gds)
You need to log in before you can comment on or make changes to this bug.