[libvirt] [PATCH 0/3] Some coverity adjustments
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
Recent changes removed the virCapsPtr, but didn't adjust/remove the corresponding ATTRIBUTE_NONNULL resulting in a build failure to build in my Coverity environment. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/domain_conf.h | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 11fafe46b3..e012975fca 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -3089,27 +3089,24 @@ unsigned int virDomainDefFormatConvertXMLFlags(unsigned int flags); char *virDomainDefFormat(virDomainDefPtr def, virDomainXMLOptionPtr xmlopt, unsigned int flags) - ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) - ATTRIBUTE_NONNULL(3); + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); char *virDomainObjFormat(virDomainObjPtr obj, virDomainXMLOptionPtr xmlopt, unsigned int flags) - ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) - ATTRIBUTE_NONNULL(3); + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); int virDomainDefFormatInternal(virDomainDefPtr def, virDomainXMLOptionPtr xmlopt, virBufferPtr buf, unsigned int flags) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) - ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4); + ATTRIBUTE_NONNULL(3); int virDomainDefFormatInternalSetRootName(virDomainDefPtr def, virDomainXMLOptionPtr xmlopt, virBufferPtr buf, const char *rootname, unsigned int flags) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) - ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4) - ATTRIBUTE_NONNULL(5); + ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4); int virDomainDiskSourceFormat(virBufferPtr buf, virStorageSourcePtr src, @@ -3286,14 +3283,14 @@ int virDomainDefSave(virDomainDefPtr def, const char *configDir) G_GNUC_WARN_UNUSED_RESULT ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) - ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4); + ATTRIBUTE_NONNULL(3); int virDomainObjSave(virDomainObjPtr obj, virDomainXMLOptionPtr xmlopt, const char *statusDir) G_GNUC_WARN_UNUSED_RESULT ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) - ATTRIBUTE_NONNULL(3) ATTRIBUTE_NONNULL(4); + ATTRIBUTE_NONNULL(3); typedef void (*virDomainLoadConfigNotify)(virDomainObjPtr dom, int newDomain, -- 2.23.0
In the error path, if we xmlFreeNode @ret, then the return ret; a few lines later returns something that's already been free'd and could be reused, so let's reinit it. Found by Coverity Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/vbox/vbox_snapshot_conf.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/vbox/vbox_snapshot_conf.c b/src/vbox/vbox_snapshot_conf.c index 5a0abd6d0e..db6c389a64 100644 --- a/src/vbox/vbox_snapshot_conf.c +++ b/src/vbox/vbox_snapshot_conf.c @@ -352,6 +352,7 @@ virVBoxSnapshotConfCreateHardDiskNode(virVBoxSnapshotConfHardDiskPtr hardDisk) if (result < 0) { xmlUnlinkNode(ret); xmlFreeNode(ret); + ret = NULL; } VIR_FREE(uuid); return ret; -- 2.23.0
Make it clearer that what we're trying to do is find @source and @target_node so that the unattentive or code analysis utility doesn't believe 'source' and 'target' could be found in the same node element. Signed-off-by: John Ferlan <jferlan@redhat.com> --- tools/virsh-domain.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 56137bdd74..9d4cdd26dd 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -12476,10 +12476,9 @@ virshUpdateDiskXML(xmlNodePtr disk_node, if (tmp->type != XML_ELEMENT_NODE) continue; - if (virXMLNodeNameEqual(tmp, "source")) + if (!source && virXMLNodeNameEqual(tmp, "source")) source = tmp; - - if (virXMLNodeNameEqual(tmp, "target")) + else if (!target_node && virXMLNodeNameEqual(tmp, "target")) target_node = tmp; /* -- 2.23.0
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
participants (2)
-
John Ferlan -
Michal Prívozník