Issues found by cppcheck in c-c

RESOLVED FIXED in Thunderbird 55.0

Status

defect
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: aceman, Assigned: aceman)

Tracking

Trunk
Thunderbird 55.0
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

The cppcheck tool (http://cppcheck.sourceforge.net/) found the following possible problems in /mailnews:

[mailnews/addrbook/src/nsAbCardProperty.cpp:1037]: (error) Uninitialized struct member: item.mAppendType
[mailnews/addrbook/src/nsAbCardProperty.cpp:1042]: (error) Uninitialized struct member: item.mAppendType

[mailnews/base/util/nsMsgKeySet.cpp:1254]: (error) Memory pointed to by 's' is freed twice.
[mailnews/base/util/nsMsgKeySet.cpp:1255]: (error) Memory pointed to by 's' is freed twice.
[mailnews/base/util/nsMsgKeySet.cpp:1256]: (error) Memory pointed to by 's' is freed twice.
[mailnews/base/util/nsMsgKeySet.cpp:1257]: (error) Memory pointed to by 's' is freed twice.
[mailnews/base/util/nsMsgKeySet.cpp:1258]: (error) Memory pointed to by 's' is freed twice.
[mailnews/base/util/nsMsgKeySet.cpp:1259]: (error) Memory pointed to by 's' is freed twice.
[mailnews/base/util/nsMsgKeySet.cpp:1260]: (error) Memory pointed to by 's' is freed twice.
[mailnews/base/util/nsMsgKeySet.cpp:1262]: (error) Memory pointed to by 's' is freed twice.
[mailnews/base/util/nsMsgKeySet.cpp:1263]: (error) Memory pointed to by 's' is freed twice.
[mailnews/base/util/nsMsgKeySet.cpp:1264]: (error) Memory pointed to by 's' is freed twice.
[mailnews/base/util/nsMsgKeySet.cpp:1265]: (error) Memory pointed to by 's' is freed twice.

[mailnews/compose/src/nsMsgSend.cpp:1416]: (error) Uninitialized variable: isHttp
[mailnews/compose/src/nsMsgSend.cpp:1417]: (error) Uninitialized variable: isHttp
[mailnews/compose/src/nsMsgSend.cpp:1431]: (error) Uninitialized variable: isData
[mailnews/compose/src/nsMsgSend.cpp:1433]: (error) Uninitialized variable: isNews
[mailnews/compose/src/nsMsgSend.cpp:1434]: (error) Uninitialized variable: isNews
[mailnews/compose/src/nsMsgSend.cpp:1435]: (error) Uninitialized variable: isNews
[mailnews/compose/src/nsMsgSend.cpp:1447]: (error) Uninitialized variable: isFile

[mailnews/imap/src/nsImapMailFolder.cpp:5402]: (error) Uninitialized variable: supportsCompaction

[mailnews/mime/src/mimefilt.cpp:49]: (error) syntax error
Posted patch patch (obsolete) — Splinter Review
I looked over the cases and it seems to me only this one is actionable.
Attachment #8874585 - Flags: review?(jorgk)
I love tools spitting out false positives, they are a real time-waster.

nsMsgSend.cpp:1416
  bool isHttp =
    (NS_SUCCEEDED(attachment->m_url->SchemeIs("http", &isHttp)) && isHttp) ||
    (NS_SUCCEEDED(attachment->m_url->SchemeIs("https", &isHttp)) && isHttp);
Tricky, but not uninitialised.

The double-free in nsMsgKeySet.cpp is a false alarm, too:
#define FROB(N,PUSHP)                  \
  i = N;                        \
  if (!(NS_SUCCEEDED(set->Output(&s)))) abort ();          \
  printf ("%3lu: %-58s %c %3lu =\n", (unsigned long)set->m_length, s,  \
      (PUSHP ? '+' : '-'), (unsigned long)i);            \
  free(s);                      \
  if (PUSHP                        \
    ? set->Add(i) < 0                  \
    : set->Remove(i) < 0)                \
  abort ();                      \
  if (!(NS_SUCCEEDED(set->Output(&s)))) abort ();          \
  printf ("%3lu: %-58s optimized =\n", (unsigned long)set->m_length, s);  \
  free(s);
Yes, it's free'ed twice but also allocated twice.

mimefilt.cpp:49 is intentional
#ifndef XP_UNIX
ERROR!  This is a unix-only file for the "mimefilt" standalone program.
    This does not go into libmime.a.
#endif

I'll leave
[mailnews/addrbook/src/nsAbCardProperty.cpp:1037]: (error) Uninitialized struct member: item.mAppendType
[mailnews/addrbook/src/nsAbCardProperty.cpp:1042]: (error) Uninitialized struct member: item.mAppendType
[mailnews/imap/src/nsImapMailFolder.cpp:5402]: (error) Uninitialized variable: supportsCompaction
to the reporter. Maybe they are valid.
Comment on attachment 8874585 [details] [diff] [review]
patch

Hmm, the issue in nsAbCardProperty.cpp could be fixed by creating an EAppendType of undefined and setting item.mAppendType to that.
Attachment #8874585 - Flags: review?(jorgk) → review+
(In reply to Jorg K (GMT+2) from comment #3)
> Hmm, the issue in nsAbCardProperty.cpp could be fixed by creating an
> EAppendType of undefined and setting item.mAppendType to that.

I think the member is unused in AppendLine so we can initialize it to the existin eAppendLine.

I also think the others are false positives, therefore I said they are not actionable.
Posted patch patch v2 (obsolete) — Splinter Review
Attachment #8874585 - Attachment is obsolete: true
Let's introduce an 'undefined' value here, please. Let's not confuse the issue by misusing an existing value. You're Mr. Purist, no?
Posted patch patch v3 (obsolete) — Splinter Review
OK.
Attachment #8874616 - Attachment is obsolete: true
Attachment #8875468 - Flags: review?(jorgk)
Comment on attachment 8875468 [details] [diff] [review]
patch v3

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

Sorry about the long discussion about this trivial thing :-(

::: mailnews/addrbook/src/nsAbCardProperty.cpp
@@ +39,3 @@
>    eAppendLine,
>    eAppendLabel,
>    eAppendCityStateZip

Any reason to have that at the front and change the numbering of the other items? "Line" was 0 before, now it shifted to 1. I'd add the undefined at the end.
Maybe because an uninitialized value could be initialized by the compiler to 0 and that would match the aAppendUndefined value.
Posted patch patch v3.1Splinter Review
Attachment #8875468 - Attachment is obsolete: true
Attachment #8875468 - Flags: review?(jorgk)
Comment on attachment 8875486 [details] [diff] [review]
patch v3.1

Sorry about the silly discussion, however, I think this is best.
Attachment #8875486 - Flags: review+
Keywords: checkin-needed
How do we enable cppcheck in c-c?
(I have run valgrind/memcheck and address sanitizer, and they are very time-consuming although they always print only genuine errors.)
It is a standalone tool (link in comment 0) and you just run it from command line on a folder with TB source code, e.g. /mailnews. It automatically detects all .cpp files and runs some basic tests on them. It seems to run multiple passes on each file depending on preprocessor defines used in it.
Thank you. I see it is not integrated into C-C source tree yet (?).
(At least valgrind has a very minimum rudimentary support for building the binary, and address sanitizer also.)
Hmm, I am still struggling with -Werror with some trivial warnings which are not checked locally but now checked on C-C tryserver.
So maybe in the future. Thank you again for your tips.
(You might as well subscribe to regular coverty C-C TB check e-mail service. It is much better than cppcheck at this stage obviously, BTW.)
https://hg.mozilla.org/comm-central/rev/58ce0f54ddab67ebe71e36035bb4a102d68fc084
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 55.0
You need to log in before you can comment on or make changes to this bug.