[libvirt] [PATCH 0/3] Some coverity adjustments

John Ferlan posted 3 patches 4 years, 4 months ago
Test syntax-check failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20191217203004.1279575-1-jferlan@redhat.com
src/conf/domain_conf.h        | 15 ++++++---------
src/vbox/vbox_snapshot_conf.c |  1 +
tools/virsh-domain.c          |  5 ++---
3 files changed, 9 insertions(+), 12 deletions(-)
[libvirt] [PATCH 0/3] Some coverity adjustments
Posted by John Ferlan 4 years, 4 months ago
I upgraded to f31 and it resulted in an essentially hosed Coverity
build/analysis environment with the following message during cov-emit
processing (a preprocessing of sorts):

"/usr/include/glib-2.0/glib/gspawn.h", line 76: error #67: expected a "}"
    G_SPAWN_ERROR_2BIG GLIB_DEPRECATED_ENUMERATOR_IN_2_32_FOR(G_SPAWN_ERROR_TOO_BIG) = G_SPAWN_ERROR_TOO_BIG,
                       ^

So instead, I'm using a guest to run Coverity "when I remember/can".

I also found that my f31 environment doesn't like building w/ docs as
I get the following messages while running the convert command:

...
usr/bin/mv: cannot stat '/tmp/magick-1191987h12h27ex0lZD.svg': No such file or directory
  GEN      kbase.html.tmp
convert: delegate failed `'uniconvertor' '%i' '%o.svg'; /usr/bin/mv '%o.svg' '%o'' @ error/delegate.c/InvokeDelegate/1958.
convert: unable to open file `/tmp/magick-1191987OqYJwrq8isaG': No such file or directory @ error/constitute.c/ReadImage/605.
convert: no images defined `migration-managed-p2p.png' @ error/convert.c/ConvertImageCommand/3235.
....

I haven't followed along as closely as I used to, but my vpath env
uses obj as a subdirectory of my main git tree/target. Whether the
new build env has anything to do with it or it's just f31, I haven't
been able to determine.


Beyond these 3 patches here - there is one other adjustment that is
necessary to build libvirt under Coverity and that's removing the
ATTRIBUTE_NONNULL(2) from the virDomainDefFormat definition in
src/conf/domain_conf.h.  This was added in commit 92d412149 which
also included two calls to virDomainDefFormat with NULL as the 2nd
argument (hyperv_driver.c and security_apparmor.c); however, the
commit message notes preparation for future work, so I'll keep a
hack for that local for now at least.

The virsh change below is innocuous yes, but it showed up in a
coverity analysis because it wasn't sure if the resulting variables
could point to the same address and if they did, then there was a
possible use after free because the @source is free'd even though
the @target_node is later referenced. The patch here avoids that
and provides a slight adjustment to not search for either node by
name if it was already found. Whether there's a weird latent issue
because <source> can be repeated while <target> cannot be is something
I suppose a reviewer can warn me about ;-).

John Ferlan (3):
  conf: Fix ATTRIBUTE_NONNULL usages
  vbox: Reset @ret after xmlFreeNode
  virsh: Adjust logic checks in virshUpdateDiskXML

 src/conf/domain_conf.h        | 15 ++++++---------
 src/vbox/vbox_snapshot_conf.c |  1 +
 tools/virsh-domain.c          |  5 ++---
 3 files changed, 9 insertions(+), 12 deletions(-)

-- 
2.23.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH 0/3] Some coverity adjustments
Posted by Michal Prívozník 4 years, 4 months ago
On 12/17/19 9:30 PM, John Ferlan wrote:
> I upgraded to f31 and it resulted in an essentially hosed Coverity
> build/analysis environment with the following message during cov-emit
> processing (a preprocessing of sorts):
> 
> "/usr/include/glib-2.0/glib/gspawn.h", line 76: error #67: expected a "}"
>     G_SPAWN_ERROR_2BIG GLIB_DEPRECATED_ENUMERATOR_IN_2_32_FOR(G_SPAWN_ERROR_TOO_BIG) = G_SPAWN_ERROR_TOO_BIG,
>                        ^
> 
> So instead, I'm using a guest to run Coverity "when I remember/can".
> 
> I also found that my f31 environment doesn't like building w/ docs as
> I get the following messages while running the convert command:
> 
> ...
> usr/bin/mv: cannot stat '/tmp/magick-1191987h12h27ex0lZD.svg': No such file or directory
>   GEN      kbase.html.tmp
> convert: delegate failed `'uniconvertor' '%i' '%o.svg'; /usr/bin/mv '%o.svg' '%o'' @ error/delegate.c/InvokeDelegate/1958.
> convert: unable to open file `/tmp/magick-1191987OqYJwrq8isaG': No such file or directory @ error/constitute.c/ReadImage/605.
> convert: no images defined `migration-managed-p2p.png' @ error/convert.c/ConvertImageCommand/3235.
> ....
> 
> I haven't followed along as closely as I used to, but my vpath env
> uses obj as a subdirectory of my main git tree/target. Whether the
> new build env has anything to do with it or it's just f31, I haven't
> been able to determine.
> 
> 
> Beyond these 3 patches here - there is one other adjustment that is
> necessary to build libvirt under Coverity and that's removing the
> ATTRIBUTE_NONNULL(2) from the virDomainDefFormat definition in
> src/conf/domain_conf.h.  This was added in commit 92d412149 which
> also included two calls to virDomainDefFormat with NULL as the 2nd
> argument (hyperv_driver.c and security_apparmor.c); however, the
> commit message notes preparation for future work, so I'll keep a
> hack for that local for now at least.

Well, obviously we pass NULL and at least in the apparmor case it is
valid (we don't need to format private data for devices), so I guess we
can remove the attribute, but I will let Dan chime in.

> 
> The virsh change below is innocuous yes, but it showed up in a
> coverity analysis because it wasn't sure if the resulting variables
> could point to the same address and if they did, then there was a
> possible use after free because the @source is free'd even though
> the @target_node is later referenced. The patch here avoids that
> and provides a slight adjustment to not search for either node by
> name if it was already found. Whether there's a weird latent issue
> because <source> can be repeated while <target> cannot be is something
> I suppose a reviewer can warn me about ;-).
> 
> John Ferlan (3):
>   conf: Fix ATTRIBUTE_NONNULL usages
>   vbox: Reset @ret after xmlFreeNode
>   virsh: Adjust logic checks in virshUpdateDiskXML
> 
>  src/conf/domain_conf.h        | 15 ++++++---------
>  src/vbox/vbox_snapshot_conf.c |  1 +
>  tools/virsh-domain.c          |  5 ++---
>  3 files changed, 9 insertions(+), 12 deletions(-)
> 

Reviewed-by: Michal Privoznik <mprivozn@redhat.com>

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list