[libvirt] [PATCH 0/6] Couple of small fixes
Some of these were found by coverity, some by me going through code. Michal Privoznik (6): virqemu: Reflect return type of virJSONValueArraySize() qemuMigrationCookieStatisticsXMLParse: Check for retvals of virXPath*() securityselinuxlabeltest: Prefer virGetLastErrorMessage() over virGetLastError virshConnect: Don't leak polkit agent libxlDoMigrateReceive: Drop useless check for !vm virObjectEventNew: Use virObjectUnref() to free virObjectEvent src/conf/object_event.c | 10 ++--- src/libxl/libxl_migration.c | 2 +- src/qemu/qemu_migration.c | 80 +++++++++++++++++++--------------------- src/util/virqemu.c | 2 +- tests/securityselinuxlabeltest.c | 3 +- tools/virsh.c | 2 +- 6 files changed, 45 insertions(+), 54 deletions(-) -- 2.8.4
The virJSONValueArraySize() function return ssize_t (with possibly returning -1 if the passed json is not an array). Storing the return value into size_t is possibly dangerous then. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virqemu.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/util/virqemu.c b/src/util/virqemu.c index a1ba562..0b516fc 100644 --- a/src/util/virqemu.c +++ b/src/util/virqemu.c @@ -85,7 +85,7 @@ virQEMUBuildCommandLineJSONArrayNumbered(const char *key, virBufferPtr buf) { const virJSONValue *member; - size_t nelems = virJSONValueArraySize(array); + ssize_t nelems = virJSONValueArraySize(array); char *prefix = NULL; size_t i; int ret = 0; -- 2.8.4
On Thu, Aug 04, 2016 at 09:47:45 +0200, Michal Privoznik wrote:
The virJSONValueArraySize() function return ssize_t (with possibly returning -1 if the passed json is not an array). Storing the return value into size_t is possibly dangerous then.
Not in this case. All code paths calling this function guarantee that the JSON object is an array, so it won't ever be negative. Don't trust anything that coverity tells you.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/util/virqemu.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/util/virqemu.c b/src/util/virqemu.c index a1ba562..0b516fc 100644 --- a/src/util/virqemu.c +++ b/src/util/virqemu.c @@ -85,7 +85,7 @@ virQEMUBuildCommandLineJSONArrayNumbered(const char *key, virBufferPtr buf) { const virJSONValue *member; - size_t nelems = virJSONValueArraySize(array); + ssize_t nelems = virJSONValueArraySize(array);
ACK if you remove the hint that it could cause a problem from the commit message. Peter
On 04.08.2016 09:54, Peter Krempa wrote:
On Thu, Aug 04, 2016 at 09:47:45 +0200, Michal Privoznik wrote:
The virJSONValueArraySize() function return ssize_t (with possibly returning -1 if the passed json is not an array). Storing the return value into size_t is possibly dangerous then.
Not in this case. All code paths calling this function guarantee that the JSON object is an array, so it won't ever be negative.
Not quite. We have the function exposed. So even though there currently isn't caller that passes non-array, it is still worth fixing IMO.
Don't trust anything that coverity tells you.
I'm not, that's why I've sent fixes just for true positives. Michal
There's no critical bug fix in here, but if there's ever a bug in our code and we send some gibberish in migration cookie, the other side doesn't check if conversion from string to integer was successful or not. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_migration.c | 80 ++++++++++++++++++++++------------------------- 1 file changed, 38 insertions(+), 42 deletions(-) diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c index d018add..39c5964 100644 --- a/src/qemu/qemu_migration.c +++ b/src/qemu/qemu_migration.c @@ -1095,16 +1095,30 @@ qemuMigrationCookieStatisticsXMLParse(xmlXPathContextPtr ctxt) stats = &jobInfo->stats; jobInfo->type = VIR_DOMAIN_JOB_COMPLETED; - virXPathULongLong("string(./started[1])", ctxt, &jobInfo->started); - virXPathULongLong("string(./stopped[1])", ctxt, &jobInfo->stopped); - virXPathULongLong("string(./sent[1])", ctxt, &jobInfo->sent); +#define STATS_PARSE(f, xpath, val) \ + do { \ + int rc = f("string(./" xpath "[1])", ctxt, val); \ + if (rc == -2) { \ + virReportError(VIR_ERR_INTERNAL_ERROR, \ + _("Malformed %s"), xpath); \ + goto cleanup; \ + } \ + } while (0) + +#define STATS_PARSEULL(xpath, val) \ + STATS_PARSE(virXPathULongLong, xpath, val) + +#define STATS_PARSEINT(xpath, val) \ + STATS_PARSE(virXPathInt, xpath, val) + + STATS_PARSEULL("started", &jobInfo->started); + STATS_PARSEULL("stopped", &jobInfo->stopped); + STATS_PARSEULL("sent", &jobInfo->sent); if (virXPathLongLong("string(./delta[1])", ctxt, &jobInfo->timeDelta) == 0) jobInfo->timeDeltaSet = true; - virXPathULongLong("string(./" VIR_DOMAIN_JOB_TIME_ELAPSED "[1])", - ctxt, &jobInfo->timeElapsed); - virXPathULongLong("string(./" VIR_DOMAIN_JOB_TIME_REMAINING "[1])", - ctxt, &jobInfo->timeRemaining); + STATS_PARSEULL(VIR_DOMAIN_JOB_TIME_ELAPSED, &jobInfo->timeElapsed); + STATS_PARSEULL(VIR_DOMAIN_JOB_TIME_REMAINING, &jobInfo->timeRemaining); if (virXPathULongLong("string(./" VIR_DOMAIN_JOB_DOWNTIME "[1])", ctxt, &stats->downtime) == 0) @@ -1113,51 +1127,33 @@ qemuMigrationCookieStatisticsXMLParse(xmlXPathContextPtr ctxt) ctxt, &stats->setup_time) == 0) stats->setup_time_set = true; - virXPathULongLong("string(./" VIR_DOMAIN_JOB_MEMORY_TOTAL "[1])", - ctxt, &stats->ram_total); - virXPathULongLong("string(./" VIR_DOMAIN_JOB_MEMORY_PROCESSED "[1])", - ctxt, &stats->ram_transferred); - virXPathULongLong("string(./" VIR_DOMAIN_JOB_MEMORY_REMAINING "[1])", - ctxt, &stats->ram_remaining); - virXPathULongLong("string(./" VIR_DOMAIN_JOB_MEMORY_BPS "[1])", - ctxt, &stats->ram_bps); + STATS_PARSEULL(VIR_DOMAIN_JOB_MEMORY_TOTAL, &stats->ram_total); + STATS_PARSEULL(VIR_DOMAIN_JOB_MEMORY_PROCESSED, &stats->ram_transferred); + STATS_PARSEULL(VIR_DOMAIN_JOB_MEMORY_REMAINING, &stats->ram_remaining); + STATS_PARSEULL(VIR_DOMAIN_JOB_MEMORY_BPS, &stats->ram_bps); if (virXPathULongLong("string(./" VIR_DOMAIN_JOB_MEMORY_CONSTANT "[1])", ctxt, &stats->ram_duplicate) == 0) stats->ram_duplicate_set = true; - virXPathULongLong("string(./" VIR_DOMAIN_JOB_MEMORY_NORMAL "[1])", - ctxt, &stats->ram_normal); - virXPathULongLong("string(./" VIR_DOMAIN_JOB_MEMORY_NORMAL_BYTES "[1])", - ctxt, &stats->ram_normal_bytes); + STATS_PARSEULL(VIR_DOMAIN_JOB_MEMORY_NORMAL, &stats->ram_normal); + STATS_PARSEULL(VIR_DOMAIN_JOB_MEMORY_NORMAL_BYTES, &stats->ram_normal_bytes); + STATS_PARSEULL(VIR_DOMAIN_JOB_MEMORY_DIRTY_RATE, &stats->ram_dirty_rate); + STATS_PARSEULL(VIR_DOMAIN_JOB_MEMORY_ITERATION, &stats->ram_iteration); - virXPathULongLong("string(./" VIR_DOMAIN_JOB_MEMORY_DIRTY_RATE "[1])", - ctxt, &stats->ram_dirty_rate); - virXPathULongLong("string(./" VIR_DOMAIN_JOB_MEMORY_ITERATION "[1])", - ctxt, &stats->ram_iteration); - - virXPathULongLong("string(./" VIR_DOMAIN_JOB_DISK_TOTAL "[1])", - ctxt, &stats->disk_total); - virXPathULongLong("string(./" VIR_DOMAIN_JOB_DISK_PROCESSED "[1])", - ctxt, &stats->disk_transferred); - virXPathULongLong("string(./" VIR_DOMAIN_JOB_DISK_REMAINING "[1])", - ctxt, &stats->disk_remaining); - virXPathULongLong("string(./" VIR_DOMAIN_JOB_DISK_BPS "[1])", - ctxt, &stats->disk_bps); + STATS_PARSEULL(VIR_DOMAIN_JOB_DISK_TOTAL, &stats->disk_total); + STATS_PARSEULL(VIR_DOMAIN_JOB_DISK_PROCESSED, &stats->disk_transferred); + STATS_PARSEULL(VIR_DOMAIN_JOB_DISK_REMAINING, &stats->disk_remaining); + STATS_PARSEULL(VIR_DOMAIN_JOB_DISK_BPS, &stats->disk_bps); if (virXPathULongLong("string(./" VIR_DOMAIN_JOB_COMPRESSION_CACHE "[1])", ctxt, &stats->xbzrle_cache_size) == 0) stats->xbzrle_set = true; - virXPathULongLong("string(./" VIR_DOMAIN_JOB_COMPRESSION_BYTES "[1])", - ctxt, &stats->xbzrle_bytes); - virXPathULongLong("string(./" VIR_DOMAIN_JOB_COMPRESSION_PAGES "[1])", - ctxt, &stats->xbzrle_pages); - virXPathULongLong("string(./" VIR_DOMAIN_JOB_COMPRESSION_CACHE_MISSES "[1])", - ctxt, &stats->xbzrle_cache_miss); - virXPathULongLong("string(./" VIR_DOMAIN_JOB_COMPRESSION_OVERFLOW "[1])", - ctxt, &stats->xbzrle_overflow); + STATS_PARSEULL(VIR_DOMAIN_JOB_COMPRESSION_BYTES, &stats->xbzrle_bytes); + STATS_PARSEULL(VIR_DOMAIN_JOB_COMPRESSION_PAGES, &stats->xbzrle_pages); + STATS_PARSEULL(VIR_DOMAIN_JOB_COMPRESSION_CACHE_MISSES, &stats->xbzrle_cache_miss); + STATS_PARSEULL(VIR_DOMAIN_JOB_COMPRESSION_OVERFLOW, &stats->xbzrle_overflow); - virXPathInt("string(./" VIR_DOMAIN_JOB_AUTO_CONVERGE_THROTTLE "[1])", - ctxt, &stats->cpu_throttle_percentage); + STATS_PARSEINT(VIR_DOMAIN_JOB_AUTO_CONVERGE_THROTTLE, &stats->cpu_throttle_percentage); cleanup: ctxt->node = save_ctxt; return jobInfo; -- 2.8.4
On Thu, Aug 04, 2016 at 09:47:46 +0200, Michal Privoznik wrote:
There's no critical bug fix in here, but if there's ever a bug in our code and we send some gibberish in migration cookie, the other side doesn't check if conversion from string to integer was successful or not.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_migration.c | 80 ++++++++++++++++++++++------------------------- 1 file changed, 38 insertions(+), 42 deletions(-)
I don't think this makes sense. If the stats can't be parsed it won't break anything. Also it's internaly generated and parsed so the possibility it will break is very low. Also you don't want to fail migration due to a mishap like this. I presume this is due to coverity. If it bothers you enough add a ignore_value(). NACK Peter
At the beginning of the test, some preparation work is done. For instance new virSecurityManager is created. If this fails for whatever reason, we try to fetch the latest error and print the error message contained in it. However, if there's a bug in our code and no error is reported, this approach will lead to crash, while with virGetLastErrorMessage() it won't. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/securityselinuxlabeltest.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/securityselinuxlabeltest.c b/tests/securityselinuxlabeltest.c index 7d5544b..4c0f427 100644 --- a/tests/securityselinuxlabeltest.c +++ b/tests/securityselinuxlabeltest.c @@ -354,9 +354,8 @@ mymain(void) if (!(mgr = virSecurityManagerNew("selinux", "QEMU", VIR_SECURITY_MANAGER_DEFAULT_CONFINED | VIR_SECURITY_MANAGER_PRIVILEGED))) { - virErrorPtr err = virGetLastError(); VIR_TEST_VERBOSE("Unable to initialize security driver: %s\n", - err->message); + virGetLastErrorMessage()); return EXIT_FAILURE; } -- 2.8.4
On Thu, Aug 04, 2016 at 09:47:47 +0200, Michal Privoznik wrote:
At the beginning of the test, some preparation work is done. For instance new virSecurityManager is created. If this fails for whatever reason, we try to fetch the latest error and print the error message contained in it. However, if there's a bug in our code and no error is reported, this approach will lead to crash, while with virGetLastErrorMessage() it won't.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tests/securityselinuxlabeltest.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
ACK
In our attempts to reconnect, we may create a polkit daemon. However, it may happen that we would rewrite the variable that already holds pointer to the agent. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tools/virsh.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/virsh.c b/tools/virsh.c index d3fe06f..cb60edc 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -168,7 +168,7 @@ virshConnect(vshControl *ctl, const char *uri, bool readonly) err = virGetLastError(); if (err && err->domain == VIR_FROM_POLKIT && err->code == VIR_ERR_AUTH_UNAVAILABLE) { - if (!(pkagent = virPolkitAgentCreate())) + if (!pkagent && !(pkagent = virPolkitAgentCreate())) goto cleanup; } else if (err && err->domain == VIR_FROM_POLKIT && err->code == VIR_ERR_AUTH_FAILED) { -- 2.8.4
On Thu, Aug 04, 2016 at 09:47:48 +0200, Michal Privoznik wrote:
In our attempts to reconnect, we may create a polkit daemon. However, it may happen that we would rewrite the variable that already holds pointer to the agent.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- tools/virsh.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/virsh.c b/tools/virsh.c index d3fe06f..cb60edc 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -168,7 +168,7 @@ virshConnect(vshControl *ctl, const char *uri, bool readonly) err = virGetLastError(); if (err && err->domain == VIR_FROM_POLKIT && err->code == VIR_ERR_AUTH_UNAVAILABLE) { - if (!(pkagent = virPolkitAgentCreate())) + if (!pkagent && !(pkagent = virPolkitAgentCreate()))
This almost looks like a false positive, but I think that the loop of conditions that would actually make us overwrite the agent is very unintrospectable for humans too. ACK
In the cleanup path, @vm cannot be possibly NULL. If it were so, we would receive SIGSEGV much earlier. At the beginning of the function we do libxlDomainObjBeginJob(.., vm, ..); and so on. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libxl/libxl_migration.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libxl/libxl_migration.c b/src/libxl/libxl_migration.c index 8be7877..f1da251 100644 --- a/src/libxl/libxl_migration.c +++ b/src/libxl/libxl_migration.c @@ -296,7 +296,7 @@ libxlDoMigrateReceive(void *opaque) libxlDomainObjEndJob(driver, vm); cleanup: - if (remove_dom && vm) { + if (remove_dom) { virDomainObjListRemove(driver->domains, vm); vm = NULL; } -- 2.8.4
On Thu, Aug 04, 2016 at 09:47:49 +0200, Michal Privoznik wrote:
In the cleanup path, @vm cannot be possibly NULL. If it were so, we would receive SIGSEGV much earlier. At the beginning of the function we do libxlDomainObjBeginJob(.., vm, ..); and so on.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libxl/libxl_migration.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
ACK
While no leak was observed yet, there might be one if virObjectEventClass is ever derived from another class. Because in that case plain VIR_FREE() will not call dispose() from parent classes possibly leaking some memory. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/object_event.c | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/src/conf/object_event.c b/src/conf/object_event.c index bbef7f6..b5a6a81 100644 --- a/src/conf/object_event.c +++ b/src/conf/object_event.c @@ -649,13 +649,9 @@ virObjectEventNew(virClassPtr klass, event->eventID = eventID; event->remoteID = -1; - if (VIR_STRDUP(event->meta.name, name) < 0) { - VIR_FREE(event); - return NULL; - } - if (VIR_STRDUP(event->meta.key, key) < 0) { - VIR_FREE(event->meta.name); - VIR_FREE(event); + if (VIR_STRDUP(event->meta.name, name) < 0 || + VIR_STRDUP(event->meta.key, key) < 0) { + virObjectUnref(event); return NULL; } event->meta.id = id; -- 2.8.4
On Thu, Aug 04, 2016 at 09:47:50 +0200, Michal Privoznik wrote:
While no leak was observed yet, there might be one if virObjectEventClass is ever derived from another class. Because in that case plain VIR_FREE() will not call dispose() from parent classes possibly leaking some memory.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/object_event.c | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-)
ACK
participants (2)
-
Michal Privoznik -
Peter Krempa