1370292 - Investigate whether adding a cppcheck linter would be useful

Status: RESOLVED WONTFIX

(Developer Infrastructure :: Lint and Formatting, enhancement, P4)

Description

[Andrew Halberstadt [:ahal]]

Attachment: errors.txt

http://cppcheck.sourceforge.net/
http://cppcheck.sourceforge.net/manual.pdf

Cppcheck is a static analysis tool that's designed to catch errors (e.g leaks, out of bounds, null pointer deref) rather than style/syntax. I'm filing this bug because it seems like it would be relatively simple to get running with mozlint, the hardest part would be bootstrapping it.

I ran cppcheck against the entire tree (output attached) using the following command which should exclude most 3rd party directories:
cppcheck $(sed -e 's/^/-i/' tools/rewriting/ThirdPartyPaths.txt) -ithird_party . 2> errors.txt

I have no idea how useful those errors are, nor if there are false positives mixed in.

If we did want to set this up, I'd make it opt-in at first, so it would only run on a given directory if a corresponding peer wanted it to. It could always be switched to opt-out at a later date if it proves to be useful.

Comment 1

[Sylvestre Ledru [:Sylvestre]]

We are currently working on integrating clang-tidy into the dev workflow (at review time). While I think clang-tidy has a brighter future than cppcheck, we could have a look about integrating it.
We are doing that job as part of releng services:
https://github.com/mozilla-releng/services

Comment 2

[Andrew Halberstadt [:ahal]]

Thanks, I did know about the clang-tidy effort. Cppcheck seems to explicitly not focus on style or syntax issues, and even recommends running something like clang-tidy in addition on their homepage. It's also worth noting that cppcheck is still very actively developed [1].

Unlike clang-tidy, cppcheck requires no compilation which means it is easy to add to the mozlint [2] infrastructure, so there would be no work required on your team's part. This might also make it a nice complement to clang-tidy for times when a developer just wants to do a quick check without doing a build.

