Bugs detected in Thunderbird and Mozilla platform code with the help of PVS-Studio
Categories
(MailNews Core :: General, defect)
Tracking
(Not tracked)
People
(Reporter: razmyslov, Assigned: benc)
References
Details
(Keywords: leave-open)
Attachments
(7 files, 1 obsolete file)
4.25 KB,
patch
|
aceman
:
review+
|
Details | Diff | Splinter Review |
4.56 KB,
patch
|
jorgk-bmo
:
review+
|
Details | Diff | Splinter Review |
2.13 KB,
patch
|
jorgk-bmo
:
review+
|
Details | Diff | Splinter Review |
5.83 KB,
patch
|
jorgk-bmo
:
review+
|
Details | Diff | Splinter Review |
3.09 KB,
patch
|
jorgk-bmo
:
review+
|
Details | Diff | Splinter Review |
2.40 KB,
patch
|
jorgk-bmo
:
feedback-
|
Details | Diff | Splinter Review |
1.97 KB,
patch
|
Details | Diff | Splinter Review |
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:69.0) Gecko/20100101 Firefox/69.0
Steps to reproduce:
Launched the PVS-Studio Static Analyzer on the Mozilla code base.
Actual results:
Found several bugs. Please, read the report. "Dark theme of Thunderbird as a reason to run a code analyzer": https://www.viva64.com/en/b/0672/
Expected results:
Use this license to verify the project yourself:
Name: Mozilla Thunderbird
Key: EGWG-555N-1KRG-T407
License Type: Team9 License up to 9 developers
License Valid Thru: Oct 28, 2019
Comment 1•5 years ago
|
||
Interesting. Please note that the Thunderbird project is only in charge of the "comm" source code, not netwerk or gfx. nsprpub is also another project. Note that the RDF code was removed from the repository two days ago. libical is a 3rd party library.
We can take care of the other comm/ issues. Please file a separate bugs for netwerk, gfx and nsprpub.
Ben, interested in some of these?
Comment 2•5 years ago
|
||
Here are the comm problems reported:
https://searchfox.org/comm-central/rev/266e9cc242cd0de076e85eb4aa0b8392fcb2ca01/mailnews/mime/emitters/nsEmitterUtils.cpp#34
2x HEADER_REPLY_TO
https://searchfox.org/comm-central/rev/266e9cc242cd0de076e85eb4aa0b8392fcb2ca01/mailnews/mime/src/mimemsig.cpp#544
2x MimeHeadersCitation
RDF: Code removed
https://searchfox.org/comm-central/rev/266e9cc242cd0de076e85eb4aa0b8392fcb2ca01/db/mork/src/morkRowCellCursor.cpp#175
Hmm, row is set to 0 and then we do row-> ... - How does that work?
https://searchfox.org/comm-central/search?q=m_lastError+%3D+-1%3B&case=false®exp=false&path=
MAPI: m_lastError = -1; - I don't think this is active code.
https://searchfox.org/comm-central/search?q=memset(parts%2C0%2Csizeof(parts))%3B&case=false®exp=false&path=
libical: memset(parts,0,sizeof(parts));
nsLDAPMessage::GetBinaryValues(): Code has been rewritten.
https://searchfox.org/comm-central/rev/266e9cc242cd0de076e85eb4aa0b8392fcb2ca01/mailnews/mime/src/mimemoz2.cpp#1794-1797
Something badly wrong here in the while loop.
Mozilla core:
netwerk (Honza):
https://searchfox.org/mozilla-central/search?q=connectStarted+%26%26+connectStarted&case=false®exp=false&path=
connectStarted && connectStarted
GFX (who's in charge)?
https://searchfox.org/mozilla-central/search?q=if+(mVRSystem+%7C%7C+mVRCompositor+%7C%7C+mVRSystem)+%7B&case=false®exp=false&path=
if (mVRSystem || mVRCompositor || mVRSystem) {
DOM (Boris):
https://searchfox.org/mozilla-central/search?q=clonedDoc-%3EmPreloadReferrerInfo+%3D+clonedDoc-%3EmPreloadReferrerInfo%3B&case=false®exp=false&path=
clonedDoc->mPreloadReferrerInfo = clonedDoc->mPreloadReferrerInfo;
XPCOM (Dave):
https://searchfox.org/mozilla-central/search?q=HRESULT+result+%3D+SHGetSpecialFolderPathW(nullptr%2C+path%2C+aFolder%2C+true)%3B&case=false®exp=false&path=
HRESULT result = SHGetSpecialFolderPathW(nullptr, path, aFolder, true);
Should be BOOL
WebRTC: (who's in charge)?
https://searchfox.org/mozilla-central/search?q=state%5Bstate_length+-+x_length+%2B+i%5D+%3D+filtered_low%5Bi%5D%3B&case=false®exp=false&path=
NSS: Kai, can you take a look at the ones in NSS and nsprpub, please.
Updated•5 years ago
|
Comment 3•5 years ago
|
||
I suggest to fix the TB issues in this bug and file separate bugs for the core issues. I really don't want to do that.
Updated•5 years ago
|
Comment 4•5 years ago
|
||
That's the easy stuff, removing doubled up sub-expressions.
Comment 6•5 years ago
|
||
Words fail to describe what I saw :-(
Comment 7•5 years ago
|
||
OK, the first two patches address most of the comm/ errors, which leaves (from comment #2):
https://searchfox.org/comm-central/rev/266e9cc242cd0de076e85eb4aa0b8392fcb2ca01/db/mork/src/morkRowCellCursor.cpp#175
Hmm, row is set to 0 and then we do row-> ... - How does that work?
https://searchfox.org/comm-central/search?q=m_lastError+%3D+-1%3B&case=false®exp=false&path=
MAPI: m_lastError = -1; - I don't think this is active code.
https://searchfox.org/comm-central/rev/266e9cc242cd0de076e85eb4aa0b8392fcb2ca01/mailnews/mime/src/mimemoz2.cpp#1794-1797
Something badly wrong here in the while loop.
And don't think we will fix the MAPI thing, which leaves Mork and Mime. Hard to believe that Mork doesn't crash all the time and that Mine doesn't hang all the time. Ben, do you have further insights?
Comment 9•5 years ago
|
||
OK, filed bug 1584785 (Network), bug 1584786 (Graphics), bug 1584787 (DOM), bug 1584788 (XPCOM), bug 1584789 (WebRTC), bug 1584791 (NSPR/NSS).
Assignee | ||
Comment 10•5 years ago
|
||
(In reply to Jorg K (GMT+2) from comment #7)
OK, the first two patches address most of the comm/ errors, which leaves (from comment #2):
https://searchfox.org/comm-central/rev/266e9cc242cd0de076e85eb4aa0b8392fcb2ca01/db/mork/src/morkRowCellCursor.cpp#175
Hmm, row is set to 0 and then we do row-> ... - How does that work?
I don't think it /does/ work - it looks pretty broken to me too.
As far as I can see, MakeCell()
is never even called:
https://searchfox.org/comm-central/search?q=MakeCell%28&path=
Here's a patch to ditch it entirely (it seems fine locally, just running a try build now).
Too heavy-handed?
Comment 11•5 years ago
|
||
Not at all. So the answer is: Dead code doesn't crash ;-) - But why is the function in two header files?
Assignee | ||
Comment 12•5 years ago
|
||
Patch for the m_lastError = -1
thing. NOTE: I've never even compiled this patch! (It's windows-only)
Changes the -1 to a generic error code: E_UNEXPECTED
(0x8000FFFF).
More correctly, we could define our own error codes. Bit 29 of HRESULT
selects between microsoft-defined (0) and customer-defined (1). But really that seems overkill.
Assignee | ||
Comment 13•5 years ago
|
||
(In reply to Jorg K (GMT+2) from comment #11)
Not at all. So the answer is: Dead code doesn't crash ;-) - But why is the function in two header files?
I guess the idea is that outside code can just use mdb.h
as the public interface and shave a little off the build time by not including module-internal headers?
Comment 14•5 years ago
|
||
Comment 15•5 years ago
|
||
Comment 16•5 years ago
|
||
Comment 17•5 years ago
|
||
OK, onto the last mystery:
https://searchfox.org/comm-central/rev/266e9cc242cd0de076e85eb4aa0b8392fcb2ca01/mailnews/mime/src/mimemoz2.cpp#1794-1797
Something badly wrong here in the while loop.
Comment 18•5 years ago
|
||
Comment 19•5 years ago
|
||
And they fixed the comparisons, too:
https://github.com/libical/libical/blob/master/src/libical/sspm.c#L754
if (strcmp((line + 2), parent_header->boundary) == 0) {
And they also added buffer_size:
https://github.com/libical/libical/blob/master/src/libical/icaltimezone.c#L154
I think I'll change the variable name to match that.
Updated•5 years ago
|
Comment 20•5 years ago
|
||
Comment 22•5 years ago
|
||
Looking at this: When you reply to a message, ResetChannelCharset() is triggered here:
https://searchfox.org/comm-central/rev/266e9cc242cd0de076e85eb4aa0b8392fcb2ca01/mailnews/mime/src/mimei.cpp#1673
But the bad code is only run for if ((msd) && (msd->format_out == nsMimeOutput::nsMimeMessageSaveAs)) {
Anyway, when I do "Save As" on a message, I don't get into ResetChannelCharset(). This needs more mucking around.
![]() |
||
Comment 23•5 years ago
|
||
Jorg, thank you for filing the various Gecko bugs here!
Comment 24•5 years ago
|
||
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/c2fa91efcf51
Fix wrong arguments to memset(), snprintf(), strncmp() in libical. r=darktrojan
![]() |
||
Comment 25•5 years ago
|
||
Comment 26•5 years ago
|
||
Thanks for following this. We're lucky the compiler didn't check the comments ;-)
Personally, I'd remove the comment altogether, but I'll let the original author/slasher decide ;-)
![]() |
||
Comment 27•5 years ago
|
||
We're lucky the compiler didn't check the comments ;-)
If I have nothing else to contribute I need to at least check for NITs :)
Assignee | ||
Comment 28•5 years ago
|
||
Ooh - well spotted, Frank!
Amusingly, SeekCell()
doesn't seem to be implemented. It's a stub.
And it is referenced in turn by comments in morkRowObject::SeekCellYarn()
, so I suspect there are turtles all the way down.... :-)
So I've gone for just deleting the MakeCell()-referencing comments and leaving it at that...
Assignee | ||
Comment 29•5 years ago
|
||
Jorg: I haven't looked at the ResetChannelCharset() one yet, but I will tomorrow.
Comment 30•5 years ago
|
||
Comment 31•5 years ago
|
||
Assignee | ||
Comment 32•5 years ago
|
||
On the ResetChannelCharset() issue:
Wow. Seems pretty clear that the original loop would just dive straight into an infinite loop :-(
Attached is my stab at reimplementing it. It compiles, but I've not got a test case for it (and it's not clear it ever gets called anyway, based on your observations, Jorg).
I suspect it could be much simpler: for example, just before the offending loop, it calls SetContentType() on the channel, which I think performs the parsing we want, and puts the charset into the channel's
contentCharset
attribute. So we could just dump the parsing here altogether. I think. Maybe.
I'm not sure this patch should be considered - It probably merits a "bigger picture" approach and I really don't understand all the context.
Comment 33•5 years ago
|
||
Comment 34•5 years ago
|
||
Sorry, I forgot to attach my debug patch.
Assignee | ||
Comment 35•5 years ago
|
||
(In reply to Jorg K (GMT+2) from comment #33)
Comment on attachment 9098425 [details] [diff] [review]
1584473-fix-ResetChannelCharset-1.patch
Thanks, I guess you didn't try this code, or did you?
Just to confirm: no. It compiles, but I've no idea if it's even possible to reach it at runtime :-)
So the logical consequence is that this code
absolutely doesn't get run.
Personally, I'd like to spend some time to prove that this is so and then
remove the faulty code so the next generation doesn't tear their hair out.
I agree. And even if this code is needed, it seems crazy that anywhere should be doing ad-hoc parsing to extract things out of of mime headers anyway.
I'll take a deeper look into it. I suspect I'm about to learn a lot more than I really wanted to about Mime structuring...
Just for completeness:
::: mailnews/mime/src/mimemoz2.cpp
while (*ptr && strchr(" ;\r\n\"", *ptr)) {
I think you're missing a ! here: !strchr(" ;\r\n"", *ptr)
I see you spotted my deliberate mistake to illustrate why we definitely shouldn't be doing ad-hoc parsing. cough :-)
I think I should have left it as separate comparisons after all.
(in fact, the *ptr &&
bit before it isn't needed either - strchr matches the '\0').
@@ +1793,5 @@
*ptr = '\0'; // We're working in a temp buf, so OK to truncate.
temp buf? Where? Am I missing something? There's no strdup any more.
The buffer is allocated by char *ct = MimeHeaders_get(...)
above and freed further down (the free doesn't show up in the diff)
Comment 36•5 years ago
|
||
Ben, if it's too hard to work out whether this code path is actually taken, how about just replacing the hang with the code you suggested and adding a MOZ_DISGNOSTIC_ASSERT(false, "We should never get into xxx");
That will only crash Dailies and debug builds.
Assignee | ||
Comment 37•5 years ago
|
||
Haven't forgotten about this one - just busy elsewhere.
I'd ignore the patch I posted - there's no reason to manually parse the content-type string. There's a good example of using an existing parsing function out in the calling code, so in the worst case I'd update my patch to use that. But I think I can do better - I'm pretty sure most of the function is redundant.
Assignee | ||
Comment 38•5 years ago
|
||
Spent a while trying to work out what ResetChannelCharset() is really used for... but couldn't figure it out.
So I've punted it to Bug 1597891. If there's ever a great Mime code reckoning, it can be part of that :-)
Assignee | ||
Updated•5 years ago
|
Comment 39•5 years ago
|
||
We're done here with the MIME bit moved to bug 1597891.
Description
•