Opened 7 years ago

Closed 7 years ago

#2278 closed defect (fixed)

Errors from cppcheck 1.56

Reported by: vorner Owned by: jelte
Priority: medium Milestone: Sprint-20121009
Component: Unclassified Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DNS Feature Depending on Ticket:
Estimated Difficulty: 6 Add Hours to Ticket: 0
Total Hours: 0.36 Internal?: no

Description (last modified by naokikambe)

My system upgraded cppcheck to yet newer version 1.56, and again it provides me with a bunch of suggestions. This time there's a lot of them, so I don't see if a branch adds any new ones against master.

src/bin/auth/tests/query_unittest.cc:768: check_fail: Dereferenced iterator 'found_rrset' has been erased (error,eraseDereference)
src/bin/auth/tests/query_unittest.cc:835: check_fail: Dereferenced iterator 'nsec_it' has been erased (error,eraseDereference)
src/bin/auth/tests/query_unittest.cc:889: check_fail: Dereferenced iterator 'nsec_it' has been erased (error,eraseDereference)
:: check_fail: Skipping configuration 'IPV6_MTU' because it seems to be invalid. Use -D if you want to check it. (information,ConfigurationNotChecked)
:: check_fail: Skipping configuration 'IPV6_MTU_DISCOVER;IPV6_PMTUDISC_DONT' because it seems to be invalid. Use -D if you want to check it. (information,ConfigurationNotChecked)
:: check_fail: Skipping configuration 'IPV6_USE_MIN_MTU' because it seems to be invalid. Use -D if you want to check it. (information,ConfigurationNotChecked)
:: check_fail: Skipping configuration 'IPV6_RECVPKTINFO' because it seems to be invalid. Use -D if you want to check it. (information,ConfigurationNotChecked)
:: check_fail: Skipping configuration 'IP_PKTINFO' because it seems to be invalid. Use -D if you want to check it. (information,ConfigurationNotChecked)
src/lib/dhcp/tests/iface_mgr_unittest.cc:837: check_fail: Inefficient usage of string::find in condition; string::compare would be faster. (performance,stlIfStrFind)
src/lib/dns/benchmarks/message_renderer_bench.cc:42: check_fail: Member variable 'MessageRendererBenchMark::renderer_' is not initialized in the constructor. (warning,uninitMemberVar)
src/lib/dns/benchmarks/message_renderer_bench.cc:42: check_fail: Member variable 'MessageRendererBenchMark::names_' is not initialized in the constructor. (warning,uninitMemberVar)
src/lib/dns/benchmarks/rdatarender_bench.cc:44: check_fail: Member variable 'RdataRenderBenchMark::renderer_' is not initialized in the constructor. (warning,uninitMemberVar)
src/lib/dns/benchmarks/rdatarender_bench.cc:47: check_fail: Member variable 'RdataRenderBenchMark::dataset_' is not initialized in the constructor. (warning,uninitMemberVar)
src/lib/dns/benchmarks/rdatarender_bench.cc:47: check_fail: Member variable 'RdataRenderBenchMark::renderer_' is not initialized in the constructor. (warning,uninitMemberVar)
src/lib/dns/python/edns_python.cc:272: check_fail: The scope of the variable 'edns_obj' can be reduced (style,variableScope)
src/lib/dns/rdata/generic/afsdb_18.cc:60: check_fail: Unused variable: server (style,unusedVariable)
src/lib/dns/rdataclass.cc:607: check_fail: Unused variable: server (style,unusedVariable)
src/lib/dns/tests/rrparamregistry_unittest.cc:40: check_fail: Variable 'test_class_unknown_str' is assigned in constructor body. Consider performing initialization in initialization list. (performance,useInitializationList)
src/lib/log/compiler/message.cc:112: check_fail: Obsolete function 'ctime' called. It is recommended to use the function 'strftime' instead. (style,obsoleteFunctionsctime)
:: check_fail: Skipping configuration 'ENABLE_LOGGER_CHECKS;EXPECT_DEATH' because it seems to be invalid. Use -D if you want to check it. (information,ConfigurationNotChecked)
src/lib/log/tests/logger_example.cc:121: check_fail: Unused variable: options (style,unusedVariable)
:: check_fail: Skipping configuration 'EXPECT_DEATH' because it seems to be invalid. Use -D if you want to check it. (information,ConfigurationNotChecked)
:: check_fail: Skipping configuration 'CMSG_LEN' because it seems to be invalid. Use -D if you want to check it. (information,ConfigurationNotChecked)
:: check_fail: Skipping configuration 'CMSG_SPACE' because it seems to be invalid. Use -D if you want to check it. (information,ConfigurationNotChecked)
src/lib/dhcp/libdhcp++.cc:149: check_fail: The function 'OptionFactoryRegister' is never used (style,unusedFunction)
src/lib/python/isc/acl/dns.cc:93: check_fail: The function 'PyInit__dns' is never used (style,unusedFunction)
src/lib/python/isc/acl/acl.cc:50: check_fail: The function 'PyInit_acl' is never used (style,unusedFunction)
src/lib/python/isc/datasrc/datasrc.cc:257: check_fail: The function 'PyInit_datasrc' is never used (style,unusedFunction)
src/lib/util/io/fdshare_python.cc:64: check_fail: The function 'PyInit_libutil_io_python' is never used (style,unusedFunction)
src/lib/python/isc/log/log.cc:712: check_fail: The function 'PyInit_log' is never used (style,unusedFunction)
src/lib/dns/python/pydnspp.cc:770: check_fail: The function 'PyInit_pydnspp' is never used (style,unusedFunction)
src/lib/util/pyunittests/pyunittests_util.cc:82: check_fail: The function 'PyInit_pyunittests_util' is never used (style,unusedFunction)
src/lib/python/isc/util/cio/socketsession_python.cc:53: check_fail: The function 'PyInit_socketsession' is never used (style,unusedFunction)
src/lib/util/hash/sha1.cc:209: check_fail: The function 'SHA1FinalBits' is never used (style,unusedFunction)
src/bin/auth/tests/datasrc_configurator_unittest.cc:115: check_fail: The function 'SetUp' is never used (style,unusedFunction)
src/bin/auth/tests/datasrc_configurator_unittest.cc:97: check_fail: The function 'TearDown' is never used (style,unusedFunction)
src/bin/sockcreator/tests/sockcreator_tests.cc:109: check_fail: The function 'addressFamilySpecificCheck' is never used (style,unusedFunction)
src/lib/log/log_formatter.cc:57: check_fail: The function 'checkExcessPlaceholders' is never used (style,unusedFunction)
src/lib/datasrc/memory_datasrc.cc:448: check_fail: The function 'findNode' is never used (style,unusedFunction)
src/lib/nsas/tests/nameserver_address_unittest.cc:67: check_fail: The function 'getAddressesCount' is never used (style,unusedFunction)
src/lib/log/logger.cc:33: check_fail: The function 'initLoggerImpl' is never used (style,unusedFunction)
src/lib/util/tests/lru_list_unittest.cc:96: check_fail: The function 'invalidateIterator' is never used (style,unusedFunction)
src/lib/datasrc/memory/tests/treenode_rrset_unittest.cc:225: check_fail: The function 'isOutputTruncated<OutputBuffer>' is never used (style,unusedFunction)
src/lib/util/unittests/newhook.cc:37: check_fail: The function 'operatordelete' is never used (style,unusedFunction)
src/lib/util/unittests/newhook.cc:24: check_fail: The function 'operatornew' is never used (style,unusedFunction)
src/lib/asiolink/io_address.cc:93: check_fail: The function 'operatoruint32_t' is never used (style,unusedFunction)
src/lib/log/compiler/message.cc:379: check_fail: The function 'replaceNonAlphaNum' is never used (style,unusedFunction)
src/bin/sockcreator/tests/sockcreator_tests.cc:52: check_fail: The function 'setAddressFamilyFields' is never used (style,unusedFunction)
src/lib/util/tests/lru_list_unittest.cc:67: check_fail: The function 'setLruIterator' is never used (style,unusedFunction)
src/lib/datasrc/memory/tests/treenode_rrset_unittest.cc:215: check_fail: The function 'setOutputLengthLimit<OutputBuffer>' is never used (style,unusedFunction)
src/lib/resolve/recursive_query.cc:252: check_fail: The function 'setQuestion' is never used (style,unusedFunction)
src/lib/asiodns/tests/dns_server_unittest.cc:92: check_fail: The function 'setServerToStop' is never used (style,unusedFunction)
src/lib/resolve/tests/recursive_query_unittest.cc:697: check_fail: The function 'setSocketTimeout' is never used (style,unusedFunction)
src/lib/resolve/tests/recursive_query_unittest.cc:713: check_fail: The function 'tryRead' is never used (style,unusedFunction)
src/lib/cache/tests/rrset_entry_unittest.cc:53: check_fail: The function 'updateTTLForTest' is never used (style,unusedFunction)

