Closed Bug 1584473 Opened 5 months ago Closed 3 months ago

Bugs detected in Thunderbird and Mozilla platform code with the help of PVS-Studio

Categories

(MailNews Core :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 71.0

People

(Reporter: razmyslov, Assigned: benc)

References

Details

(Keywords: leave-open)

Attachments

(7 files, 1 obsolete file)

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

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?

Flags: needinfo?(benc)

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

https://searchfox.org/comm-central/rev/266e9cc242cd0de076e85eb4aa0b8392fcb2ca01/mailnews/import/outlook/src/MapiApi.cpp#1310
2x PT_ERROR

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&regexp=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&regexp=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&regexp=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&regexp=false&path=
if (mVRSystem || mVRCompositor || mVRSystem) {

DOM (Boris):
https://searchfox.org/mozilla-central/search?q=clonedDoc-%3EmPreloadReferrerInfo+%3D+clonedDoc-%3EmPreloadReferrerInfo%3B&case=false&regexp=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&regexp=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&regexp=false&path=


NSS: Kai, can you take a look at the ones in NSS and nsprpub, please.

Flags: needinfo?(kaie)
Flags: needinfo?(honzab.moz)
Flags: needinfo?(dtownsend)
Flags: needinfo?(bzbarsky)
Summary: Bugs detected in Thunderbird with the help of PVS-Studio → Bugs detected in Thunderbird and Mozilla platform code with the help of PVS-Studio

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.

Keywords: leave-open

That's the easy stuff, removing doubled up sub-expressions.

Attachment #9097158 - Flags: review?(acelists)
Comment on attachment 9097158 [details] [diff] [review]
1584473-mime-mapi.patch

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

Thanks.

::: mailnews/mime/emitters/nsEmitterUtils.cpp
@@ +34,5 @@
> +        !strcmp(header, HEADER_REFERENCES) ||
> +        !strcmp(header, HEADER_NEWSGROUPS) ||
> +        !strcmp(header, HEADER_MESSAGE_ID) || !strcmp(header, HEADER_FROM) ||
> +        !strcmp(header, HEADER_FOLLOWUP_TO) || !strcmp(header, HEADER_CC) ||
> +        !strcmp(header, HEADER_ORGANIZATION) || !strcmp(header, HEADER_BCC))

I would prefer one strcmp per line here, but if the formatter folds it again, then we must live with this ugliness.
Attachment #9097158 - Flags: review?(acelists) → review+
Status: UNCONFIRMED → NEW
Component: Untriaged → General
Ever confirmed: true
OS: Unspecified → All
Product: Thunderbird → MailNews Core
Hardware: Unspecified → All
Attached patch 1584473-libical.patch (obsolete) — Splinter Review

Words fail to describe what I saw :-(

Attachment #9097159 - Flags: review?(geoff)

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&regexp=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?

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/629098a84b46
Fix doubled up sub-expressions in MIME and MAPI. r=aceman

OK, filed bug 1584785 (Network), bug 1584786 (Graphics), bug 1584787 (DOM), bug 1584788 (XPCOM), bug 1584789 (WebRTC), bug 1584791 (NSPR/NSS).

Flags: needinfo?(kaie)
Flags: needinfo?(honzab.moz)
Flags: needinfo?(dtownsend)
Flags: needinfo?(bzbarsky)

(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?

Assignee: nobody → benc
Flags: needinfo?(benc)

Not at all. So the answer is: Dead code doesn't crash ;-) - But why is the function in two header files?

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.

(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 on attachment 9097239 [details] [diff] [review]
1584473-mork-remove-makecell-1.patch

Less code is better code ;-) (that's not true actually).
Attachment #9097239 - Flags: review+
Comment on attachment 9097242 [details] [diff] [review]
1584473-fix-cmapiapi-HRESULT-1.patch

As I said, I think this is dead code, but as you please.
Attachment #9097242 - Flags: review+
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/2220e70b16a4
Remove broken/unused morkRowCellCursor::MakeCell(). r=jorgk
https://hg.mozilla.org/comm-central/rev/f1957edea768
Fix undefined HRESULT codes in class CMapiApi in MapiApi.cpp. r=jorgk
Comment on attachment 9097159 [details] [diff] [review]
1584473-libical.patch

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

Yes, well. It beats me why we haven't updated this whole library from upstream (not that that is perfect either).

r+ with the comment addressed.

::: calendar/libical/src/libical/icalparameter.c
@@ +110,5 @@
>      if (param->x_name != 0){
>  	free ((void*)param->x_name);
>      }
>      
> +    /* THIS LOOKS WRONG: memset(param,0,sizeof(param)); */

https://github.com/libical/libical/blob/master/src/libical/icalparameter.c#L78 has this as `memset(param, 0, sizeof(icalparameter));`
Attachment #9097159 - Flags: review?(geoff) → review+

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.

Target Milestone: --- → Thunderbird 71.0
Attachment #9097159 - Attachment is obsolete: true
Attachment #9097252 - Flags: review+

Damn, the MIME thing is still to go.

Keywords: leave-open

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.

Jorg, thank you for filing the various Gecko bugs here!

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/c2fa91efcf51
Fix wrong arguments to memset(), snprintf(), strncmp() in libical. r=darktrojan

Keywords: checkin-needed
Comment on attachment 9097239 [details] [diff] [review]
1584473-mork-remove-makecell-1.patch

1584473-mork-remove-makecell-1.patch removed makeCell but are the comments still ok?

> NS_IMETHOD SeekCell( // same as SetRow() followed by MakeCell()

Shouldn't it be?
// same as SetRow() followed by get cell at current pos in the row
Flags: needinfo?(jorgk)

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 ;-)

Flags: needinfo?(jorgk) → needinfo?(benc)

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 :)

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...

Flags: needinfo?(benc)
Attachment #9097839 - Flags: review?(frgrahl)

Jorg: I haven't looked at the ResetChannelCharset() one yet, but I will tomorrow.

Comment on attachment 9097839 [details] [diff] [review]
1584473-remove-leftover-makecell-comments.patch

Thanks.
Attachment #9097839 - Flags: review?(frgrahl) → review+
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/0fd7c679be52
Follow-up: Remove leftover references to nsIMdbRowCellCursor::MakeCell() in comments. r=jorgk DONTBUILD

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 on attachment 9098425 [details] [diff] [review]
1584473-fix-ResetChannelCharset-1.patch

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

Thanks, I guess you didn't try this code, or did you?

We are both convinced that this code will 100% cause a hang, right? It's been in the system at least since TB 1.9.1:
https://dxr.mozilla.org/comm-1.9.1/source/mailnews/mime/src/mimemoz2.cpp#2048
and never caused a hang, right? So the logical consequence is that his 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.

::: mailnews/mime/src/mimemoz2.cpp
@@ +1789,5 @@
> +          if (*ptr == '"') {
> +            ++ptr;  // Skip optional opening quote.
> +          }
> +          char *cSet = ptr;  // Save beginning of charset name.
> +          while (*ptr && strchr(" ;\r\n\"", *ptr)) {

I think you're missing a ! here: !strchr(" ;\r\n\"", *ptr)

@@ +1793,5 @@
> +          while (*ptr && strchr(" ;\r\n\"", *ptr)) {
> +            ++ptr;
> +          }
> +          if (ptr != cSet) {
> +            *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.

@@ +1795,5 @@
> +          }
> +          if (ptr != cSet) {
> +            *ptr = '\0';  // We're working in a temp buf, so OK to truncate.
> +            PR_FREEIF(obj->options->default_charset);
> +            obj->options->default_charset = strdup(cSet);

Instead of a strdup here, you could malloc cSet-ptr+1 bytes, then copy bytes and null terminate in the copy.
Attachment #9098425 - Flags: feedback-
Attached patch mime-debug.patchSplinter Review

Sorry, I forgot to attach my debug patch.

(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)

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.

Flags: needinfo?(benc)

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.

Blocks: 1597891

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 :-)

Flags: needinfo?(benc)

We're done here with the MIME bit moved to bug 1597891.

Status: NEW → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.