Hello,
there are results from code review which was done as part of our code-import
procedures. Opendnssec-1.4.6 was scanned with static code analyzers and it
uncovered couple interesting bugs (however I don't see any security problem in
the list).
Bugs below are split to sections per analyzer. Bugs are ordered inside each by
decreasing (subjective :-) importance.
Coverity
========
Results from Coverity static analyzer are at:
http://people.redhat.com/~pspacek/a/2014/09/30/opendnssec/coverity.html
This section describes my conclusions from brief analysis of the Coverity
report. You can use category name form first line to grep through the report.
LOCK (CWE-667)
Real problems in error handling paths. I guess that this could cause deadlock
or some similar problem.
FORWARD_NULL (CWE-476)
It seems like real problems at least in signer/src/wire/xfrd.c and
signer/src/signer/namedb.c.
USE_AFTER_FREE (CWE-825)
It seems like real problems at least in enforcer/enforcerd/enforcer.c and
signer/src/adapter/addns.c.
ATOMICITY (CWE-667)
Could indicate a real problem but I don't know enough about "task" management
to be sure.
RESOURCE_LEAK (CWE-772)
Seems like real resource leak at least in signer/src/signer/zone.c.
ARRAY_VS_SINGLETON (CWE-119)
REVERSE_INULL (CWE-476)
CHECKED_RETURN (CWE-252)
Mostly nitpicks.
UNREACHABLE (CWE-561)
Unreachable garbage code - could be commented out.
UNUSED_VALUE (CWE-563)
Set and never read variable - could be omitted.
DEADCODE (CWE-561)
NO_EFFECT (CWE-398)
False positive - probably caused by future-proofing (unused "default" cases in
switch statements etc.).
MISSING_LOCK (CWE-667)
Looks like false positives
CPPCHECK
========
Memory leak at least in libhsm/src/bin/hsmutil.c.
Operation on uninitialized variable at least in enforcer/ksm/ksm_keyword.c.
COMPILER
========
Assorted warnings. None of them seems like a huge issue.
I have quickly looked at "format not a string literal, format string not
checked" but it seems that these are auxiliary variables with constant values
assigned outside of the "print" function.
I would recommend you to add macro like this:
#ifdef __GNUC__
#define ODS_FORMAT_PRINTF(fmt, args) __attribute__((__format__(__printf__,
fmt, args)))
#else
#define ODS_FORMAT_PRINTF(fmt, args)
#endif
And annotate your own variants of "print" functions with it.
Clang
=====
Results from Clang static analyzer are at
http://people.redhat.com/~pspacek/a/2014/09/30/opendnssec/clang-scan-build-2014-09-30-155014-3764-1.tar.bz2
At first Clang produced huge amount of false positives. I have added patch
http://people.redhat.com/~pspacek/a/2014/09/30/opendnssec/log_attr.patch
which marks ods_fatal_exit() function as custom "assert". This lowered error
count from > 300 to 84.
I would be glad if you could include the patch in a next release. The
attribute can be enclosed in __GNUC__ ifdef if necessary.
The set of reported errors is not exactly the same as from Coverity. Further
analysis follows. I have categorized bugs according to classification in Clang
report.
- Argument with 'nonnull' attribute passed null
- Assigned value is garbage or undefined
- Dereference of null pointer
- Double free
Seems like bugs in error handling.
- Memory leak
Some real leaks (e.g. libhsm/src/lib/libhsm.c:3105) and some false positives
(e.g. libhsm/src/lib/libhsm.c:1801).
- Dead assignment
Some of them cause no harm (e.g. enforcer/utils/ksmutil.c:1503) but some
others (e.g. enforcer/enforcerd/enforcer.c:224) point to missing return value
checks after function calls.
- Allocator sizeof operand mismatch
Formal nitpicks.
- Result of operation is garbage or undefined
- Uninitialized argument value
False positives. This really surprises me because these checks are usually
pretty reliable! Apparently Clang has a problem with "status = MsgLog(status)"
semantics.
I hope this helps. Let me know if you are interested in similar bug reports in
future.
--
Petr Spacek @ Red Hat
_______________________________________________
Opendnssec-user mailing list
Opendnssec-user@lists.opendnssec.org
https://lists.opendnssec.org/mailman/listinfo/opendnssec-user
