[cppcheck] Full report

[21710675-501a-43e9-83a9-05bf05d9b741]

BPO	18521
Nosy	@loewis, @birkenfeld, @ronaldoussoren, @vstinner, @tiran, @ezio-melotti, @serhiy-storchaka
Files	cppcheck_reports.txt cppcheck_reports_filtered.txt cppcheck_reports_filtered_grouped.txt

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields: ```python assignee = None closed_at = created_at = labels = [] title = '[cppcheck] Full report' updated_at = user = 'https://bugs.python.org/serval2412' ``` bugs.python.org fields: ```python activity = actor = 'georg.brandl' assignee = 'none' closed = True closed_date = closer = 'georg.brandl' components = [] creation = creator = 'serval2412' dependencies = [] files = ['30995', '30996', '32108'] hgrepos = [] issue_num = 18521 keywords = [] message_count = 17.0 messages = ['193442', '193444', '193446', '193605', '193613', '193618', '194052', '194238', '199837', '199839', '199840', '199841', '199842', '199844', '199845', '199924', '199925'] nosy_count = 9.0 nosy_names = ['loewis', 'georg.brandl', 'ronaldoussoren', 'vstinner', 'christian.heimes', 'ezio.melotti', 'python-dev', 'serhiy.storchaka', 'serval2412'] pr_nums = [] priority = 'normal' resolution = 'fixed' stage = None status = 'closed' superseder = None type = None url = 'https://bugs.python.org/issue18521' versions = ['Python 3.4'] ```

[21710675-501a-43e9-83a9-05bf05d9b741]

Hello,

I retrieved Cpython sources today and runned cppcheck ("git updated" today) on the whole sources with enable=all I attached the full report. There are certainly false positive but some reports may help, eg: [Python/getargs.c:379]: (style) Array index 'i' is used before limits check. Indeed, here is the code: 379 while (levels[i] > 0 && i \< 32 && (int)(p-buf) \< 220) {

[Modules/md5module.c:345] -> [Modules/md5module.c:342]: (style) Found duplicate branches for 'if' and 'else' 342 if (Py_TYPE(self) == &MD5type) { 343 if ( (newobj = newMD5object())==NULL) 344 return NULL; 345 } else { 346 if ( (newobj = newMD5object())==NULL) 347 return NULL; 348 }

[Objects/iterobject.c:87]: (error) Uninitialized variable: seqsize [Objects/setobject.c:549]: (error) Address of local auto-variable assigned to a function parameter etc.

Hope it helps.

Julien

[tiran]

The report contains too much noise to be useful. I suggest that you remove some of the noise.

• get rid of "The scope of the variable 'foo' can be reduced.". It's nit-picking.

• ignore 3rd party code: Modules/_ctypes/libffi*, Modules/_sha3/keccak, Modules/expat, Modules/_decimal/libmpdec

• ignore Windows stuff on non-Windows platforms: PC, PCbuild, Tools/msi

[21710675-501a-43e9-83a9-05bf05d9b741]

Here's a new version of the file after some filtering (for the record, I used this command line: grep -v 'The scope' cppcheck_reports.txt |grep -v 'Modules/_ctypes/libffi' |grep -v Modules/_sha3/keccak| grep -v Modules/expat | grep -v Modules/_decimal/libmpdec |grep -v PC | grep -v msi | grep -v 'Skipping config' |grep -v 'Too many')

Is it ok for you?

Julien

[ronaldoussoren]

What is cppcheck?

The report contains some very dodgy false positives, like:

[Modules/_cursesmodule.c:1095]: (style) Variable 'rtn' is assigned a value that is never used. [Modules/_cursesmodule.c:1097]: (style) Unused variable: break

1) The "rtn" is used further on the function 2) "break" is not a variable at all, but a C keyword

