2:47 a.m.
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
2:47 a.m.
New subject: [libvirt] [PATCH 1/6] virqemu: Reflect return type of virJSONValueArraySize()
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(a)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
2:54 a.m.
New subject: [libvirt] [PATCH 1/6] virqemu: Reflect return type of virJSONValueArraySize()
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(a)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
8:35 a.m.
New subject: [libvirt] [PATCH 1/6] virqemu: Reflect return type of virJSONValueArraySize()
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
2:47 a.m.
New subject: [libvirt] [PATCH 2/6] qemuMigrationCookieStatisticsXMLParse: Check for retvals of virXPath*()
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(a)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
5:09 a.m.
New subject: [libvirt] [PATCH 2/6] qemuMigrationCookieStatisticsXMLParse: Check for retvals of virXPath*()
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(a)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
2:47 a.m.
New subject: [libvirt] [PATCH 3/6] securityselinuxlabeltest: Prefer virGetLastErrorMessage() over virGetLastError
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(a)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
2:55 a.m.
New subject: [libvirt] [PATCH 3/6] securityselinuxlabeltest: Prefer virGetLastErrorMessage() over virGetLastError
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(a)redhat.com>
---
tests/securityselinuxlabeltest.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
ACK
2:47 a.m.
New subject: [libvirt] [PATCH 4/6] virshConnect: Don't leak polkit agent
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(a)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
5:14 a.m.
New subject: [libvirt] [PATCH 4/6] virshConnect: Don't leak polkit agent
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(a)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
2:47 a.m.
New subject: [libvirt] [PATCH 5/6] libxlDoMigrateReceive: Drop useless check for !vm
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(a)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
2:58 a.m.
New subject: [libvirt] [PATCH 5/6] libxlDoMigrateReceive: Drop useless check for !vm
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(a)redhat.com>
---
src/libxl/libxl_migration.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
ACK
2:47 a.m.
New subject: [libvirt] [PATCH 6/6] virObjectEventNew: Use virObjectUnref() to free virObjectEvent
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(a)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
5:18 a.m.
New subject: [libvirt] [PATCH 6/6] virObjectEventNew: Use virObjectUnref() to free virObjectEvent
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(a)redhat.com>
---
src/conf/object_event.c | 10 +++-------
1 file changed, 3 insertions(+), 7 deletions(-)
ACK
3032
days inactive
3032
days old
13 comments
2 participants
participants (2)
-
Michal Privoznik -
Peter Krempa