This is spun-off of bug 676179: Actually, cppcheck showed 69 leaks in the code a few days ago. (45 memory leak, 24 resource leak (not closed file, etc.)) Some of them are false positive. I try to hunt the real ones one-by-one, and file a bug report for every individual bug. (I am not familiar with the firefox structure, and if I indicate two error in one report, you will not be able to assign them to two different components.) So some of them are already reported, and I work on the rest. But this is a very simple process, anyone can do it with some time, so I encourage Mozilla to use cppcheck as a daily test, because it's very powerful tool. (However, it has some false positive reports, especially when pointer==nsnull test happens, or where a factory method is used.) When I have time, I report as many bug as I can, but there are a few hundreds (few dozen leak, and a lot of possible null pointer dereference and others), so it will take a while. -------------------------- So this tracking bug tries to link those bugs together and evaluate the usefulness of the tool.

The cppcheck tool is available at http://cppcheck.sourceforge.net/ . The tool is run by david.volgyes@gmail.com and he is filing all the reports about potential bugs (as dependencies).

CCing :njn as suggested in bug 676179 comment #13.

Thank you for the creation of the meta-bug. However, I cannot modify it (adding new dependencies), so thank you for the new bug addition too. (I have just seen that my new bugs are assigned to this bug too.) By the way, I am moving from one country to another, so my answers and reports will come slowly, but I try my best. I will report bugs in the future, but with this pace it will take a while. I highly encourage you to run cppcheck on your own, and take a look on Debian's similar page: http://qa.debian.org/daca/ (These are usually easy bugs, good for beginners (like me).)

You can add new dependencies to it: when creating a new bug, select advanced form (not bugzilla helper, maybe you have to set it in your bugzilla preferences). Then click "show advanced fields". The field "Blocks" will appear and there you fill in this number 679417. Then fill in all other fields as needed when filing a new bug.

I'll go through the dependent bugs and mark relevant ones with [MemShrink]

Is there a reason we don't run this with our other tests on every check in (or even just every nth checkin).

Ehsan, you've been playing with static analysis stuff lately. What are your thoughts about running cppcheck in automation? Is it doable? Worthwhile? CCing some other folks who may be interested as well.

I have nothing against doing that, but I also am not very familiar with cppcheck and I don't know how many issues it can discover on trunk for example (is it only bug 1111288?!) and how many false positives it produces, etc. Someone who has some experience running this tool on our code base can probably answer better.

I'm not even sure who that would fall under. Nick?

I've never run this tool myself and don't claim any expertise. Bug 676179 comment 6 has some useful data.

I spent 5 minutes trying to learn how to use this tool and stopped when I realized that it's not a compiler drop-in replacement, so I'd have to teach it about include paths, compiler flags, and whatnot!

Hi, I reported the original bug around 3 years ago, so please, spend 5 more minutes with the problem. (I am sorry for the slow reply, but right now I travel until 2nd of January which means I am offline most of the time.) Cppcheck is not a compiler replacement, but actually it's very easy to use: cppcheck SOURCEPATH Yes, it can miss some bugs sometimes. If you give all the tiny details, all the include paths, etc., it could find more bugs. But even in this configuration-less state it finds dozens of bugs. E.g. int a; b=a; For this code snippet it does not matter the include path or anything else, it is a bug regardless of the context. It can find it. In similar way it finds a lot of bugs or potential bugs (e.g. if the file is not used in the final project, e.g. Makefile does not contain it.) So it checks the source tree for C/C++ codes and analysis it without any additional flags. The false positive rate is low, but of course it happens. So please, get any Linux distribution, install it (e.g. on ubuntu it is part of the standard repositories, so just "apt-get install cppcheck"), and try this line on the source tree: cppcheck -q SOURCEPATH (-q: quiet, it does not list anything except the bugs) And you can "grep" this for "leak", "mismatch" or similar terms. (E.g. one of the frequent problems in C++ projects: allocation with "new" and deallocation with "free", which does not cause segfault, but invalid, it skips the destructor.) You can later try the more advanced features, e.g. suppress false positives, choose C++ standard, or even xml output, when you can parse the output and automatically process it. E.g. some IDE have support for cppcheck and they can highlight the problematic lines, etc. (False positives: there is an out-of-bound check: accessing array element with invalid index. This is the weakest part of the test, most of the false positives are here. Try first the resource/memory leaks where the false positive rate is very low.) So yeah, I know it's different, it's not LLVM's scan-build, or clint or something similar, but please don't give up before you run it at least once on the source tree.

I have (obviously!) tried out cppcheck. Some observations: - It reported over 600 errors. - In a few cases there were dozens of repeats of the same error in a single file. - The false positive rate is really high. This is quite annoying. Some of them are because there are invariants that cppcheck cannot know about, and some are just because cppcheck can be a bit stupid. - I filed 11 bugs. Four or five of these are real defects. The rest are things that aren't actually a problem but the code is confusing or weird and worth changing. - There were some real defects that I haven't bothered filing bugs for. Some of these are unimportant (e.g. we leak memory just before we abort). Quite a few of them are in tests, and I haven't looked at these ones much so far. - A surprising number of the errors (both false and true positives) are in third-party code, especially ICU and WebRTC. A lot of the time false positives are caused by code that's not incorrect, but is a bit weird. I also haven't gone through all of these errors, simply because I haven't yet felt like taking the effort to work out how to report bugs upstream. - Overall it's good that a few real defects have been found, but I don't feel like running cppcheck regularly will give high value, because the false positive rate is so high. - I have a file containing the reported errors and my analysis of them. I haven't attached it because there are a couple of s-s bugs in there. I could redact those from it if people are interested.

(In reply to Nicholas Nethercote [:njn] from comment #13) > - A surprising number of the errors (both false and true positives) are in > third-party code, especially ICU and WebRTC. A lot of the time false > positives are caused by code that's not incorrect, but is a bit weird. I > also haven't gone through all of these errors, simply because I haven't yet > felt like taking the effort to work out how to report bugs upstream. From my experience looking at Coverity reports, a lot of the ICU problems are in the code that does stuff at build time, and not anything that we ship. They also used a few code patterns all over the place that were scary but I managed to convince myself were correct. For WebRTC, jesup should be able to tell you how to deal with upstream, or if you need to. There's a decent amount of files that we apply stuff to locally, I feel like.

I filed bug 1370292 to look into standing this up as a linter in CI. Things have progressed enough that doing so would now be fairly easy. As part of the investigation I also re-ran cppcheck on the tree, see: https://bug1370292.bmoattachments.org/attachment.cgi?id=8874498 Please comment over in bug 1370292 if you have a sense of how useful it would be.

Nothing for the last 4 years in this bug. And the finding of bug 1370357 shows that it isn't that interesting... Closing.