Subtickets

Change History (10)

comment:1 Changed 7 years ago by naokikambe

  • Description modified (diff)

comment:2 Changed 7 years ago by muks

  • Estimated Difficulty changed from 0 to 6

comment:3 Changed 7 years ago by muks

  • Milestone changed from New Tasks to Sprint-20121009

Also look at:

cppcheck --enable=all --suppressions src/cppcheck-suppress.lst --inline-suppr  --quiet --error-exitcode=1  --template '{file}:{line}: check_fail: {message} ({severity},{id})'  src
src/lib/datasrc/memory/memory_client.cc:738: check_fail: Possible null pointer dereference: node (error,nullPointer)
src/lib/datasrc/memory/zone_table.cc:145: check_fail: Possible null pointer dereference: node (error,nullPointer)
*** Error code 1

Stop in /usr/home/jreed/builder/work/BIND10-cppcheck/20120917123001-FreeBSD8-amd64-GCC/build.

From here: http://git.bind10.isc.org/~tester/builder/BIND10-cppcheck/20120917123001-FreeBSD8-amd64-GCC/logs/cppcheck.out

comment:4 Changed 7 years ago by jelte

  • Owner set to jelte
  • Status changed from new to assigned

comment:5 Changed 7 years ago by jelte

  • Owner changed from jelte to UnAssigned
  • Status changed from assigned to reviewing

