1370357 - Issues found by cppcheck in c-c

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[:aceman]

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

Comment 1

[:aceman]

Attachment: patch

I looked over the cases and it seems to me only this one is actionable.

Comment 2

[Jorg K (CEST = GMT+2)]

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 3

[Jorg K (CEST = GMT+2)]

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.

Comment 4

[:aceman]

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

Comment 5

[:aceman]

Attachment: patch v2

Comment 6

[Jorg K (CEST = GMT+2)]

Let's introduce an 'undefined' value here, please. Let's not confuse the issue by misusing an existing value. You're Mr. Purist, no?

Comment 7

[:aceman]

Attachment: patch v3

OK.

Comment 8

[Jorg K (CEST = GMT+2)]

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.

Comment 9

[:aceman]

Maybe because an uninitialized value could be initialized by the compiler to 0 and that would match the aAppendUndefined value.

Comment 10

[:aceman]

Attachment: patch v3.1

Comment 11

[Jorg K (CEST = GMT+2)]

Comment on attachment 8875486 [details] [diff] [review]
patch v3.1

Sorry about the silly discussion, however, I think this is best.

Comment 12

[ISHIKAWA, Chiaki]

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

Comment 13

[:aceman]

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.

Comment 14

[ISHIKAWA, Chiaki]

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

Comment 15

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/58ce0f54ddab67ebe71e36035bb4a102d68fc084