[Dnsmasq-discuss] [dnsmasq] Errors found by static analysis of source code (Coverity)

[Simon Kelley]

On 04/02/13 10:24, Tomas Hozza wrote:
> Hello Simon.
>
> We at Red Hat are scanning a lot of open source packages
> with static analysis tool named Coverity. I have been scanning
> and reviewing group of network daemons where dnsmasq falls
> in, too.
>
> I scanned the latest dnsmasq-2.66-test13 source with Coverity
> version 6.5.1. It found 115 errors from which a lot of are just
> false positives or are not worth fixing. I wrote patches for
> issues that I think should be fixed. Please review and
> consider fixing these issues. I'm also including the Coverity
> scan log, so you can have a look at all errors.
>
> Coverity is also running a project where they allow open source
> project to be scanned for FREE. If you find it interesting
> you can find more information on http://scan.coverity.com/.
>

Many thanks for doing this, especially for writing the patches. Comments 
below.

0001-BUFFER_SIZE_WARNING-CWE-170.patch
I don't want to take this. It ensures that the last byte of the name 
field in struct ifreq is always zero before calling ioctl. The only
place this could overrun is in the kernel, and that does it's own checks 
(I looked). At least in theory, a kernel could accept a 16-character 
interface name, with all the bytes of the field valid characters. (Linux 
doesn't, it zeros the last byte before using the name.)

0002-CHECKED_RETURN-CWE-252.patch
Not sure I want this either: it checks for a vanishingly unlikely error 
return from fcntl on the log socket, and if it fails, it just logs the 
error, through the log socket. 	It doesn't log failure if the GETFL 
fnctl, just the SETFL one.

0003-CHECKED_RETURN-CWE-252.patch
0004-CHECKED_RETURN-CWE-252.patch
0005-CHECKED_RETURN-CWE-252.patch
0006-CHECKED_RETURN-CWE-252.patch
Same as above, but doesn't rely on the socket being manipulated to log 
the error at least. Will this ever happen, and is it worth carrying and 
translating the error message? I don't think so.

0007-CONSTANT_EXPRESSION_RESULT-CWE-569.patch
Good catch, taking that one, for sure.

0008-FORWARD_NULL-CWE-476.patch
Good too, I forget the qdcount == 0 case.

(Those two are in new code.)

less 0009-OVERRUN.patch
copy-and-paste fail! Taking that for sure. Old, old code too.

0010-RESOURCE_LEAK-CWE-404.patch
Tweaked fix, but accept principle.

0011-RESOURCE_LEAK-CWE-404.patch
Taken, but defence is that code-path leads to an abort anyway, so note 
freeing memory is of no consequence.

0012-RESOURCE_LEAK-CWE-404.patch
Good one, taken.

0013-RESOURCE_LEAK-CWE-404.patch
Not taken, note comment at head of function:
  /* NOTE: the memory used to return msg is leaked: use msgs in
     events only to describe fatal errors. */

0014-RESOURCE_LEAK-CWE-404.patch
I think the existing code is OK, if convoluted. If we opened a file, we 
keep the stream in daemon->leasestream, if we opened a pipe, 
daemon->leasestream stays NULL, and we pclose the pipe. Looks like the 
analyser doesn't know that pclose frees memory.

0015-RESOURCE_LEAK-CWE-404.patch
See 0013-RESOURCE_LEAK-CWE-404.patch

0016-RESOURCE_LEAK-CWE-404.patch
Leaks a single, fixed length string, probably shorter than the extra 
code to avoid it. Not taken.

0017-RESOURCE_LEAK-CWE-404.patch
Taken, good catch.




I'm out of time now, will check the rest tomorrow.


Cheers,

Simon.