[libvirt] [PATCH 0/6] Fix some Coverity issues
Newer Coverity (7.7.0) found a couple real issues and a few more false positives. There's still a few more to be resolved, but still trying to figure them out... libxlDomainMigrationPrepare - claim is args is leaked. Although it seems to be handled in libxlMigrateReceive or libxlDoMigrateReceive. Don't know the code well enough to do proper triage. qemuProcessStop - claim is usage of net->ifname in the condition (vport->virtPortType == VIR_NETDEV_VPORT_PROFILE_OPENVSWITCH) is using a NULL net->ifname that would have been VIR_FREE()'d when case VIR_DOMAIN_NET_TYPE_DIRECT a few lines above. qemuDomainBlockRebase - claim is overwriting 'dest' after call to qemuDomainBlockCopyCommon causes resource leak. Although reading code says otherwise, except of course if CopyCommon goes to cleanup rather than endjob. Even changing the location doesn't resolve the issue. Fix the Coverity tag for the DEADCODE... It's always a coin-flip either dead_error_begin or dead_error_condition - I gave Jiri bad advice as to which one to use it seems. John Ferlan (6): Avoid Coverity FORWARD_NULL prior to strtok_r calls tests: Resolve Coverity RESOURCE_LEAK tests: Resolve Coverity RESOURCE_LEAK virsh: Resolve Coverity DEADCODE qemu: Resolve Coverity CHECKED_RETURN qemu: Resolve Coverity RESOURCE_LEAK src/esx/esx_vi.c | 1 + src/libxl/libxl_conf.c | 1 + src/openvz/openvz_conf.c | 2 ++ src/qemu/qemu_driver.c | 24 +++++++++++------------- src/qemu/qemu_process.c | 6 +++--- src/xenapi/xenapi_utils.c | 1 + tests/qemucaps2xmltest.c | 1 + tests/virnetdaemontest.c | 2 ++ tools/virsh.c | 2 +- 9 files changed, 23 insertions(+), 17 deletions(-) -- 2.1.0
The 'strtok_r' function requires passing a NULL as the first parameter on subsequent calls in order to ensure the code picks up where it left off on a previous call. However, Coverity doesn't quite realize this and points out that if a NULL was passed in as the third argument it would result in a possible NULL deref because the strtok_r function will assign the third argument to the first in the call is NULL. Adding an sa_assert() prior to each initial call quiets Coverity Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/esx/esx_vi.c | 1 + src/libxl/libxl_conf.c | 1 + src/openvz/openvz_conf.c | 2 ++ src/xenapi/xenapi_utils.c | 1 + 4 files changed, 5 insertions(+) diff --git a/src/esx/esx_vi.c b/src/esx/esx_vi.c index af822b1..a57d753 100644 --- a/src/esx/esx_vi.c +++ b/src/esx/esx_vi.c @@ -1173,6 +1173,7 @@ esxVI_Context_LookupManagedObjectsByPath(esxVI_Context *ctx, const char *path) goto cleanup; /* Lookup Datacenter */ + sa_assert(tmp); item = strtok_r(tmp, "/", &saveptr); if (!item) { diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index 35fd495..ab588e8 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -353,6 +353,7 @@ libxlCapsInitGuests(libxl_ctx *ctx, virCapsPtr caps) /* Split capabilities string into tokens. strtok_r is OK here because * we "own" the buffer. Parse out the features from each token. */ + sa_assert(ver_info->capabilities); for (str = ver_info->capabilities, nr_guest_archs = 0; nr_guest_archs < sizeof(guest_archs) / sizeof(guest_archs[0]) && (token = strtok_r(str, " ", &saveptr)) != NULL; diff --git a/src/openvz/openvz_conf.c b/src/openvz/openvz_conf.c index db0a9a7..003272e 100644 --- a/src/openvz/openvz_conf.c +++ b/src/openvz/openvz_conf.c @@ -138,6 +138,7 @@ openvzParseBarrierLimit(const char* value, char *str; int ret = -1; + sa_assert(value); if (VIR_STRDUP(str, value) < 0) goto error; @@ -965,6 +966,7 @@ openvzGetVPSUUID(int vpsid, char *uuidstr, size_t len) } } + sa_assert(line); iden = strtok_r(line, " ", &saveptr); uuidbuf = strtok_r(NULL, "\n", &saveptr); diff --git a/src/xenapi/xenapi_utils.c b/src/xenapi/xenapi_utils.c index a80e084..6b710cd 100644 --- a/src/xenapi/xenapi_utils.c +++ b/src/xenapi/xenapi_utils.c @@ -301,6 +301,7 @@ getCpuBitMapfromString(char *mask, unsigned char *cpumap, int maplen) int max_bits = maplen * 8; char *num = NULL, *bp = NULL; bzero(cpumap, maplen); + sa_assert(mask); num = strtok_r(mask, ",", &bp); while (num != NULL) { if (virStrToLong_i(num, NULL, 10, &pos) < 0) -- 2.1.0
On Wed, Sep 23, 2015 at 07:18:28PM -0400, John Ferlan wrote:
The 'strtok_r' function requires passing a NULL as the first parameter on subsequent calls in order to ensure the code picks up where it left off on a previous call. However, Coverity doesn't quite realize this and points out that if a NULL was passed in as the third argument it would result in a possible NULL deref because the strtok_r function will assign the third argument to the first in the call is NULL.
The description seems contradictory to the patch. From the asserts that fix it, I'd assume Coverity has a problem with the first argument possibly being NULL - usually obtained from a library function Coverity doesn't understand.
Adding an sa_assert() prior to each initial call quiets Coverity
Thus rendering the coverity scan useless.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/esx/esx_vi.c | 1 + src/libxl/libxl_conf.c | 1 + src/openvz/openvz_conf.c | 2 ++ src/xenapi/xenapi_utils.c | 1 + 4 files changed, 5 insertions(+)
diff --git a/src/esx/esx_vi.c b/src/esx/esx_vi.c index af822b1..a57d753 100644 --- a/src/esx/esx_vi.c +++ b/src/esx/esx_vi.c @@ -1173,6 +1173,7 @@ esxVI_Context_LookupManagedObjectsByPath(esxVI_Context *ctx, const char *path) goto cleanup;
/* Lookup Datacenter */ + sa_assert(tmp);
This should be asserted in the caller. It seems priv->parsedUri->path can be NULL in esxConnectToVCenter, but this function is not called in that case.
item = strtok_r(tmp, "/", &saveptr);
if (!item) { diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index 35fd495..ab588e8 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -353,6 +353,7 @@ libxlCapsInitGuests(libxl_ctx *ctx, virCapsPtr caps) /* Split capabilities string into tokens. strtok_r is OK here because * we "own" the buffer. Parse out the features from each token. */ + sa_assert(ver_info->capabilities);
This one should be closer to the function that filled out the string.
for (str = ver_info->capabilities, nr_guest_archs = 0; nr_guest_archs < sizeof(guest_archs) / sizeof(guest_archs[0]) && (token = strtok_r(str, " ", &saveptr)) != NULL; diff --git a/src/openvz/openvz_conf.c b/src/openvz/openvz_conf.c index db0a9a7..003272e 100644 --- a/src/openvz/openvz_conf.c +++ b/src/openvz/openvz_conf.c @@ -138,6 +138,7 @@ openvzParseBarrierLimit(const char* value, char *str; int ret = -1;
+ sa_assert(value);
Same here.
if (VIR_STRDUP(str, value) < 0) goto error;
@@ -965,6 +966,7 @@ openvzGetVPSUUID(int vpsid, char *uuidstr, size_t len) } }
+ sa_assert(line);
This one already is right after getline.
iden = strtok_r(line, " ", &saveptr); uuidbuf = strtok_r(NULL, "\n", &saveptr);
diff --git a/src/xenapi/xenapi_utils.c b/src/xenapi/xenapi_utils.c index a80e084..6b710cd 100644 --- a/src/xenapi/xenapi_utils.c +++ b/src/xenapi/xenapi_utils.c @@ -301,6 +301,7 @@ getCpuBitMapfromString(char *mask, unsigned char *cpumap, int maplen) int max_bits = maplen * 8; char *num = NULL, *bp = NULL; bzero(cpumap, maplen); + sa_assert(mask); num = strtok_r(mask, ",", &bp); while (num != NULL) { if (virStrToLong_i(num, NULL, 10, &pos) < 0)
virBitmap* functions can be used instead. Jan
On 09/24/2015 04:13 AM, Ján Tomko wrote:
On Wed, Sep 23, 2015 at 07:18:28PM -0400, John Ferlan wrote:
The 'strtok_r' function requires passing a NULL as the first parameter on subsequent calls in order to ensure the code picks up where it left off on a previous call. However, Coverity doesn't quite realize this and points out that if a NULL was passed in as the third argument it would result in a possible NULL deref because the strtok_r function will assign the third argument to the first in the call is NULL.
The description seems contradictory to the patch. From the asserts that fix it, I'd assume Coverity has a problem with the first argument possibly being NULL - usually obtained from a library function Coverity doesn't understand.
Been a while since I wrote this one and I've forgotten the details - what I do "see" is according to Coverity for each of the instances below, the strtok_r ends up calling __strtok_r_1c for some reason. Although there are other strtok_r calls which don't end up in that same path (some in the same module. I'll dig a bit deeper on each. John
Adding an sa_assert() prior to each initial call quiets Coverity
Thus rendering the coverity scan useless.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/esx/esx_vi.c | 1 + src/libxl/libxl_conf.c | 1 + src/openvz/openvz_conf.c | 2 ++ src/xenapi/xenapi_utils.c | 1 + 4 files changed, 5 insertions(+)
diff --git a/src/esx/esx_vi.c b/src/esx/esx_vi.c index af822b1..a57d753 100644 --- a/src/esx/esx_vi.c +++ b/src/esx/esx_vi.c @@ -1173,6 +1173,7 @@ esxVI_Context_LookupManagedObjectsByPath(esxVI_Context *ctx, const char *path) goto cleanup;
/* Lookup Datacenter */ + sa_assert(tmp);
This should be asserted in the caller. It seems priv->parsedUri->path can be NULL in esxConnectToVCenter, but this function is not called in that case.
item = strtok_r(tmp, "/", &saveptr);
if (!item) { diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index 35fd495..ab588e8 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -353,6 +353,7 @@ libxlCapsInitGuests(libxl_ctx *ctx, virCapsPtr caps) /* Split capabilities string into tokens. strtok_r is OK here because * we "own" the buffer. Parse out the features from each token. */ + sa_assert(ver_info->capabilities);
This one should be closer to the function that filled out the string.
for (str = ver_info->capabilities, nr_guest_archs = 0; nr_guest_archs < sizeof(guest_archs) / sizeof(guest_archs[0]) && (token = strtok_r(str, " ", &saveptr)) != NULL; diff --git a/src/openvz/openvz_conf.c b/src/openvz/openvz_conf.c index db0a9a7..003272e 100644 --- a/src/openvz/openvz_conf.c +++ b/src/openvz/openvz_conf.c @@ -138,6 +138,7 @@ openvzParseBarrierLimit(const char* value, char *str; int ret = -1;
+ sa_assert(value);
Same here.
if (VIR_STRDUP(str, value) < 0) goto error;
@@ -965,6 +966,7 @@ openvzGetVPSUUID(int vpsid, char *uuidstr, size_t len) } }
+ sa_assert(line);
This one already is right after getline.
iden = strtok_r(line, " ", &saveptr); uuidbuf = strtok_r(NULL, "\n", &saveptr);
diff --git a/src/xenapi/xenapi_utils.c b/src/xenapi/xenapi_utils.c index a80e084..6b710cd 100644 --- a/src/xenapi/xenapi_utils.c +++ b/src/xenapi/xenapi_utils.c @@ -301,6 +301,7 @@ getCpuBitMapfromString(char *mask, unsigned char *cpumap, int maplen) int max_bits = maplen * 8; char *num = NULL, *bp = NULL; bzero(cpumap, maplen); + sa_assert(mask); num = strtok_r(mask, ",", &bp); while (num != NULL) { if (virStrToLong_i(num, NULL, 10, &pos) < 0)
virBitmap* functions can be used instead.
Jan
In the error path need to unref the 'caps' as well Signed-off-by: John Ferlan <jferlan@redhat.com> --- tests/qemucaps2xmltest.c | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/qemucaps2xmltest.c b/tests/qemucaps2xmltest.c index 6a5811c..7adde24 100644 --- a/tests/qemucaps2xmltest.c +++ b/tests/qemucaps2xmltest.c @@ -113,6 +113,7 @@ testGetCaps(char *capsData, const testQemuData *data) error: virObjectUnref(qemuCaps); + virObjectUnref(caps); return NULL; } -- 2.1.0
On Wed, Sep 23, 2015 at 07:18:29PM -0400, John Ferlan wrote:
In the error path need to unref the 'caps' as well
Signed-off-by: John Ferlan <jferlan@redhat.com> --- tests/qemucaps2xmltest.c | 1 + 1 file changed, 1 insertion(+)
ACK Jan
The cleanup path did not clear the reference for sk1 and sk2 Signed-off-by: John Ferlan <jferlan@redhat.com> --- tests/virnetdaemontest.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/virnetdaemontest.c b/tests/virnetdaemontest.c index fb8a6c0..24cbd54 100644 --- a/tests/virnetdaemontest.c +++ b/tests/virnetdaemontest.c @@ -125,6 +125,8 @@ testCreateServer(const char *host, int family) virObjectUnref(cln2); virObjectUnref(svc1); virObjectUnref(svc2); + virObjectUnref(sk1); + virObjectUnref(sk2); return srv; error: -- 2.1.0
On Wed, Sep 23, 2015 at 07:18:30PM -0400, John Ferlan wrote:
The cleanup path did not clear the reference for sk1 and sk2
Signed-off-by: John Ferlan <jferlan@redhat.com> --- tests/virnetdaemontest.c | 2 ++ 1 file changed, 2 insertions(+)
ACK Jan
Use 'dead_error_condition' instead of 'dead_error_begin' Signed-off-by: John Ferlan <jferlan@redhat.com> --- tools/virsh.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/virsh.c b/tools/virsh.c index 76b5e9f..7484bed 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -118,7 +118,7 @@ virshCatchDisconnect(virConnectPtr conn, case VIR_CONNECT_CLOSE_REASON_KEEPALIVE: str = N_("Disconnected from %s due to keepalive timeout"); break; - /* coverity[dead_error_begin] */ + /* coverity[dead_error_condition] */ case VIR_CONNECT_CLOSE_REASON_CLIENT: case VIR_CONNECT_CLOSE_REASON_LAST: break; -- 2.1.0
On Wed, Sep 23, 2015 at 07:18:31PM -0400, John Ferlan wrote:
Use 'dead_error_condition' instead of 'dead_error_begin'
Just wondering - what is the difference? Doesn't dead_error_begin require a dead_error_end?
Signed-off-by: John Ferlan <jferlan@redhat.com> --- tools/virsh.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
ACK. Jan
On 09/24/2015 03:47 AM, Ján Tomko wrote:
On Wed, Sep 23, 2015 at 07:18:31PM -0400, John Ferlan wrote:
Use 'dead_error_condition' instead of 'dead_error_begin'
Just wondering - what is the difference?
Perhaps just the one condition and not a set of conditions or lines? Not quite sure, but you know where to find the internal output and you'll see the data spewed by Coverity about the deadcode.
Doesn't dead_error_begin require a dead_error_end?
No - once it's dead, it's not coming back to life. John
Signed-off-by: John Ferlan <jferlan@redhat.com> --- tools/virsh.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
ACK.
Jan
Coverity complains that return from virHookCall is not checked in one place in qemuProcessStop. Since the comment notes that we cannot stop the operation even it if fails, just added the ignore_value. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_process.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index b961f40..eb04883 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -5276,9 +5276,9 @@ void qemuProcessStop(virQEMUDriverPtr driver, char *xml = qemuDomainDefFormatXML(driver, vm->def, 0); /* we can't stop the operation even if the script raised an error */ - virHookCall(VIR_HOOK_DRIVER_QEMU, vm->def->name, - VIR_HOOK_QEMU_OP_STOPPED, VIR_HOOK_SUBOP_END, - NULL, xml, NULL); + ignore_value(virHookCall(VIR_HOOK_DRIVER_QEMU, vm->def->name, + VIR_HOOK_QEMU_OP_STOPPED, VIR_HOOK_SUBOP_END, + NULL, xml, NULL)); VIR_FREE(xml); } -- 2.1.0
On Wed, Sep 23, 2015 at 07:18:32PM -0400, John Ferlan wrote:
Coverity complains that return from virHookCall is not checked in one place in qemuProcessStop. Since the comment notes that we cannot stop the operation even it if fails, just added the ignore_value.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_process.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
ACK Jan
This seemed to be more of a false positive as for some reason Coverity was missing the "ret < 0" goto error condition and somehow believing that event could be overwritten. At first I thought it was just the ret != 0 condition difference, but it wasn't. In any case, make use of the recent change to qemuDomainEventQueue to check event == NULL and just pass it as a parameter directly in the error path. That avoids the error. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_driver.c | 24 +++++++++++------------- 1 file changed, 11 insertions(+), 13 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 2387cf3..3a98774 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -3155,7 +3155,7 @@ qemuDomainSaveMemory(virQEMUDriverPtr driver, virFileWrapperFdFree(wrapperFd); VIR_FREE(xml); - if (ret != 0 && needUnlink) + if (ret < 0 && needUnlink) unlink(path); return ret; @@ -3174,7 +3174,6 @@ qemuDomainSaveInternal(virQEMUDriverPtr driver, virDomainPtr dom, char *xml = NULL; bool was_running = false; int ret = -1; - int rc; virObjectEventPtr event = NULL; qemuDomainObjPrivatePtr priv = vm->privateData; virCapsPtr caps; @@ -3249,21 +3248,20 @@ qemuDomainSaveInternal(virQEMUDriverPtr driver, virDomainPtr dom, /* Shut it down */ qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_SAVED, 0); virDomainAuditStop(vm, "saved"); - event = virDomainEventLifecycleNewFromObj(vm, - VIR_DOMAIN_EVENT_STOPPED, - VIR_DOMAIN_EVENT_STOPPED_SAVED); + event = virDomainEventLifecycleNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED, + VIR_DOMAIN_EVENT_STOPPED_SAVED); endjob: - if (ret != 0) { + if (ret < 0) { if (was_running && virDomainObjIsActive(vm)) { virErrorPtr save_err = virSaveLastError(); - rc = qemuProcessStartCPUs(driver, vm, dom->conn, - VIR_DOMAIN_RUNNING_SAVE_CANCELED, - QEMU_ASYNC_JOB_SAVE); - if (rc < 0) { + if (qemuProcessStartCPUs(driver, vm, dom->conn, + VIR_DOMAIN_RUNNING_SAVE_CANCELED, + QEMU_ASYNC_JOB_SAVE) < 0) { VIR_WARN("Unable to resume guest CPUs after save failure"); - event = virDomainEventLifecycleNewFromObj(vm, - VIR_DOMAIN_EVENT_SUSPENDED, - VIR_DOMAIN_EVENT_SUSPENDED_API_ERROR); + qemuDomainEventQueue(driver, + virDomainEventLifecycleNewFromObj(vm, + VIR_DOMAIN_EVENT_SUSPENDED, + VIR_DOMAIN_EVENT_SUSPENDED_API_ERROR)); } virSetError(save_err); virFreeError(save_err); -- 2.1.0
On Wed, Sep 23, 2015 at 07:18:33PM -0400, John Ferlan wrote:
This seemed to be more of a false positive as for some reason Coverity was missing the "ret < 0" goto error condition and somehow believing that event could be overwritten. At first I thought it was just the ret != 0 condition difference, but it wasn't.
Then those changes are cosmetic and don't need to be in a commit fixing the RESOURCE_LEAK.
In any case, make use of the recent change to qemuDomainEventQueue to check event == NULL and just pass it as a parameter directly in the error path. That avoids the error.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_driver.c | 24 +++++++++++------------- 1 file changed, 11 insertions(+), 13 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 2387cf3..3a98774 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -3155,7 +3155,7 @@ qemuDomainSaveMemory(virQEMUDriverPtr driver, virFileWrapperFdFree(wrapperFd); VIR_FREE(xml);
- if (ret != 0 && needUnlink) + if (ret < 0 && needUnlink) unlink(path);
return ret; @@ -3174,7 +3174,6 @@ qemuDomainSaveInternal(virQEMUDriverPtr driver, virDomainPtr dom, char *xml = NULL; bool was_running = false; int ret = -1; - int rc; virObjectEventPtr event = NULL; qemuDomainObjPrivatePtr priv = vm->privateData; virCapsPtr caps; @@ -3249,21 +3248,20 @@ qemuDomainSaveInternal(virQEMUDriverPtr driver, virDomainPtr dom, /* Shut it down */ qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_SAVED, 0); virDomainAuditStop(vm, "saved"); - event = virDomainEventLifecycleNewFromObj(vm, - VIR_DOMAIN_EVENT_STOPPED, - VIR_DOMAIN_EVENT_STOPPED_SAVED); + event = virDomainEventLifecycleNewFromObj(vm, VIR_DOMAIN_EVENT_STOPPED, + VIR_DOMAIN_EVENT_STOPPED_SAVED); endjob: - if (ret != 0) { + if (ret < 0) { if (was_running && virDomainObjIsActive(vm)) { virErrorPtr save_err = virSaveLastError(); - rc = qemuProcessStartCPUs(driver, vm, dom->conn, - VIR_DOMAIN_RUNNING_SAVE_CANCELED, - QEMU_ASYNC_JOB_SAVE); - if (rc < 0) { + if (qemuProcessStartCPUs(driver, vm, dom->conn, + VIR_DOMAIN_RUNNING_SAVE_CANCELED, + QEMU_ASYNC_JOB_SAVE) < 0) { VIR_WARN("Unable to resume guest CPUs after save failure");
- event = virDomainEventLifecycleNewFromObj(vm, - VIR_DOMAIN_EVENT_SUSPENDED, - VIR_DOMAIN_EVENT_SUSPENDED_API_ERROR); + qemuDomainEventQueue(driver, + virDomainEventLifecycleNewFromObj(vm, + VIR_DOMAIN_EVENT_SUSPENDED, + VIR_DOMAIN_EVENT_SUSPENDED_API_ERROR));
ACK to this hunk as the coverity fix, and the rest pushed separately. Jan
On 09/24/2015 03:45 AM, Ján Tomko wrote:
On Wed, Sep 23, 2015 at 07:18:33PM -0400, John Ferlan wrote:
This seemed to be more of a false positive as for some reason Coverity was missing the "ret < 0" goto error condition and somehow believing that event could be overwritten. At first I thought it was just the ret != 0 condition difference, but it wasn't.
Then those changes are cosmetic and don't need to be in a commit fixing the RESOURCE_LEAK.
OK - I've split this patch in two - cosmetic and resource_leak and pushed the patches that have been ACK'd... I'll continue to plug through issues noted from patch 1 along with the ones referenced in my cover letter. Tks - John
On 09/23/2015 05:18 PM, John Ferlan wrote:
Newer Coverity (7.7.0) found a couple real issues and a few more false positives. There's still a few more to be resolved, but still trying to figure them out...
libxlDomainMigrationPrepare - claim is args is leaked. Although it seems to be handled in libxlMigrateReceive or libxlDoMigrateReceive. Don't know the code well enough to do proper triage.
args is a subclass of virObject. It is freed when refcnt reaches zero and the dispose function is called. args is created (refcnt=1) in libxlDomainMigrationPrepare() and added to the virNetSocket IO callback. When the sender connects, the callback invokes libxlMigrateReceive() passing args. libxlMigrateReceive() starts a thread to receive the migration data (avoids blocking the event loop while receiving the migration data :-)), passing args to the thread start function, libxlDoMigrateReceive(). args is unref'ed in libxlMigrateReceive() if there are any failures, otherwise args is unref'ed in libxlDoMigrateReceive(). Regards, Jim
participants (3)
-
Jim Fehlig -
John Ferlan -
Ján Tomko