(As an aside, if you end up working on getting lints like ESLint and flake8 hooked to the review process, please integrate mozlint instead and you'll get everything else for free).

[1] https://github.com/danmar/cppcheck/commits/master
[2] http://gecko.readthedocs.io/en/latest/tools/lint/index.html

Comment 3

[Eric Rahm [:erahm]]

Nick looked at this a few years back in the cppcheck bug you linked to, one of his conclusions in bug 679417, comment 13:

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

So yeah, opt-in is probably best, but arguably it's probably a larger time suck than it's worth.

Comment 4

[Andrew Halberstadt [:ahal]]

Good catch on that analysis. Yeah, the worst case scenario here is standing it up and no one wanting to use it because of a high false positive rate. Though there are a few things to consider:

1. Nick's analysis was 2 years ago, and the tool has had a lot of development in that time.
2. We can disable any rules that might be especially prone to false positives.
3. The time cost of standing this up is small.

Maybe I'll post the errors.txt to dev.platform and we can get a better idea of how useful/useless they are.

Comment 5

[Eric Rahm [:erahm]]

Sure it's possible it's gotten better! Nick's probably a better judge of that.

Comment 6

[Sylvestre Ledru [:Sylvestre]]

Could you share what kind of cppcheck checkers you think would be interesting currently?

Comment 7

[Nathan Froyd [:froydnj]]

(In reply to Andrew Halberstadt [:ahal] from comment #0)
> I ran cppcheck against the entire tree (output attached) using the following
> command which should exclude most 3rd party directories:
> cppcheck $(sed -e 's/^/-i/' tools/rewriting/ThirdPartyPaths.txt)
> -ithird_party . 2> errors.txt
> 
> I have no idea how useful those errors are, nor if there are false positives
> mixed in.

What version of the tree did you use?

I see an awful lot of "syntax error" in the output, which does not fill me with confidence in the robustness of the tool.

Comment 8

[Andrew Halberstadt [:ahal]]

(In reply to Sylvestre Ledru [:sylvestre] from comment #6)
> Could you share what kind of cppcheck checkers you think would be
> interesting currently?

Here's the full list:
https://sourceforge.net/p/cppcheck/wiki/ListOfChecks/

I confess I'm not proficient enough in C++ to make that determination, hence this bug. Would it be possible to compare the list in the attachment with what clang-tidy spits out? Wondering what (if anything) this is flagging that clang-tidy would have missed.

(In reply to Nathan Froyd [:froydnj] from comment #7)
> What version of the tree did you use?
> 
> I see an awful lot of "syntax error" in the output, which does not fill me
> with confidence in the robustness of the tool.

It was a recent central. It's possible I wasn't using the right standard (this was c++11), or maybe the text pre-processor is coming into play. But barring those, yeah.. it's fishy.

Comment 9

[Eric Rahm [:erahm]]

A quick perusal of the xpcom errors. TL;DR doesn't do much for xpcom. I think you're right that posting this to dev-platform would be most useful, it's possible others find value (and bugs) in the output.

[xpcom/base/Logging.cpp:283]: (error) va_list 'va' used before va_start() was called.
[xpcom/base/Logging.cpp:284]: (error) va_list 'va' used before va_start() was called.

FP. It doesn't seem to understand you can pass va_list as a function argument.

[xpcom/base/SystemMemoryReporter.cpp:740]: (error) Resource leak: sizeFile

FP. Doesn't understand that we're doing an early return because |sizeFile| is null.

[xpcom/base/nsTraceRefcnt.cpp:965]: (error) Resource leak: logFile

FP. Doesn't understand that we're doing an early return because |logFile| is null.

[xpcom/base/Logging.cpp:283]: (error) va_list 'va' used before va_start() was called.
[xpcom/base/Logging.cpp:284]: (error) va_list 'va' used before va_start() was called.

FP. See above. Why is it reporting this twice?

[xpcom/base/SystemMemoryReporter.cpp:740]: (error) Resource leak: sizeFile

ditto.

[xpcom/base/nsTraceRefcnt.cpp:965]: (error) Resource leak: logFile

ditto.

[xpcom/glue/tests/gtest/TestFileUtils.cpp:65]: (error) syntax error

Doesn't seem to be including the gtest headers?

[xpcom/io/Base64.cpp:290]: (error) Signed integer overflow for expression '(1073741823)*3'.
[xpcom/io/Base64.cpp:326]: (error) Signed integer overflow for expression '(1073741823)*3'.
[xpcom/io/Base64.cpp:611]: (error) Signed integer overflow for expression '(1073741823)*3'.

FP. MAX_UINT32 is unsigned, not signed.

[xpcom/reflect/xptcall/md/unix/xptcstubs_linux_m68k.cpp:66]: (error) Invalid number of character '(' when no macros are defined.

Legit. Presumably never compiled? This would be a compilation error.

[xpcom/tests/gtest/TestAtoms.cpp:46]: (error) syntax error
[xpcom/tests/gtest/TestCRT.cpp:78]: (error) syntax error
[xpcom/tests/gtest/TestExpirationTracker.cpp:177]: (error) syntax error
[xpcom/tests/gtest/TestRacingServiceManager.cpp:246]: (error) syntax error
[xpcom/tests/gtest/TestStateWatching.cpp:26]: (error) syntax error
[xpcom/tests/gtest/TestStrings.cpp:1305]: (error) syntax error
[xpcom/tests/gtest/TestTArray.cpp:97]: (error) syntax error
[xpcom/tests/gtest/TestTArray2.cpp:174]: (error) syntax error
[xpcom/tests/gtest/TestTaskQueue.cpp:16]: (error) syntax error
[xpcom/tests/gtest/TestThreadPoolListener.cpp:126]: (error) syntax error
[xpcom/tests/gtest/TestUTF.cpp:46]: (error) syntax error

Doesn't seem to be including the gtest headers?

Comment 10

[Nicholas Nethercote [inactive]]

I looked at the js/ ones:

[js/src/editline/editline.c:153]: (error) Common realloc mistake: 'Screen' nulle
d but not freed upon failure
    
True positive, but low-impact code.

[js/src/jit/arm/Simulator-arm.cpp:3527]: (error) Shifting 32-bit value by 32 bit
s is undefined behaviour
    
True positive.
    
[js/src/vm/Runtime.h:1204]: (error) Member variable 'lock' is initialized by its
elf.
    
False positive, due to name shadowing.    

[js/src/jscntxt.h:1257]: (error) Member variable 'cx' is initialized by itself.

False positive, due to name shadowing.

[js/src/jsapi-tests/testGCFinalizeCallback.cpp:121]: (error) Array index -1 is o
ut of bounds.

False positive.

[js/src/shell/OSObject.cpp:374]: (error) Resource leak: fp

False positive.

[js/src/wasm/WasmBaselineCompile.cpp:3100]: (error) Uninitialized variable: ool
[js/src/wasm/WasmBaselineCompile.cpp:3120]: (error) Uninitialized variable: ool

False positive, doesn't know about MOZ_CRASH.

[js/xpconnect/src/Sandbox.cpp:233]: (error) Memory leak: crypto
[js/xpconnect/src/Sandbox.cpp:248]: (error) Memory leak: registrar

False positives, I think.

[js/xpconnect/src/xpcprivate.h:1373]: (error) Array 'mInterfaces[1]' accessed at
 index 1, which is out of bounds.

This one's weird. I don't understand it.

Comment 11

[Nicholas Nethercote [inactive]]

Seems like the false positive rate is still high, so dealing with that is the primary issue. IIRC you can suppress errors with a specially formatted comment?

Still, I can imagine people getting annoyed -- and rightly so -- if their patch gets backed out because of a cppcheck error that turns out to be a false positive and then they have to add a special comment to get around it.

Comment 12

[Andrew Halberstadt [:ahal]]

Thanks for digging! It seems like the answer to this bug might still be "no".

I'll post the results to dev.platform to give people a chance to see if there are any other legit bugs, invite comments here and barring a vocal champion for, resolve this WONTFIX.

Comment 13

[Kartikaya Gupta (email:[e-mail])]

I looked at gfx/ - mostly false positives but one seemed correct. I filed bug 1370526 for it.

Comment 14

[Christoph Kerschbaumer [:ckerschb]]

Hey, thanks for putting in the effort. I took a quick look at dom/security - only false positives in that part of the tree. Anyway, always happy to manually look at a few false positives if it helps stability, security, quality overall.

Comment 15

[Steve Fink [:sfink] [:s:]]

(In reply to Nicholas Nethercote [:njn] from comment #10)
> I looked at the js/ ones:

I did too, before I saw njn's post here. I agree with all of his conclusions. Notes below:

> [js/src/jit/arm/Simulator-arm.cpp:3527]: (error) Shifting 32-bit value by 32
> bit
> s is undefined behaviour
>     
> True positive.

Yeah. The ARM instruction looks like it specifically allows shifting by 32, which means to sign-extend across the full word. I'm not sure what the difference is between an arithmetic right shift of 31 vs 32; seems like in both cases you fill the word with the sign bit.

> [js/xpconnect/src/xpcprivate.h:1373]: (error) Array 'mInterfaces[1]'
> accessed at
>  index 1, which is out of bounds.
> 
> This one's weird. I don't understand it.

It's a variable length array member of XPCNativeSet, which ends in

    // Always last - object sized for array.
    // These are strong references.
    XPCNativeInterface*     mInterfaces[1];

but the code asserts that you're not outside of the *real* bounds. Pedantically, it looks like this is a true positive: http://www.open-std.org/Jtc1/sc22/wg14/www/docs/dr_051.html

Practically, it is commonly done so compilers aren't going to break it.

Comment 16

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

I looked at netwerk/, all of them are true positive but with minor impact. filed bug 1372065 for the corresponding fix.

Comment 17

[Andrew Halberstadt [:ahal]]

I think given the lack of enthusiasm here and in the dev-platform post, plus the work Sylvestre's team has been doing with clang-tidy, this is safe to WONTFIX.