Ready for review.

There were a few real errors in there; mainly a destructor where the ~ was forgotten (making it a constructor...), and a few unused variables.

Less bad problems where use of an obsolete posix call (ctime(), in message.cc, replaced it with strftime).

Thirdly, there are also a number of warnings about some preprocessor directives (IPV6_MTU e.g.), not too sure what to do about them. But since they are not errors, at least the test would succeed again with the merger of this branch, if we upgrade to 1.56.

I also added two suppressions (yes, in cppcheck-suppress.lst);

  • eraseDereference. This is a known false positive which is fixed in the current development version of cppcheck
  • unusedFunction. Was less sure about this one, but we suddenly get a LOT of unused function errors. I've checked most of them, and some are somewhat false positives as well (in the case of templates, usually), and some are technically true, but probably hard to tell for cppcheck (PyINIT_XXX for instance). The latter could be 'fixed' by adding more headers, or early declarations. But neither are needed for the code to work, and I did not really want to put 50 cppcheck-suppress comments in non-test source code, so I opted to suppress them for now.

Both of these should of course be checked and possibly removed if we upgrade to > 1.56. Added a comment in the suppress file that says this.

comment:6 Changed 7 years ago by vorner

  • Owner changed from UnAssigned to vorner

comment:7 follow-up: Changed 7 years ago by vorner

  • Owner changed from vorner to jelte

Hello

The code changes use useful. But I'd have some questions.

You changed the thing around formatting time. Is the trim function used any more anywhere?

I'm not sure I understand the thing with unused functions. Are the functions used or not? Are they used in some strange way so cppcheck doesn't see them?

About the known false positives, is there a bug report on cppcheck or something?

Thank you

comment:8 in reply to: ↑ 7 ; follow-up: Changed 7 years ago by jelte

  • Owner changed from jelte to vorner

Replying to vorner:

You changed the thing around formatting time. Is the trim function used any more anywhere?

yeah, the message_reader still uses it, and util/filename appears to use it as well.

I'm not sure I understand the thing with unused functions. Are the functions used or not? Are they used in some strange way so cppcheck doesn't see them?

I've seen three different causes of reports

  1. Demonstrably false positives in the vicinity of templates; like
    src/lib/log/logger.cc:33: check_fail: The function 'initLoggerImpl' is never used (style,unusedFunction)
    src/bin/sockcreator/tests/sockcreator_tests.cc:52: check_fail: The function 'setAddressFamilyFields' is never used (style,unusedFunction)
    src/lib/datasrc/memory/tests/treenode_rrset_unittest.cc:215: check_fail: The function 'setOutputLengthLimit<OutputBuffer>' is never used (style,unusedFunction)
    

In these cases, it would seem cppcheck does not do templating well.

  1. Grey areas; mainly all the PyINIT functions, these are indeed never explicitely called, but are called dynamically by python magic.
  1. Ones that appear to be direct false positives, but I don't have any clue why these are flagged and others not;
    src/lib/util/hash/sha1.cc:209: check_fail: The function 'SHA1FinalBits' is never used (style,unusedFunction)
    src/bin/auth/tests/datasrc_configurator_unittest.cc:115: check_fail: The function 'SetUp' is never used (style,unusedFunction)
    src/bin/auth/tests/datasrc_configurator_unittest.cc:97: check_fail: The function 'TearDown' is never used (style,unusedFunction)
    

In some cases (at least the ones for 2 and most of the ones for 3), we could work around it by sticking them in a header, or adding an early declaration (which works around the report but would be useless functionality-wise). If we decide to do that we can probably add local suppressions for the rest.

About the known false positives, is there a bug report on cppcheck or something?

For the iterators there is:
http://sourceforge.net/apps/trac/cppcheck/ticket/4123

I did find several old reports about unused functions, but no new open ones, so I guess none for the latter, it's certainly not fixed in the current master branch of cppcheck.

comment:9 in reply to: ↑ 8 Changed 7 years ago by vorner

  • Owner changed from vorner to jelte
  • Total Hours changed from 0 to 0.36

Hello

Replying to jelte:

I did find several old reports about unused functions, but no new open ones, so I guess none for the latter, it's certainly not fixed in the current master branch of cppcheck.

OK, so let's leave the code as it is now. I think it can be merged.

But it might make sense to open a bug report at the cppcheck side.

comment:10 Changed 7 years ago by jelte

  • Resolution set to fixed
  • Status changed from reviewing to closed

thnx, merged, closing ticket

Note: See TracTickets for help on using tickets.