[Opendnssec-user] v1.4.6 static analysis results & discovered bugs

[Petr Spacek]

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