[libvirt] [PATCH 00/10] Resolve CHECKED_RETURN errors found by Coverity
Bug: https://bugzilla.redhat.com/show_bug.cgi?id=880388 This set of patches resolves the "CHECKED_RETURN (CWE-252)" errors found by Coverity. John Ferlan (10): interface: Check and handle error for virAsprintf() calls. parallels: Check and handle error for virAsprintf() calls. Ignore the return inparallelsMakePoolName() since subsequent check validates name was allocated. rpc: Check status when attempting to set SO_REUSEADDR flag on outgoing connection. On failure, VIR_WARN(), but continue to connect. vmware: Ignore the return status check for vmwareUpdateVMStatus in convenience routine vmwareDomainObjListUpdateDomain xen: Check return status for setting TCP_NODELAY option and generate a VIR_DEBUG message on failure. Allow connection to continue. virlockspacetest: Check return on mkdir for LOCKSPACE_DIR vmx2xmltest: Check and handle error for virAsprintf() calls. xml2vmxtest: Check and handle error for virAsprintf() calls. virsh: Ignore error returns for virBufferTrim(). phyp: Check and handle select() errors from waitsocket(). src/interface/interface_backend_udev.c | 5 ++-- src/parallels/parallels_storage.c | 5 ++-- src/phyp/phyp_driver.c | 42 ++++++++++++++++++++++++++-------- src/rpc/virnetsocket.c | 4 +++- src/vmware/vmware_driver.c | 2 +- src/xen/xend_internal.c | 6 +++-- tests/virlockspacetest.c | 3 ++- tests/vmx2xmltest.c | 7 ++++-- tests/xml2vmxtest.c | 5 ++-- tools/virsh.c | 4 ++-- 10 files changed, 56 insertions(+), 27 deletions(-) -- 1.7.11.7
--- src/interface/interface_backend_udev.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/interface/interface_backend_udev.c b/src/interface/interface_backend_udev.c index cc20b98..3231b73 100644 --- a/src/interface/interface_backend_udev.c +++ b/src/interface/interface_backend_udev.c @@ -654,9 +654,8 @@ udevIfaceGetIfaceDef(struct udev *udev, char *name) ifacedef->data.bridge.stp = stp; /* Members of the bridge */ - virAsprintf(&member_path, "%s/%s", - udev_device_get_syspath(dev), "brif"); - if (!member_path) { + if (virAsprintf(&member_path, "%s/%s", + udev_device_get_syspath(dev), "brif") < 0) { virReportOOMError(); goto cleanup; } -- 1.7.11.7
On Thu, Jan 03, 2013 at 02:16:13PM -0500, John Ferlan wrote:
--- src/interface/interface_backend_udev.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/src/interface/interface_backend_udev.c b/src/interface/interface_backend_udev.c index cc20b98..3231b73 100644 --- a/src/interface/interface_backend_udev.c +++ b/src/interface/interface_backend_udev.c @@ -654,9 +654,8 @@ udevIfaceGetIfaceDef(struct udev *udev, char *name) ifacedef->data.bridge.stp = stp;
/* Members of the bridge */ - virAsprintf(&member_path, "%s/%s", - udev_device_get_syspath(dev), "brif"); - if (!member_path) { + if (virAsprintf(&member_path, "%s/%s", + udev_device_get_syspath(dev), "brif") < 0) { virReportOOMError(); goto cleanup; }
ACK Daniel -- |: 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 :|
--- src/parallels/parallels_storage.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/parallels/parallels_storage.c b/src/parallels/parallels_storage.c index e768d88..2908bee 100644 --- a/src/parallels/parallels_storage.c +++ b/src/parallels/parallels_storage.c @@ -146,7 +146,7 @@ static char *parallelsMakePoolName(virConnectPtr conn, const char *path) if (i == 0) name = strdup(path); else - virAsprintf(&name, "%s-%u", path, i); + ignore_value(virAsprintf(&name, "%s-%u", path, i)); if (!name) { virReportOOMError(); @@ -310,8 +310,7 @@ static int parallelsAddDiskVolume(virStoragePoolObjPtr pool, if (VIR_ALLOC(def)) goto no_memory; - virAsprintf(&def->name, "%s-%s", dom->def->name, diskName); - if (!def->name) + if (virAsprintf(&def->name, "%s-%s", dom->def->name, diskName) < 0) goto no_memory; def->type = VIR_STORAGE_VOL_FILE; -- 1.7.11.7
NB, you should restrict the first line of the commit message to approx 70 characters. Then have a single blank line, followed by the longer description. This ensures that you get sensible subject lines :-) On Thu, Jan 03, 2013 at 02:16:14PM -0500, John Ferlan wrote:
--- src/parallels/parallels_storage.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/src/parallels/parallels_storage.c b/src/parallels/parallels_storage.c index e768d88..2908bee 100644 --- a/src/parallels/parallels_storage.c +++ b/src/parallels/parallels_storage.c @@ -146,7 +146,7 @@ static char *parallelsMakePoolName(virConnectPtr conn, const char *path) if (i == 0) name = strdup(path); else - virAsprintf(&name, "%s-%u", path, i); + ignore_value(virAsprintf(&name, "%s-%u", path, i));
if (!name) { virReportOOMError(); @@ -310,8 +310,7 @@ static int parallelsAddDiskVolume(virStoragePoolObjPtr pool, if (VIR_ALLOC(def)) goto no_memory;
- virAsprintf(&def->name, "%s-%s", dom->def->name, diskName); - if (!def->name) + if (virAsprintf(&def->name, "%s-%s", dom->def->name, diskName) < 0) goto no_memory;
def->type = VIR_STORAGE_VOL_FILE;
ACK Daniel -- |: 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 :|
On 01/03/2013 12:32 PM, Daniel P. Berrange wrote:
NB, you should restrict the first line of the commit message to approx 70 characters. Then have a single blank line, followed by the longer description. This ensures that you get sensible subject lines :-)
Also, using the same subject line across multiple patches can cause grief to backporting efforts. Inserting a prefix to say which portion of code is touched helps to both remove the ambiguity and make it easier to see at a glance why there are multiple patches. I've modified the commit message to: parallels: check and handle error for virAsprintf() calls Ignore the return in parallelsMakePoolName() since subsequent check validates name was allocated.
ACK
and I've now pushed 1 and 2, and will continue reading through the series. Congratulations on your first libvirt patch. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 01/03/2013 04:38 PM, Eric Blake wrote:
On 01/03/2013 12:32 PM, Daniel P. Berrange wrote:
NB, you should restrict the first line of the commit message to approx 70 characters. Then have a single blank line, followed by the longer description. This ensures that you get sensible subject lines :-)
Also, using the same subject line across multiple patches can cause grief to backporting efforts. Inserting a prefix to say which portion of code is touched helps to both remove the ambiguity and make it easier to see at a glance why there are multiple patches. I've modified the commit message to:
parallels: check and handle error for virAsprintf() calls
Ignore the return in parallelsMakePoolName() since subsequent check validates name was allocated.
I have learned my lesson about the longer lines. A mistake I hope to not make a second time! That certainly was one of those git things I had forgotten I knew, but since it was a few months since I had used git, it wasn't front and center of my consciousness. I was trying to figure out the 'norm' for those prefixes. I wasn't sure if they were automagically placed there, but once I started through the send-email process I realized those are self placed. Something perhaps for the contributing patches web/wiki page for us nubes.
ACK
and I've now pushed 1 and 2, and will continue reading through the series. Congratulations on your first libvirt patch.
Thanks!
--- src/rpc/virnetsocket.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c index ef93892..6684eef 100644 --- a/src/rpc/virnetsocket.c +++ b/src/rpc/virnetsocket.c @@ -470,7 +470,9 @@ int virNetSocketNewConnectTCP(const char *nodename, goto error; } - setsockopt(fd, SOL_SOCKET, SO_REUSEADDR, &opt, sizeof(opt)); + if (setsockopt(fd, SOL_SOCKET, SO_REUSEADDR, &opt, sizeof(opt)) < 0) { + VIR_WARN("Unable to enable port reuse"); + } if (connect(fd, runp->ai_addr, runp->ai_addrlen) >= 0) break; -- 1.7.11.7
On Thu, Jan 03, 2013 at 02:16:15PM -0500, John Ferlan wrote:
--- src/rpc/virnetsocket.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c index ef93892..6684eef 100644 --- a/src/rpc/virnetsocket.c +++ b/src/rpc/virnetsocket.c @@ -470,7 +470,9 @@ int virNetSocketNewConnectTCP(const char *nodename, goto error; }
- setsockopt(fd, SOL_SOCKET, SO_REUSEADDR, &opt, sizeof(opt)); + if (setsockopt(fd, SOL_SOCKET, SO_REUSEADDR, &opt, sizeof(opt)) < 0) { + VIR_WARN("Unable to enable port reuse"); + }
if (connect(fd, runp->ai_addr, runp->ai_addrlen) >= 0) break;
Hmm, not sure I agree with this. If this is something that should not occurr, then we should virReportError. If it is something we expect to occur, then VIR_WARN will annoy people with irrelevant messages. My inclination is to treat it as a fatal error Daniel -- |: 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 :|
On 01/03/2013 02:34 PM, Daniel P. Berrange wrote:
On Thu, Jan 03, 2013 at 02:16:15PM -0500, John Ferlan wrote:
--- src/rpc/virnetsocket.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c index ef93892..6684eef 100644 --- a/src/rpc/virnetsocket.c +++ b/src/rpc/virnetsocket.c @@ -470,7 +470,9 @@ int virNetSocketNewConnectTCP(const char *nodename, goto error; }
- setsockopt(fd, SOL_SOCKET, SO_REUSEADDR, &opt, sizeof(opt)); + if (setsockopt(fd, SOL_SOCKET, SO_REUSEADDR, &opt, sizeof(opt)) < 0) { + VIR_WARN("Unable to enable port reuse"); + }
if (connect(fd, runp->ai_addr, runp->ai_addrlen) >= 0) break;
Hmm, not sure I agree with this. If this is something that should not occurr, then we should virReportError. If it is something we expect to occur, then VIR_WARN will annoy people with irrelevant messages.
I asked about this yesterday and Michal P responded. The REUSEADDR is a more of a hint for connections, see the end of: https://www.redhat.com/archives/libvir-list/2013-January/msg00064.html I don't mind either way.
My inclination is to treat it as a fatal error
Daniel
On 03.01.2013 20:34, Daniel P. Berrange wrote:
On Thu, Jan 03, 2013 at 02:16:15PM -0500, John Ferlan wrote:
--- src/rpc/virnetsocket.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c index ef93892..6684eef 100644 --- a/src/rpc/virnetsocket.c +++ b/src/rpc/virnetsocket.c @@ -470,7 +470,9 @@ int virNetSocketNewConnectTCP(const char *nodename, goto error; }
- setsockopt(fd, SOL_SOCKET, SO_REUSEADDR, &opt, sizeof(opt)); + if (setsockopt(fd, SOL_SOCKET, SO_REUSEADDR, &opt, sizeof(opt)) < 0) { + VIR_WARN("Unable to enable port reuse"); + }
if (connect(fd, runp->ai_addr, runp->ai_addrlen) >= 0) break;
Hmm, not sure I agree with this. If this is something that should not occurr, then we should virReportError. If it is something we expect to occur, then VIR_WARN will annoy people with irrelevant messages.
My inclination is to treat it as a fatal error
Daniel
I don't think so. This is on the sender side where REUSEADDR is just a hint and really takes effect if the sender is overflowed with TCP connections. Inability to set this, doesn't necessary mean we will fail to establish new connection. IOW it's not a show stopper. If this was on the receiver side, the scenario would be completely different. Michal
--- src/vmware/vmware_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/vmware/vmware_driver.c b/src/vmware/vmware_driver.c index 3763f40..9c81df8 100644 --- a/src/vmware/vmware_driver.c +++ b/src/vmware/vmware_driver.c @@ -949,7 +949,7 @@ static void vmwareDomainObjListUpdateDomain(void *payload, const void *name ATTR struct vmware_driver *driver = data; virDomainObjPtr vm = payload; virDomainObjLock(vm); - vmwareUpdateVMStatus(driver, vm); + ignore_value(vmwareUpdateVMStatus(driver, vm)); virDomainObjUnlock(vm); } -- 1.7.11.7
On 01/03/2013 12:16 PM, John Ferlan wrote:
--- src/vmware/vmware_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/vmware/vmware_driver.c b/src/vmware/vmware_driver.c index 3763f40..9c81df8 100644 --- a/src/vmware/vmware_driver.c +++ b/src/vmware/vmware_driver.c @@ -949,7 +949,7 @@ static void vmwareDomainObjListUpdateDomain(void *payload, const void *name ATTR struct vmware_driver *driver = data; virDomainObjPtr vm = payload; virDomainObjLock(vm); - vmwareUpdateVMStatus(driver, vm); + ignore_value(vmwareUpdateVMStatus(driver, vm));
ACK, I'll reformat the commit message and push this one soon. As for 3/10, it looks like there is still a discussion going on as to whether to warn or silently ignore failure to set the hint, so I deferred pushing that one for now. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
--- src/xen/xend_internal.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c index 84a25e8..1f779b0 100644 --- a/src/xen/xend_internal.c +++ b/src/xen/xend_internal.c @@ -91,8 +91,10 @@ do_connect(virConnectPtr xend) /* * try to desactivate slow-start */ - setsockopt(s, IPPROTO_TCP, TCP_NODELAY, (void *)&no_slow_start, - sizeof(no_slow_start)); + if (setsockopt(s, IPPROTO_TCP, TCP_NODELAY, (void *)&no_slow_start, + sizeof(no_slow_start)) < 0) { + VIR_DEBUG("Unable to disable nagle algorithm"); + } if (connect(s, (struct sockaddr *)&priv->addr, priv->addrlen) == -1) { -- 1.7.11.7
On Thu, Jan 03, 2013 at 02:16:17PM -0500, John Ferlan wrote:
--- src/xen/xend_internal.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c index 84a25e8..1f779b0 100644 --- a/src/xen/xend_internal.c +++ b/src/xen/xend_internal.c @@ -91,8 +91,10 @@ do_connect(virConnectPtr xend) /* * try to desactivate slow-start */ - setsockopt(s, IPPROTO_TCP, TCP_NODELAY, (void *)&no_slow_start, - sizeof(no_slow_start)); + if (setsockopt(s, IPPROTO_TCP, TCP_NODELAY, (void *)&no_slow_start, + sizeof(no_slow_start)) < 0) { + VIR_DEBUG("Unable to disable nagle algorithm"); + }
Hmm, I'd just make this ignore_value() i think - it is something we expect to fail with UNIX sockets Daniel -- |: 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 :|
On 01/03/2013 02:36 PM, Daniel P. Berrange wrote:
On Thu, Jan 03, 2013 at 02:16:17PM -0500, John Ferlan wrote:
--- src/xen/xend_internal.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c index 84a25e8..1f779b0 100644 --- a/src/xen/xend_internal.c +++ b/src/xen/xend_internal.c @@ -91,8 +91,10 @@ do_connect(virConnectPtr xend) /* * try to desactivate slow-start */ - setsockopt(s, IPPROTO_TCP, TCP_NODELAY, (void *)&no_slow_start, - sizeof(no_slow_start)); + if (setsockopt(s, IPPROTO_TCP, TCP_NODELAY, (void *)&no_slow_start, + sizeof(no_slow_start)) < 0) { + VIR_DEBUG("Unable to disable nagle algorithm"); + }
Hmm, I'd just make this ignore_value() i think - it is something we expect to fail with UNIX sockets
Daniel
Comparing this to src/rpc/virnetsocket.c which also uses TCP_NODELAY and errors out I figured that in this case having a debug message may be useful if someone was debugging a problem, but I have no other argument for not using ignore_value(). Going back also allows me to fix the comment. John
--- src/xen/xend_internal.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c index 84a25e8..01d317c 100644 --- a/src/xen/xend_internal.c +++ b/src/xen/xend_internal.c @@ -89,11 +89,10 @@ do_connect(virConnectPtr xend) } /* - * try to desactivate slow-start + * try to deactivate slow-start */ - setsockopt(s, IPPROTO_TCP, TCP_NODELAY, (void *)&no_slow_start, - sizeof(no_slow_start)); - + ignore_value(setsockopt(s, IPPROTO_TCP, TCP_NODELAY, (void *)&no_slow_start, + sizeof(no_slow_start))); if (connect(s, (struct sockaddr *)&priv->addr, priv->addrlen) == -1) { VIR_FORCE_CLOSE(s); /* preserves errno */ -- 1.7.11.7
On 01/08/2013 10:40 AM, John Ferlan wrote:
--- src/xen/xend_internal.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c index 84a25e8..01d317c 100644 --- a/src/xen/xend_internal.c +++ b/src/xen/xend_internal.c @@ -89,11 +89,10 @@ do_connect(virConnectPtr xend) }
/* - * try to desactivate slow-start + * try to deactivate slow-start */ - setsockopt(s, IPPROTO_TCP, TCP_NODELAY, (void *)&no_slow_start, - sizeof(no_slow_start)); - + ignore_value(setsockopt(s, IPPROTO_TCP, TCP_NODELAY, (void *)&no_slow_start, + sizeof(no_slow_start)));
I've gone ahead and pushed this one. It seems like you have quite a few outstanding patches that are pending review; it may help to post a single message in a new thread (not buried in an existing thread) with ULRs to list archives of which messages still need some attention; https://www.redhat.com/archives/libvir-list/2013-January/thread.html may be a useful starting point. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
--- tests/virlockspacetest.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/virlockspacetest.c b/tests/virlockspacetest.c index 8673700..7678396 100644 --- a/tests/virlockspacetest.c +++ b/tests/virlockspacetest.c @@ -293,7 +293,8 @@ static int testLockSpaceResourceLockPath(const void *args ATTRIBUTE_UNUSED) lockspace = virLockSpaceNew(NULL); - mkdir(LOCKSPACE_DIR, 0700); + if (mkdir(LOCKSPACE_DIR, 0700) < 0) + goto cleanup; if (virLockSpaceCreateResource(lockspace, LOCKSPACE_DIR "/foo") < 0) goto cleanup; -- 1.7.11.7
On Thu, Jan 03, 2013 at 02:16:18PM -0500, John Ferlan wrote:
--- tests/virlockspacetest.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/tests/virlockspacetest.c b/tests/virlockspacetest.c index 8673700..7678396 100644 --- a/tests/virlockspacetest.c +++ b/tests/virlockspacetest.c @@ -293,7 +293,8 @@ static int testLockSpaceResourceLockPath(const void *args ATTRIBUTE_UNUSED)
lockspace = virLockSpaceNew(NULL);
- mkdir(LOCKSPACE_DIR, 0700); + if (mkdir(LOCKSPACE_DIR, 0700) < 0) + goto cleanup;
if (virLockSpaceCreateResource(lockspace, LOCKSPACE_DIR "/foo") < 0) goto cleanup;
ACK Daniel -- |: 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 :|
--- tests/vmx2xmltest.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/tests/vmx2xmltest.c b/tests/vmx2xmltest.c index 03a8989..2430350 100644 --- a/tests/vmx2xmltest.c +++ b/tests/vmx2xmltest.c @@ -179,7 +179,9 @@ testParseVMXFileName(const char *fileName, void *opaque ATTRIBUTE_UNUSED) goto cleanup; } - virAsprintf(&src, "[%s] %s", datastoreName, directoryAndFileName); + if (virAsprintf(&src, "[%s] %s", datastoreName, + directoryAndFileName) < 0) + goto cleanup; } else if (STRPREFIX(fileName, "/")) { /* Found absolute path referencing a file outside a datastore */ src = strdup(fileName); @@ -188,7 +190,8 @@ testParseVMXFileName(const char *fileName, void *opaque ATTRIBUTE_UNUSED) src = NULL; } else { /* Found single file name referencing a file inside a datastore */ - virAsprintf(&src, "[datastore] directory/%s", fileName); + if (virAsprintf(&src, "[datastore] directory/%s", fileName) < 0) + goto cleanup; } cleanup: -- 1.7.11.7
On Thu, Jan 03, 2013 at 02:16:19PM -0500, John Ferlan wrote:
--- tests/vmx2xmltest.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/tests/vmx2xmltest.c b/tests/vmx2xmltest.c index 03a8989..2430350 100644 --- a/tests/vmx2xmltest.c +++ b/tests/vmx2xmltest.c @@ -179,7 +179,9 @@ testParseVMXFileName(const char *fileName, void *opaque ATTRIBUTE_UNUSED) goto cleanup; }
- virAsprintf(&src, "[%s] %s", datastoreName, directoryAndFileName); + if (virAsprintf(&src, "[%s] %s", datastoreName, + directoryAndFileName) < 0) + goto cleanup; } else if (STRPREFIX(fileName, "/")) { /* Found absolute path referencing a file outside a datastore */ src = strdup(fileName); @@ -188,7 +190,8 @@ testParseVMXFileName(const char *fileName, void *opaque ATTRIBUTE_UNUSED) src = NULL; } else { /* Found single file name referencing a file inside a datastore */ - virAsprintf(&src, "[datastore] directory/%s", fileName); + if (virAsprintf(&src, "[datastore] directory/%s", fileName) < 0) + goto cleanup; }
cleanup:
ACK Daniel -- |: 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 :|
--- tests/xml2vmxtest.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tests/xml2vmxtest.c b/tests/xml2vmxtest.c index 653ab6c..c578109 100644 --- a/tests/xml2vmxtest.c +++ b/tests/xml2vmxtest.c @@ -193,8 +193,9 @@ testFormatVMXFileName(const char *src, void *opaque ATTRIBUTE_UNUSED) directoryAndFileName += strspn(directoryAndFileName, " "); } - virAsprintf(&absolutePath, "/vmfs/volumes/%s/%s", datastoreName, - directoryAndFileName); + if (virAsprintf(&absolutePath, "/vmfs/volumes/%s/%s", datastoreName, + directoryAndFileName) < 0) + goto cleanup; } else if (STRPREFIX(src, "/")) { /* Found absolute path */ absolutePath = strdup(src); -- 1.7.11.7
On Thu, Jan 03, 2013 at 02:16:20PM -0500, John Ferlan wrote:
--- tests/xml2vmxtest.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/tests/xml2vmxtest.c b/tests/xml2vmxtest.c index 653ab6c..c578109 100644 --- a/tests/xml2vmxtest.c +++ b/tests/xml2vmxtest.c @@ -193,8 +193,9 @@ testFormatVMXFileName(const char *src, void *opaque ATTRIBUTE_UNUSED) directoryAndFileName += strspn(directoryAndFileName, " "); }
- virAsprintf(&absolutePath, "/vmfs/volumes/%s/%s", datastoreName, - directoryAndFileName); + if (virAsprintf(&absolutePath, "/vmfs/volumes/%s/%s", datastoreName, + directoryAndFileName) < 0) + goto cleanup; } else if (STRPREFIX(src, "/")) { /* Found absolute path */ absolutePath = strdup(src);
ACK Daniel -- |: 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 :|
On 01/03/2013 12:37 PM, Daniel P. Berrange wrote:
On Thu, Jan 03, 2013 at 02:16:20PM -0500, John Ferlan wrote:
--- tests/xml2vmxtest.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/tests/xml2vmxtest.c b/tests/xml2vmxtest.c index 653ab6c..c578109 100644 --- a/tests/xml2vmxtest.c +++ b/tests/xml2vmxtest.c @@ -193,8 +193,9 @@ testFormatVMXFileName(const char *src, void *opaque ATTRIBUTE_UNUSED) directoryAndFileName += strspn(directoryAndFileName, " "); }
- virAsprintf(&absolutePath, "/vmfs/volumes/%s/%s", datastoreName, - directoryAndFileName); + if (virAsprintf(&absolutePath, "/vmfs/volumes/%s/%s", datastoreName, + directoryAndFileName) < 0) + goto cleanup; } else if (STRPREFIX(src, "/")) { /* Found absolute path */ absolutePath = strdup(src);
ACK
I'm squashing 7 and 8 into one patch, since they both touch the tests/ directory. I've now pushed 6-8 (Dan asked for a v2 of patch 5). -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
--- tools/virsh.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index 283194a..9f834e4 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -544,7 +544,7 @@ vshTreePrintInternal(vshControl *ctl, false, indent) < 0) goto cleanup; } - virBufferTrim(indent, " ", -1); + ignore_value(virBufferTrim(indent, " ", -1)); /* If there was no child device, and we're the last in * a list of devices, then print another blank line */ @@ -552,7 +552,7 @@ vshTreePrintInternal(vshControl *ctl, vshPrint(ctl, "%s\n", virBufferCurrentContent(indent)); if (!root) - virBufferTrim(indent, NULL, 2); + ignore_value(virBufferTrim(indent, NULL, 2)); ret = 0; cleanup: return ret; -- 1.7.11.7
On Thu, Jan 03, 2013 at 02:16:21PM -0500, John Ferlan wrote:
--- tools/virsh.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tools/virsh.c b/tools/virsh.c index 283194a..9f834e4 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -544,7 +544,7 @@ vshTreePrintInternal(vshControl *ctl, false, indent) < 0) goto cleanup; } - virBufferTrim(indent, " ", -1); + ignore_value(virBufferTrim(indent, " ", -1));
/* If there was no child device, and we're the last in * a list of devices, then print another blank line */ @@ -552,7 +552,7 @@ vshTreePrintInternal(vshControl *ctl, vshPrint(ctl, "%s\n", virBufferCurrentContent(indent));
if (!root) - virBufferTrim(indent, NULL, 2); + ignore_value(virBufferTrim(indent, NULL, 2)); ret = 0; cleanup: return ret;
I must say I don't see the point in the return value for virBufferTrim. I think I'd recommend turning it into a 'void' function Daniel -- |: 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 :|
On 01/03/2013 12:39 PM, Daniel P. Berrange wrote:
On Thu, Jan 03, 2013 at 02:16:21PM -0500, John Ferlan wrote:
--- tools/virsh.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
I must say I don't see the point in the return value for virBufferTrim. I think I'd recommend turning it into a 'void' function
Ah, but virbuftest.c exposes why a return value can be useful - it lets you know how much was trimmed, or if trimming was prevented because the string didn't end with the suffix you expect. Making the function 'void' would require rewriting the test, and losing out on that functionality. Just because all current callers outside of the testsuite don't use that functionality (virnetsshsession.c, virsh.c) doesn't mean we should necessarily get rid of it - is there any alternative way to shut up Coverity to say that we can safely ignore this return value without having to mark up all the callers? -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 01/04/2013 12:50 PM, Eric Blake wrote:
On 01/03/2013 12:39 PM, Daniel P. Berrange wrote:
On Thu, Jan 03, 2013 at 02:16:21PM -0500, John Ferlan wrote:
--- tools/virsh.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
I must say I don't see the point in the return value for virBufferTrim. I think I'd recommend turning it into a 'void' function
Ah, but virbuftest.c exposes why a return value can be useful - it lets you know how much was trimmed, or if trimming was prevented because the string didn't end with the suffix you expect. Making the function 'void' would require rewriting the test, and losing out on that functionality. Just because all current callers outside of the testsuite don't use that functionality (virnetsshsession.c, virsh.c) doesn't mean we should necessarily get rid of it - is there any alternative way to shut up Coverity to say that we can safely ignore this return value without having to mark up all the callers?
Coverity allows one to place a comment in the code prior to the call to signify to the analysis engine to ignore a particular issue or a set of particular issues, such as in these cases: /* coverity[check_return] */ I already have a list of those starting in a local branch. In this particular case I chose ignore_value because the code is merely removing what was added to indent in prior lines. For the "(!root)" case if we failed to add, then the code jumps to cleanup. I suppose an argument could be made that if either fails we could print some sort of error, but it just seemed unnecessary since I believe whatever was going to be printed was already printed. The indent is only used to create a bit more beatification it seems. John
--- src/phyp/phyp_driver.c | 42 ++++++++++++++++++++++++++++++++---------- 1 file changed, 32 insertions(+), 10 deletions(-) diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c index 8e26b0c..2dabd19 100644 --- a/src/phyp/phyp_driver.c +++ b/src/phyp/phyp_driver.c @@ -76,7 +76,6 @@ static int waitsocket(int socket_fd, LIBSSH2_SESSION * session) { struct timeval timeout; - int rc; fd_set fd; fd_set *writefd = NULL; fd_set *readfd = NULL; @@ -98,9 +97,7 @@ waitsocket(int socket_fd, LIBSSH2_SESSION * session) if (dir & LIBSSH2_SESSION_BLOCK_OUTBOUND) writefd = &fd; - rc = select(socket_fd + 1, readfd, writefd, NULL, &timeout); - - return rc; + return select(socket_fd + 1, readfd, writefd, NULL, &timeout); } /* this function is the layer that manipulates the ssh channel itself @@ -131,7 +128,11 @@ phypExec(LIBSSH2_SESSION *session, const char *cmd, int *exit_status, while ((channel = libssh2_channel_open_session(session)) == NULL && libssh2_session_last_error(session, NULL, NULL, 0) == LIBSSH2_ERROR_EAGAIN) { - waitsocket(sock, session); + if (waitsocket(sock, session) < 0 && errno != EINTR) { + virReportSystemError(errno, "%s", + _("unable to wait on libssh2 socket")); + goto err; + } } if (channel == NULL) { @@ -140,7 +141,11 @@ phypExec(LIBSSH2_SESSION *session, const char *cmd, int *exit_status, while ((rc = libssh2_channel_exec(channel, cmd)) == LIBSSH2_ERROR_EAGAIN) { - waitsocket(sock, session); + if (waitsocket(sock, session) < 0 && errno != EINTR) { + virReportSystemError(errno, "%s", + _("unable to wait on libssh2 socket")); + goto err; + } } if (rc != 0) { @@ -161,7 +166,11 @@ phypExec(LIBSSH2_SESSION *session, const char *cmd, int *exit_status, /* this is due to blocking that would occur otherwise so we loop on * this condition */ if (rc == LIBSSH2_ERROR_EAGAIN) { - waitsocket(sock, session); + if (waitsocket(sock, session) < 0 && errno != EINTR) { + virReportSystemError(errno, "%s", + _("unable to wait on libssh2 socket")); + goto err; + } } else { break; } @@ -170,7 +179,11 @@ phypExec(LIBSSH2_SESSION *session, const char *cmd, int *exit_status, exitcode = 127; while ((rc = libssh2_channel_close(channel)) == LIBSSH2_ERROR_EAGAIN) { - waitsocket(sock, session); + if (waitsocket(sock, session) < 0 && errno != EINTR) { + virReportSystemError(errno, "%s", + _("unable to wait on libssh2 socket")); + goto err; + } } if (rc == 0) { @@ -735,7 +748,11 @@ phypUUIDTable_Pull(virConnectPtr conn) LIBSSH2_ERROR_EAGAIN) { goto err; } else { - waitsocket(sock, session); + if (waitsocket(sock, session) < 0 && errno != EINTR) { + virReportSystemError(errno, "%s", + _("unable to wait on libssh2 socket")); + goto err; + } } } } while (!channel); @@ -769,7 +786,12 @@ phypUUIDTable_Pull(virConnectPtr conn) /* this is due to blocking that would occur otherwise * so we loop on this condition */ - waitsocket(sock, session); /* now we wait */ + /* now we wait */ + if (waitsocket(sock, session) < 0 && errno != EINTR) { + virReportSystemError(errno, "%s", + _("unable to wait on libssh2 socket")); + goto err; + } continue; } break; -- 1.7.11.7
On Thu, Jan 03, 2013 at 02:16:22PM -0500, John Ferlan wrote:
--- src/phyp/phyp_driver.c | 42 ++++++++++++++++++++++++++++++++---------- 1 file changed, 32 insertions(+), 10 deletions(-)
diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c index 8e26b0c..2dabd19 100644 --- a/src/phyp/phyp_driver.c +++ b/src/phyp/phyp_driver.c @@ -76,7 +76,6 @@ static int waitsocket(int socket_fd, LIBSSH2_SESSION * session) { struct timeval timeout; - int rc; fd_set fd; fd_set *writefd = NULL; fd_set *readfd = NULL; @@ -98,9 +97,7 @@ waitsocket(int socket_fd, LIBSSH2_SESSION * session) if (dir & LIBSSH2_SESSION_BLOCK_OUTBOUND) writefd = &fd;
- rc = select(socket_fd + 1, readfd, writefd, NULL, &timeout); - - return rc; + return select(socket_fd + 1, readfd, writefd, NULL, &timeout); }
/* this function is the layer that manipulates the ssh channel itself @@ -131,7 +128,11 @@ phypExec(LIBSSH2_SESSION *session, const char *cmd, int *exit_status, while ((channel = libssh2_channel_open_session(session)) == NULL && libssh2_session_last_error(session, NULL, NULL, 0) == LIBSSH2_ERROR_EAGAIN) { - waitsocket(sock, session); + if (waitsocket(sock, session) < 0 && errno != EINTR) { + virReportSystemError(errno, "%s", + _("unable to wait on libssh2 socket")); + goto err; + } }
if (channel == NULL) { @@ -140,7 +141,11 @@ phypExec(LIBSSH2_SESSION *session, const char *cmd, int *exit_status,
while ((rc = libssh2_channel_exec(channel, cmd)) == LIBSSH2_ERROR_EAGAIN) { - waitsocket(sock, session); + if (waitsocket(sock, session) < 0 && errno != EINTR) { + virReportSystemError(errno, "%s", + _("unable to wait on libssh2 socket")); + goto err; + } }
if (rc != 0) { @@ -161,7 +166,11 @@ phypExec(LIBSSH2_SESSION *session, const char *cmd, int *exit_status, /* this is due to blocking that would occur otherwise so we loop on * this condition */ if (rc == LIBSSH2_ERROR_EAGAIN) { - waitsocket(sock, session); + if (waitsocket(sock, session) < 0 && errno != EINTR) { + virReportSystemError(errno, "%s", + _("unable to wait on libssh2 socket")); + goto err; + } } else { break; } @@ -170,7 +179,11 @@ phypExec(LIBSSH2_SESSION *session, const char *cmd, int *exit_status, exitcode = 127;
while ((rc = libssh2_channel_close(channel)) == LIBSSH2_ERROR_EAGAIN) { - waitsocket(sock, session); + if (waitsocket(sock, session) < 0 && errno != EINTR) { + virReportSystemError(errno, "%s", + _("unable to wait on libssh2 socket")); + goto err; + } }
if (rc == 0) { @@ -735,7 +748,11 @@ phypUUIDTable_Pull(virConnectPtr conn) LIBSSH2_ERROR_EAGAIN) { goto err; } else { - waitsocket(sock, session); + if (waitsocket(sock, session) < 0 && errno != EINTR) { + virReportSystemError(errno, "%s", + _("unable to wait on libssh2 socket")); + goto err; + } } } } while (!channel); @@ -769,7 +786,12 @@ phypUUIDTable_Pull(virConnectPtr conn) /* this is due to blocking that would occur otherwise * so we loop on this condition */
- waitsocket(sock, session); /* now we wait */ + /* now we wait */ + if (waitsocket(sock, session) < 0 && errno != EINTR) { + virReportSystemError(errno, "%s", + _("unable to wait on libssh2 socket")); + goto err; + } continue; } break;
ACK Daniel -- |: 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 :|
On 01/03/2013 12:39 PM, Daniel P. Berrange wrote:
On Thu, Jan 03, 2013 at 02:16:22PM -0500, John Ferlan wrote:
--- src/phyp/phyp_driver.c | 42 ++++++++++++++++++++++++++++++++---------- 1 file changed, 32 insertions(+), 10 deletions(-)
ACK
I've pushed this one as well (but not 9). You'll want to run 'git pull --rebase' to see which patches still remain, and post a v2 on those. Feel free to ask for help (here or in IRC) in figuring out how to make git do what you want for rebasing patches. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Thu, Jan 03, 2013 at 02:16:12PM -0500, John Ferlan wrote:
Bug: https://bugzilla.redhat.com/show_bug.cgi?id=880388
This set of patches resolves the "CHECKED_RETURN (CWE-252)" errors found by Coverity.
John Ferlan (10): interface: Check and handle error for virAsprintf() calls. parallels: Check and handle error for virAsprintf() calls. Ignore the return inparallelsMakePoolName() since subsequent check validates name was allocated. rpc: Check status when attempting to set SO_REUSEADDR flag on outgoing connection. On failure, VIR_WARN(), but continue to connect. vmware: Ignore the return status check for vmwareUpdateVMStatus in convenience routine vmwareDomainObjListUpdateDomain xen: Check return status for setting TCP_NODELAY option and generate a VIR_DEBUG message on failure. Allow connection to continue. virlockspacetest: Check return on mkdir for LOCKSPACE_DIR vmx2xmltest: Check and handle error for virAsprintf() calls. xml2vmxtest: Check and handle error for virAsprintf() calls. virsh: Ignore error returns for virBufferTrim(). phyp: Check and handle select() errors from waitsocket().
There are a number of issues with vifAsprintf(). As a further patch I think you should add ATTRIBUTE_RETURN_CHECK to this function, so we see the problems immediately rather than relying on coverity. Daniel -- |: 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