Closed Bug 1280662 Opened 9 years ago Closed 5 years 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: 5 years 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.

Attachment

General

Creator:
Created:
Updated:
Size: