[libvirt] Coverity scan
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. 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 Of the defects found there are quite a few which can be considered as "false positives", some are trivial issues, a few complex issues, and the rest while resulting in a core usually occur in some error path. The bulk of the BAD_SIZEOF errors are the result of using a %p in the PROBE macro on structure pointers - it's a false positive, yet annoying. The bulk of FORWARD_NULL defects are from a false positive in vbox_templ.c. The bulk of RESOURCE_LEAK defects are from the use of macros to build code in esx_vi_types - which is where I'm triaging now. Of all the errors listed, "only" 62 files are affected. Over the next few weeks, I'll start sending patch requests starting with some of the trivial problems just so I can get my feet wet with the process as it's certainly different than my closed world experiences. Since part of that process is to communicate early so people know what you're doing and what's coming - that's what I'm doing! Also, now that I have a bit of experience with Coverity - I can run it again (more frequently) against the latest upstream bits. John Ferlan
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. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
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. 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? Tks, John
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
On Thu, Dec 20, 2012 at 10:25:04AM -0500, John Ferlan wrote:
Of the defects found there are quite a few which can be considered as "false positives", some are trivial issues, a few complex issues, and the rest while resulting in a core usually occur in some error path. The bulk of the BAD_SIZEOF errors are the result of using a %p in the PROBE macro on structure pointers - it's a false positive, yet annoying.
For this I would suggest contacting the systemtap developers mailing list and describing the issue & asking if there's anything they can do to sdt.h to work around the coverity trigger. Perhaps there's some kind of annotation to make coverity ignore it, or perhaps they can re-write their macro to avoid it. Daniel [1] http://sourceware.org/systemtap/getinvolved.html -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
participants (4)
-
Daniel P. Berrange -
Eric Blake -
John Ferlan -
Michal Privoznik