[libvirt] Coverity scan

[Michal Privoznik]

On 02.01.2013 20:22, John Ferlan wrote:
> On 12/20/2012 04:24 PM, Eric Blake wrote:
>> On 12/20/2012 08:25 AM, John Ferlan wrote:
>>>
>>> First allow me to introduce myself - I'm John Ferlan a new Red Hat employee (3 weeks).  I came from the closed world at HP where for the last 7 years I worked in a group developing/supporting HP's Integrity Virtual Machine software prior to it being outsourced to India this past May. I primarily worked in the CLI/API and daemon space, although I also spent quite a bit of time in the lower virtualization layers which mimicked the Integrity instructions. I am very happy to be in the open world and look forward to contributing.  Everyone has to start some where.
>>
>> Welcome to libvirt!  Based on recent list traffic, there are several
>> people, not just John, and not just at Red Hat, that are aiming to
>> provide initial contributions.  Be sure to provide your feedback and
>> questions on the list, and hopefully we can use that to make our hacking
>> documentation even easier to follow for future newcomers.
>>
>> You may want to figure out why your mailer doesn't automatically wrap
>> long lines.  Webmail interfaces (gmail, zimbra) and Microsoft Exchange
>> tend to be the worst mail agents when it comes to RFC compliance and
>> lack of knobs to tune for improving the situation, which in turn can
>> make it harder to read your messages and apply patches you submit.  You
>> may notice that many people on this list use mutt or Thunderbird rather
>> than web mailers, if only to have better formatted mail, but it's not a
>> hard pre-requisite (we won't reject a patch just because of how it was
>> sent, although it might take longer to apply if we have to jump through
>> hoops to get it extracted from the mail).  For mailers that have the
>> right knobs, turning on format=flowed is one way of sending reasonably
>> formatted mail [but be aware that Thunderbird has had a rather nasty bug
>> for several years now where defaulting to format=flowed can result in
>> botching the whitespace of the quoted portion of a message you are
>> replying to].
>>
>>>
>>> My first task here at Red Hat was to triage a Coverity scan executed against libvirt-1.0.0-1.fc19.src.rpm done in late November.  There were 285 issues documented. I quickly found that some of the defects found there were already fixed in later submits upstream, so I ran a new Coverity scan last Friday and it came back with 297 issues broken down as follows:
>>>
>>>      1 ARRAY_VS_SINGLETON
>>>     33 BAD_SIZEOF
>>>     17 CHECKED_RETURN
>>>      1 CONSTANT_EXPRESSION_RESULT
>>>      5 COPY_PASTE_ERROR
>>>     13 DEADCODE
>>>     46 FORWARD_NULL
>>>      2 MISSING_RETURN
>>>      2 NEGATIVE_RETURNS
>>>      7 NULL_RETURNS
>>>      1 OVERRUN
>>>    137 RESOURCE_LEAK
>>>     18 REVERSE_INULL
>>>      1 SIGN_EXTENSION
>>>      3 UNINIT
>>>      8 UNUSED_VALUE
>>>      2 USE_AFTER_FREE
>>
>> Thanks for doing this.  Helping to reduce the false positives and
>> eliminate the real bugs will make it easier to run Coverity on a
>> periodic basis and check for recent regressions while the code is still
>> fresh on our minds.  Looking forward to the patches you come up with.
>>
> 
> 
> Ran a new scan recently - the number of defects increased slightly (+4
> FORWARD_NULL, +2 MISSING_RETURN, and + 1 UNINIT).
> 
> I worked my way through the entire list and just figured I'd start with
> an easier group for my first submit attempt. I have a series of
> relatively easy changes for the "CHECKED_RETURN" condition; however, I
> was hoping to perhaps get some feedback on two files which had more
> ramifications from just checking the return status.
> 
> The first is 'src/phyp/phyp_driver.c'. Neither waitsocket() nor any of
> the callers check the return status. The implication being if select()
> fails the code will continue to wait. So is this "desired"? Conversely
> what are the ramifications of checking status on select() and using
> virReportSystemError()? Or perhaps some other way to log the failure?
> For any of the callers, any concerns over checking status return == -1
> and jumping to the err label? I guess I'm concerned over making
> logic/flow changes to some long standing assumption.  I've been on the
> opposite end of debugging one of those.

I think we should check for the return value of select() / waitsocket().
But I also think we can silently ignore EINTR. So the code should look
something like this:

errno = 0;
while (some_libssh2-ish_rubbish) {
    if (waitsocket(...) < 0 && errno != EINTR) {
        virReportSystemError(errno, "%s", _("unable to wait on libssh2 socket");
        goto err;
    }
}

> 
> The second is 'src/rpc/virnetsocket.c'.  The Coverity complaint is that
> the setsockopt() call to set SO_REUSEADDR doesn't check the return
> status. I think this particular case may be a cut-n-paste type change as
> the from virNetSocketNewListenTCP(). Unless I'm missing something, does
> setting the reuse addr on a connect socket do anything? Can it just
> safely be removed?

In fact on Linux (yet another difference from *BSD) it does make sense to set
SO_REUSEADDR even for outgoing connections, because on Linux, even outgoing
connection stays in TIME_WAIT state for some time. This means, port cannot be
reused at this time, unless reuse was set in both previous and current process.
On a machine with higher count of connections, port reusing can be handy then.
However, I don't think failing to set the bit is a fatal error. It's only
a hint for the kernel, so I'd just VIR_WARN() about it.

Michal