There are also valid messages, but there appear to be a lot of false positives (based on the limited amount of checking that I've done)

[21710675-501a-43e9-83a9-05bf05d9b741]

quotation from http://en.wikipedia.org/wiki/Cppcheck : " Cppcheck is a static code analysis tool for the C and C++ programming languages " or from http://cppcheck.sourceforge.net/ " Cppcheck is a static analysis tool for C/C++ code. Unlike C/C++ compilers and many other analysis tools it does not detect syntax errors in the code. Cppcheck primarily detects the types of bugs that the compilers normally do not detect. The goal is to detect only real errors in the code (i.e. have zero false positives). "

[ronaldoussoren]

It doesn't really live up to its description.

From the start of the 'filtered' list:

• Messages about PyThread_acquire_lock appear to be false positives, that name is a function that's called by the macros expanded on those lines.

• Syntax error in _ctypes.h: the code is ugly, but is valid C; the message is about:

  else if PySlice_Check(item) {

  This is valid because PySlice_Check is a macro that expands into an expression with parentheses:

   #define PySlice_Check(op) (Py_TYPE(op) == &PySlice_Type)

That said, I would have used explicit parentheses here.

• unused variable in _ctypes_test.c: false positives, the variable is used in a function call two lines lower.

• callproc.c:1620: False positive because the tool doesn't know that PyMem_Malloc is a malloc function and returns unitialized memory (the first one that isn't a problem with the tool)

• messages about alloca: correct, I haven't looked seriously if the use of alloca is defensible here.

• callproc.c:751: False positive, the variable is used by some macros that are used in the function.

• the unused 'rtn' variable and 'break' in cursesmodule I've mentioned in a previous message.

I haven't checked the rest of the list, but so far I'm not impressed by this tool. That's too bad, static analysis tools can be helpful in improving code quality.

The high rate of false positives might be due to the preprocessor feature described in chapter 3 of the manual, the tool seems to be confused a lot by code that uses macros. Getting it to run properly on the CPython tools might therefore require significant tuning.

[61337411-43fc-4a9c-b8d5-4060aede66d0]

Julien: I propose to resolve this issue in the same way as we have done previously with analysis tools (with some unfortunate exceptions).

Somebody motivated enough (hopefully you) agrees to initially study the tool output, and ideally also then agrees to run the tool on a regular basis (but this is really not mandatory - the help with the initial run is already appreciated).

You would then filter out the reports, and see which of them are useful. If you can, have the tool silence the bogus reports (e.g. with a configuration file). For the issues that you consider valid, please file individual bug reports (possibly combining related reports, e.g. by module or by error kind, but only if they can all be reasonably fixed in a single commit).

The key concern of the developers here is probably this: a) there is no doubt that getting issues detected and fixed would be a helpful contribution, but b) the amount of false positives makes it unattractive to actually run the tool yourself.

If you do not want to volunteer, this is fine as well. Just don't feel sad if the issue gets closed with no action.

[21710675-501a-43e9-83a9-05bf05d9b741]

Thank you for your feedback, you can close this tracker.

[ezio-melotti]

I grouped the errors by message and checked of there were any actual errors. Most of the reports seemed wrong or false positive, but a few looked fixable. I haven't checked the "Unusued variable" ones, because they are harmless (even thought we could clean them up), and the "value assigned to a var that is never used" (I suspect most of these are to silence compiler warnings about unused return values). These two categories cover more than half of the errors.

I attach the file with the grouped content and some notes I took about the errors I looked at in case someone wants to check the remaining issues.

[1762cc99-3127-4a62-9baf-30c3d0f51ef7]

New changeset eaeaed43ff1c by Georg Brandl in branch 'default': Re bpo-18521: fix not-quite-C syntax that works only because the PyXXX_Check are macros defined with () around them. http://hg.python.org/cpython/rev/eaeaed43ff1c

[1762cc99-3127-4a62-9baf-30c3d0f51ef7]

New changeset 7396d10405db by Georg Brandl in branch 'default': Re bpo-18521: remove assignments of variables that are immediately reassigned. http://hg.python.org/cpython/rev/7396d10405db

[1762cc99-3127-4a62-9baf-30c3d0f51ef7]

New changeset 63bc2fe28a6e by Georg Brandl in branch 'default': Re bpo-18521: move array bounds check before array access. http://hg.python.org/cpython/rev/63bc2fe28a6e

[birkenfeld]

I fixed some items from Ezio's list that I think were legitimate; the rest is mostly invalid.

[serhiy-storchaka]

Shouldn't converttuple() in getargs.c set "levels[1] = 0;" after second "levels[0] = i+1;"?

[serhiy-storchaka]

Agree that all other looks false positive or unimportant.

[birkenfeld]

Shouldn't converttuple() in getargs.c set "levels[1] = 0;" after second "levels[0] = i+1;"?

I think it is fine, since convertitem() or converttuple() called from convertitem() will set their levels[0] (which is levels[1] in the original context) to zero on error.

[birkenfeld]

Closing this one as Fixed, then. Thanks everybody.