[libvirt] [PATCH 0/3] Resolve Coverity found issues
Upgraded to Coverity 6.5.3 which uncovered a few code paths with issues. John Ferlan (3): lxc_process: Resolve Coverity NULL_RETURNS error xencapstest: Resolve Coverity CHECKED_RETURN error shunloadtest: Resolve Coverity CHECKED_RETURN error src/lxc/lxc_process.c | 4 ++-- tests/shunloadhelper.c | 12 ++++++++---- tests/shunloadtest.c | 26 +++++++++++++++++--------- tests/xencapstest.c | 3 ++- 4 files changed, 29 insertions(+), 16 deletions(-) -- 1.8.1.4
In virLXCProcessReboot, if the 'conn' value is NULL, the eventual call to virLXCNetworkLookupByName() via virLXCProcessStart() and virLXCProcessSetupInterfaces() will cause a core since it references conn->networkDriver directly. --- src/lxc/lxc_process.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c index b06d748..4410db4 100644 --- a/src/lxc/lxc_process.c +++ b/src/lxc/lxc_process.c @@ -181,7 +181,6 @@ virLXCProcessReboot(virLXCDriverPtr driver, autodestroy = true; } else { conn = virConnectOpen("lxc:///"); - /* Ignoring NULL conn which is mostly harmless here */ } /* In a reboot scenario, we need to make sure we continue @@ -192,7 +191,8 @@ virLXCProcessReboot(virLXCDriverPtr driver, vm->newDef = NULL; virLXCProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_SHUTDOWN); vm->newDef = savedDef; - if (virLXCProcessStart(conn, driver, vm, autodestroy, reason) < 0) { + if (!conn || + virLXCProcessStart(conn, driver, vm, autodestroy, reason) < 0) { VIR_WARN("Unable to handle reboot of vm %s", vm->def->name); goto cleanup; -- 1.8.1.4
On Thu, May 16, 2013 at 10:01:51AM -0400, John Ferlan wrote:
In virLXCProcessReboot, if the 'conn' value is NULL, the eventual call to virLXCNetworkLookupByName() via virLXCProcessStart() and virLXCProcessSetupInterfaces() will cause a core since it references conn->networkDriver directly. --- src/lxc/lxc_process.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c index b06d748..4410db4 100644 --- a/src/lxc/lxc_process.c +++ b/src/lxc/lxc_process.c @@ -181,7 +181,6 @@ virLXCProcessReboot(virLXCDriverPtr driver, autodestroy = true; } else { conn = virConnectOpen("lxc:///"); - /* Ignoring NULL conn which is mostly harmless here */
I'm actually inclined to say that we should stop ignoring failures of virConnectOpen(). I don't think there's any valid reason for us to be doing that. Then we wouldnt have to worry about conn being NULL later This ignoring of errors is historical legacy of my own creation when first writing the QEMU driver. I can't think what possessed me to ignore the errors at the time. 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 05/16/2013 08:01 AM, John Ferlan wrote:
In virLXCProcessReboot, if the 'conn' value is NULL, the eventual call to virLXCNetworkLookupByName() via virLXCProcessStart() and virLXCProcessSetupInterfaces() will cause a core since it references conn->networkDriver directly.
Did you investigate which commit introduced this problem?
--- src/lxc/lxc_process.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 05/16/2013 10:19 AM, Eric Blake wrote:
On 05/16/2013 08:01 AM, John Ferlan wrote:
In virLXCProcessReboot, if the 'conn' value is NULL, the eventual call to virLXCNetworkLookupByName() via virLXCProcessStart() and virLXCProcessSetupInterfaces() will cause a core since it references conn->networkDriver directly.
Did you investigate which commit introduced this problem?
yes, cb612ee4, when the function was created. I will add it to the commit message Given Daniel's response - is there a preference to perhaps add some sort of message that conn was NULL from the virConnectOpen(). I would think that the virLXCProcessStop() would still need to occur though, but also wouldn't want to assume anything :-) John
--- src/lxc/lxc_process.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
ACK.
The return from virInitialize() needs to be checked. --- tests/xencapstest.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/xencapstest.c b/tests/xencapstest.c index e220234..8bdd6cd 100644 --- a/tests/xencapstest.c +++ b/tests/xencapstest.c @@ -161,7 +161,8 @@ mymain(void) int ret = 0; xenHypervisorInit(&hv_versions); - virInitialize(); + if (virInitialize() < 0) + return EXIT_FAILURE; if (virtTestRun("Capabilities for i686, no PAE, no HVM", 1, testXeni686, NULL) != 0) -- 1.8.1.4
On 05/16/2013 08:01 AM, John Ferlan wrote:
The return from virInitialize() needs to be checked. --- tests/xencapstest.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
ACK.
diff --git a/tests/xencapstest.c b/tests/xencapstest.c index e220234..8bdd6cd 100644 --- a/tests/xencapstest.c +++ b/tests/xencapstest.c @@ -161,7 +161,8 @@ mymain(void) int ret = 0;
xenHypervisorInit(&hv_versions); - virInitialize(); + if (virInitialize() < 0) + return EXIT_FAILURE;
if (virtTestRun("Capabilities for i686, no PAE, no HVM", 1, testXeni686, NULL) != 0)
-- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
The shunloadStart function didn't check the status of virInitialize which was flagged by Coverity. Adjust the function and shunloadtest in order to handle the situation. --- tests/shunloadhelper.c | 12 ++++++++---- tests/shunloadtest.c | 26 +++++++++++++++++--------- 2 files changed, 25 insertions(+), 13 deletions(-) diff --git a/tests/shunloadhelper.c b/tests/shunloadhelper.c index a8f5aef..f2afbe8 100644 --- a/tests/shunloadhelper.c +++ b/tests/shunloadhelper.c @@ -36,16 +36,20 @@ static void shunloadError(void *userData ATTRIBUTE_UNUSED, { } -void shunloadStart(void); +int shunloadStart(void); -void shunloadStart(void) { +int shunloadStart(void) { virConnectPtr conn; virSetErrorFunc(NULL, shunloadError); - virInitialize(); + if (virInitialize() < 0) + return -1; conn = virConnectOpen("test:///default"); virDomainDestroy(NULL); - if (conn) + if (conn) { virConnectClose(conn); + return 0; + } + return -1; } diff --git a/tests/shunloadtest.c b/tests/shunloadtest.c index 8271b93..8190e97 100644 --- a/tests/shunloadtest.c +++ b/tests/shunloadtest.c @@ -56,17 +56,22 @@ pthread_cond_t cond = PTHREAD_COND_INITIALIZER; pthread_mutex_t lock = PTHREAD_MUTEX_INITIALIZER; bool running = false; +bool failstart = false; bool quit = false; static void *threadMain(void *arg) { - void (*startup)(void) = arg; - - startup(); - - pthread_mutex_lock(&lock); - running = true; - pthread_cond_signal(&cond); + int (*startup)(void) = arg; + + if (startup() < 0) { + pthread_mutex_lock(&lock); + failstart = true; + pthread_cond_signal(&cond); + } else { + pthread_mutex_lock(&lock); + running = true; + pthread_cond_signal(&cond); + } while (!quit) { pthread_cond_wait(&cond, &lock); @@ -119,7 +124,7 @@ int main(int argc ATTRIBUTE_UNUSED, char **argv) /* Wait for the thread to start and call libvirt */ pthread_mutex_lock(&lock); - while (!running) { + while (!running && !failstart) { pthread_cond_wait(&cond, &lock); } @@ -138,7 +143,10 @@ int main(int argc ATTRIBUTE_UNUSED, char **argv) * causing a SEGV ! */ - fprintf(stderr, "OK\n"); + if (failstart) + fprintf(stderr, "FAIL to initialize libvirt\n"); + else + fprintf(stderr, "OK\n"); return 0; } -- 1.8.1.4
On 05/16/2013 08:01 AM, John Ferlan wrote:
The shunloadStart function didn't check the status of virInitialize which was flagged by Coverity. Adjust the function and shunloadtest in order to handle the situation. --- tests/shunloadhelper.c | 12 ++++++++---- tests/shunloadtest.c | 26 +++++++++++++++++--------- 2 files changed, 25 insertions(+), 13 deletions(-)
ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 05/16/2013 10:01 AM, John Ferlan wrote:
Upgraded to Coverity 6.5.3 which uncovered a few code paths with issues.
John Ferlan (3): lxc_process: Resolve Coverity NULL_RETURNS error xencapstest: Resolve Coverity CHECKED_RETURN error shunloadtest: Resolve Coverity CHECKED_RETURN error
src/lxc/lxc_process.c | 4 ++-- tests/shunloadhelper.c | 12 ++++++++---- tests/shunloadtest.c | 26 +++++++++++++++++--------- tests/xencapstest.c | 3 ++- 4 files changed, 29 insertions(+), 16 deletions(-)
patches 2/3 and 3/3 were pushed, I'm reworking 1/3 to store the virConnectPtr John
participants (3)
-
Daniel P. Berrange -
Eric Blake -
John Ferlan