(coverity) uninitialized scalar variable: mailnews/imap/src/nsImapProtocol.cpp: |size| is not initialized always.
Categories
(MailNews Core :: Networking: IMAP, defect)
Tracking
(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)
5.45 KB,
patch
|
mkmelin
:
review+
|
Details | Diff | Splinter Review |
Assignee | ||
Comment 1•6 years ago
|
||
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()
orGetNamespaceForMailboxForHost()
, 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.
Updated•6 years ago
|
Comment 2•5 years ago
|
||
Should we do this before moving on to Bug 1565517 - crash in nsImapProtocol::GetMessageSize via nsIMAPBodyShell::Generate ?
Reporter | ||
Comment 3•5 years ago
|
||
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.
Assignee | ||
Comment 4•5 years ago
|
||
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
Reporter | ||
Comment 5•5 years ago
|
||
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.
Assignee | ||
Comment 6•5 years ago
|
||
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•5 years ago
|
||
Updated•5 years ago
|
Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/3528b15b3c00
Fix uninitialised var in nsImapProtocol::GetMessageSize(). r=mkmelin DONTBUILD
Updated•5 years ago
|
Updated•5 years ago
|
Comment 9•5 years ago
|
||
Please nominate for uplift to esr when you are ready. Perhaps best not to land and ship at same time as other imap patches.
Description
•