[libvirt] [PATCH 0/4] several Coverity fixes
I ran Coverity on the state of yesterday's git; while I have not finished reading through all the findings, I figured I'd get some review started on these. Eric Blake (4): virsh: avoid integer overflow virsh: avoid uninitialized variable rpc: avoid freeing uninitialized variable build: avoid double-close bug with pipe2 .gnulib | 2 +- src/rpc/virnetclient.c | 2 +- src/rpc/virnetsocket.c | 4 ++-- tools/virsh.c | 7 +++++-- 4 files changed, 9 insertions(+), 6 deletions(-) -- 1.7.4.4
Detected by Coverity. info.nrVirtCpu is unsigned short, but if cpumaplen is int, then the product of the two in vshMalloc risks unintended sign extension. cmdVcpuinfo had already solved this by using size_t cpumaplen. * tools/virsh.c (cmdVcpuPin): Use correct type. --- tools/virsh.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index d15d206..643f05c 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -3025,7 +3025,7 @@ cmdVcpuPin(vshControl *ctl, const vshCmd *cmd) bool ret = true; unsigned char *cpumap = NULL; unsigned char *cpumaps = NULL; - int cpumaplen; + size_t cpumaplen; bool bit, lastbit, isInvert; int i, cpu, lastcpu, maxcpu, ncpus; bool unuse = false; -- 1.7.4.4
On Thu, Jun 30, 2011 at 08:14:53AM -0600, Eric Blake wrote:
Detected by Coverity. info.nrVirtCpu is unsigned short, but if cpumaplen is int, then the product of the two in vshMalloc risks unintended sign extension. cmdVcpuinfo had already solved this by using size_t cpumaplen.
* tools/virsh.c (cmdVcpuPin): Use correct type. --- tools/virsh.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/tools/virsh.c b/tools/virsh.c index d15d206..643f05c 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -3025,7 +3025,7 @@ cmdVcpuPin(vshControl *ctl, const vshCmd *cmd) bool ret = true; unsigned char *cpumap = NULL; unsigned char *cpumaps = NULL; - int cpumaplen; + size_t cpumaplen; bool bit, lastbit, isInvert; int i, cpu, lastcpu, maxcpu, ncpus; bool unuse = false;
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 :|
Detected by Coverity; neither vshCmddefHelp nor vshCmdOptParse was initializing opts_required. * tools/virsh.c (vshCmddefOptParse): Always initialize bitmaps. --- tools/virsh.c | 5 ++++- 1 files changed, 4 insertions(+), 1 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index 643f05c..fdb4283 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -11915,12 +11915,15 @@ vshCmddefGetInfo(const vshCmdDef * cmd, const char *name) } static int -vshCmddefOptParse(const vshCmdDef *cmd, uint32_t* opts_need_arg, +vshCmddefOptParse(const vshCmdDef *cmd, uint32_t *opts_need_arg, uint32_t *opts_required) { int i; bool optional = false; + *opts_need_arg = 0; + *opts_required = 0; + if (!cmd->opts) return 0; -- 1.7.4.4
On Thu, Jun 30, 2011 at 08:14:54AM -0600, Eric Blake wrote:
Detected by Coverity; neither vshCmddefHelp nor vshCmdOptParse was initializing opts_required.
* tools/virsh.c (vshCmddefOptParse): Always initialize bitmaps. --- tools/virsh.c | 5 ++++- 1 files changed, 4 insertions(+), 1 deletions(-)
diff --git a/tools/virsh.c b/tools/virsh.c index 643f05c..fdb4283 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -11915,12 +11915,15 @@ vshCmddefGetInfo(const vshCmdDef * cmd, const char *name) }
static int -vshCmddefOptParse(const vshCmdDef *cmd, uint32_t* opts_need_arg, +vshCmddefOptParse(const vshCmdDef *cmd, uint32_t *opts_need_arg, uint32_t *opts_required) { int i; bool optional = false;
+ *opts_need_arg = 0; + *opts_required = 0; + if (!cmd->opts) return 0;
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 :|
Detected by Coverity. Both are instances of bad things happening if pipe2 fails; the virNetClientNew failure could free garbage, and virNetSocketNewConnectCommand could close random fds. Note: POSIX doesn't guarantee the contents of fd[0] and fd[1] after pipe failure: http://austingroupbugs.net/view.php?id=467 We may need to introduce a virPipe2 wrapper that guarantees that on pipe failure, the fds are explicitly set to -1, rather than our current state of assuming the fds are unchanged from their value prior to the failed pipe call. * src/rpc/virnetclient.c (virNetClientNew): Initialize variable. * src/rpc/virnetsocket.c (virNetSocketNewConnectCommand): Likewise. --- src/rpc/virnetclient.c | 2 +- src/rpc/virnetsocket.c | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/rpc/virnetclient.c b/src/rpc/virnetclient.c index 39bdf14..b551b99 100644 --- a/src/rpc/virnetclient.c +++ b/src/rpc/virnetclient.c @@ -113,7 +113,7 @@ static void virNetClientIncomingEvent(virNetSocketPtr sock, static virNetClientPtr virNetClientNew(virNetSocketPtr sock, const char *hostname) { - virNetClientPtr client; + virNetClientPtr client = NULL; int wakeupFD[2] = { -1, -1 }; if (pipe2(wakeupFD, O_CLOEXEC) < 0) { diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c index 96d2dfd..d16f8e5 100644 --- a/src/rpc/virnetsocket.c +++ b/src/rpc/virnetsocket.c @@ -510,8 +510,8 @@ int virNetSocketNewConnectCommand(virCommandPtr cmd, virNetSocketPtr *retsock) { pid_t pid = 0; - int sv[2]; - int errfd[2]; + int sv[2] = { -1, -1 }; + int errfd[2] = { -1, -1 }; *retsock = NULL; -- 1.7.4.4
On Thu, Jun 30, 2011 at 08:14:55AM -0600, Eric Blake wrote:
Detected by Coverity. Both are instances of bad things happening if pipe2 fails; the virNetClientNew failure could free garbage, and virNetSocketNewConnectCommand could close random fds.
Note: POSIX doesn't guarantee the contents of fd[0] and fd[1] after pipe failure: http://austingroupbugs.net/view.php?id=467 We may need to introduce a virPipe2 wrapper that guarantees that on pipe failure, the fds are explicitly set to -1, rather than our current state of assuming the fds are unchanged from their value prior to the failed pipe call.
* src/rpc/virnetclient.c (virNetClientNew): Initialize variable. * src/rpc/virnetsocket.c (virNetSocketNewConnectCommand): Likewise. --- src/rpc/virnetclient.c | 2 +- src/rpc/virnetsocket.c | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/rpc/virnetclient.c b/src/rpc/virnetclient.c index 39bdf14..b551b99 100644 --- a/src/rpc/virnetclient.c +++ b/src/rpc/virnetclient.c @@ -113,7 +113,7 @@ static void virNetClientIncomingEvent(virNetSocketPtr sock, static virNetClientPtr virNetClientNew(virNetSocketPtr sock, const char *hostname) { - virNetClientPtr client; + virNetClientPtr client = NULL; int wakeupFD[2] = { -1, -1 };
if (pipe2(wakeupFD, O_CLOEXEC) < 0) { diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c index 96d2dfd..d16f8e5 100644 --- a/src/rpc/virnetsocket.c +++ b/src/rpc/virnetsocket.c @@ -510,8 +510,8 @@ int virNetSocketNewConnectCommand(virCommandPtr cmd, virNetSocketPtr *retsock) { pid_t pid = 0; - int sv[2]; - int errfd[2]; + int sv[2] = { -1, -1 }; + int errfd[2] = { -1, -1 };
*retsock = NULL;
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 :|
Based on Coverity's finding on the previous patch, I audited gnulib's pipe2 code and found that we had the potential for a subtle double-close bug, unless gnulib guarantees that the contents of the fd array are unchanged on pipe2() failure. * .gnulib: Update to latest, for pipe2 fix. --- .gnulib | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/.gnulib b/.gnulib index cbfd25f..7269b35 160000 --- a/.gnulib +++ b/.gnulib @@ -1 +1 @@ -Subproject commit cbfd25f0edf5f4b853e8ed2e01a4782ec80faba2 +Subproject commit 7269b35c8d9be1a6f97906b9e29b8c422b92fc31 -- 1.7.4.4
On Thu, Jun 30, 2011 at 08:14:56AM -0600, Eric Blake wrote:
Based on Coverity's finding on the previous patch, I audited gnulib's pipe2 code and found that we had the potential for a subtle double-close bug, unless gnulib guarantees that the contents of the fd array are unchanged on pipe2() failure.
* .gnulib: Update to latest, for pipe2 fix. --- .gnulib | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/.gnulib b/.gnulib index cbfd25f..7269b35 160000 --- a/.gnulib +++ b/.gnulib @@ -1 +1 @@ -Subproject commit cbfd25f0edf5f4b853e8ed2e01a4782ec80faba2 +Subproject commit 7269b35c8d9be1a6f97906b9e29b8c422b92fc31
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 06/30/2011 08:24 AM, Daniel P. Berrange wrote:
On Thu, Jun 30, 2011 at 08:14:56AM -0600, Eric Blake wrote:
Based on Coverity's finding on the previous patch, I audited gnulib's pipe2 code and found that we had the potential for a subtle double-close bug, unless gnulib guarantees that the contents of the fd array are unchanged on pipe2() failure.
* .gnulib: Update to latest, for pipe2 fix. --- .gnulib | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/.gnulib b/.gnulib index cbfd25f..7269b35 160000 --- a/.gnulib +++ b/.gnulib @@ -1 +1 @@ -Subproject commit cbfd25f0edf5f4b853e8ed2e01a4782ec80faba2 +Subproject commit 7269b35c8d9be1a6f97906b9e29b8c422b92fc31
ACK
Series pushed. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
participants (2)
-
Daniel P. Berrange -
Eric Blake