[libvirt] [PATCH 0/5] Resolve parallels and virCommandPtr resource leaks
This is v3 of the parallels_driver.c changes. The most recent review pointed out that virCommandPtr's need to be free'd. I found a few other instances where they weren't free'd. I also found a few instances where the status check from virCommandRun was not < 0 and cleaned them to be more consistent. John Ferlan (5): parallels: Resolve some resource leaks security: Need to add virCommandFree() storage: Need to add virCommandFree() util: Need to add virCommandFree() parallels_utils: Check return status properly from virCommandRun() src/parallels/parallels_driver.c | 47 ++++++++++++++++++++++++---------------- src/parallels/parallels_utils.c | 2 +- src/security/security_apparmor.c | 1 + src/storage/storage_backend_fs.c | 2 ++ src/util/virnetdevopenvswitch.c | 2 ++ 5 files changed, 34 insertions(+), 20 deletions(-) -- 1.7.11.7
Be sure to VIR_FREE(accel) and moved virDomainVideoDefFree() within no_memory label to be consistent Resolve resource leak in parallelsApplyIfaceParams() when the 'oldnet' is allocated locally. Also virCommandFree(cmd) as necessary. --- src/parallels/parallels_driver.c | 47 ++++++++++++++++++++++++---------------- 1 file changed, 28 insertions(+), 19 deletions(-) diff --git a/src/parallels/parallels_driver.c b/src/parallels/parallels_driver.c index 1b47246..9c88d71 100644 --- a/src/parallels/parallels_driver.c +++ b/src/parallels/parallels_driver.c @@ -271,17 +271,17 @@ parallelsAddVideoInfo(virDomainDefPtr def, virJSONValuePtr value) if (!(tmp = virJSONValueObjectGetString(value, "size"))) { parallelsParseError(); - goto cleanup; + goto error; } if (virStrToLong_ul(tmp, &endptr, 10, &mem) < 0) { parallelsParseError(); - goto cleanup; + goto error; } if (!STREQ(endptr, "Mb")) { parallelsParseError(); - goto cleanup; + goto error; } if (VIR_ALLOC(video) < 0) @@ -304,8 +304,9 @@ parallelsAddVideoInfo(virDomainDefPtr def, virJSONValuePtr value) no_memory: virReportOOMError(); -cleanup: + VIR_FREE(accel); virDomainVideoDefFree(video); +error: return -1; } @@ -1645,7 +1646,7 @@ static int parallelsAddHddByVolume(parallelsDomObjPtr pdom, virCommandAddArgFormat(cmd, "--position=%d", disk->info.addr.drive.target); - if (virCommandRun(cmd, NULL)) + if (virCommandRun(cmd, NULL) < 0) goto cleanup; if (parallelsStorageVolumeDefRemove(pool, voldef)) @@ -1793,9 +1794,10 @@ static int parallelsApplyIfaceParams(parallelsDomObjPtr pdom, { bool create = false; bool is_changed = false; - virCommandPtr cmd; + virCommandPtr cmd = NULL; char strmac[VIR_MAC_STRING_BUFLEN]; int i; + int ret = -1; if (!oldnet) { create = true; @@ -1808,58 +1810,58 @@ static int parallelsApplyIfaceParams(parallelsDomObjPtr pdom, if (!create && oldnet->type != newnet->type) { virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", _("Changing network type is not supported")); - return -1; + goto cleanup; } if (!STREQ_NULLABLE(oldnet->model, newnet->model)) { virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", _("Changing network device model is not supported")); - return -1; + goto cleanup; } if (!STREQ_NULLABLE(oldnet->data.network.portgroup, newnet->data.network.portgroup)) { virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", _("Changing network portgroup is not supported")); - return -1; + goto cleanup; } if (!virNetDevVPortProfileEqual(oldnet->virtPortProfile, newnet->virtPortProfile)) { virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", _("Changing virtual port profile is not supported")); - return -1; + goto cleanup; } if (newnet->tune.sndbuf_specified) { virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", _("Setting send buffer size is not supported")); - return -1; + goto cleanup; } if (!STREQ_NULLABLE(oldnet->script, newnet->script)) { virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", _("Setting startup script is not supported")); - return -1; + goto cleanup; } if (!STREQ_NULLABLE(oldnet->filter, newnet->filter)) { virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", _("Changing filter params is not supported")); - return -1; + goto cleanup; } if (newnet->bandwidth != NULL) { virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", _("Setting bandwidth params is not supported")); - return -1; + goto cleanup; } for (i = 0; i < sizeof(newnet->vlan); i++) { if (((char *)&newnet->vlan)[i] != 0) { virReportError(VIR_ERR_ARGUMENT_UNSUPPORTED, "%s", _("Setting vlan params is not supported")); - return -1; + goto cleanup; } } @@ -1903,13 +1905,20 @@ static int parallelsApplyIfaceParams(parallelsDomObjPtr pdom, if (!create && !is_changed) { /* nothing changed - no need to run prlctl */ - return 0; + ret = 0; + goto cleanup; } - if (virCommandRun(cmd, NULL)) - return -1; + if (virCommandRun(cmd, NULL) < 0) + goto cleanup; - return 0; + ret = 0; + +cleanup: + if (create) + VIR_FREE(oldnet); + virCommandFree(cmd); + return ret; } static int -- 1.7.11.7
On 23.01.2013 23:04, John Ferlan wrote:
Be sure to VIR_FREE(accel) and moved virDomainVideoDefFree() within no_memory label to be consistent
Resolve resource leak in parallelsApplyIfaceParams() when the 'oldnet' is allocated locally. Also virCommandFree(cmd) as necessary. --- src/parallels/parallels_driver.c | 47 ++++++++++++++++++++++++---------------- 1 file changed, 28 insertions(+), 19 deletions(-)
diff --git a/src/parallels/parallels_driver.c b/src/parallels/parallels_driver.c index 1b47246..9c88d71 100644 --- a/src/parallels/parallels_driver.c +++ b/src/parallels/parallels_driver.c @@ -271,17 +271,17 @@ parallelsAddVideoInfo(virDomainDefPtr def, virJSONValuePtr value)
if (!(tmp = virJSONValueObjectGetString(value, "size"))) { parallelsParseError(); - goto cleanup; + goto error; }
if (virStrToLong_ul(tmp, &endptr, 10, &mem) < 0) { parallelsParseError(); - goto cleanup; + goto error; }
if (!STREQ(endptr, "Mb")) { parallelsParseError(); - goto cleanup; + goto error; }
if (VIR_ALLOC(video) < 0) @@ -304,8 +304,9 @@ parallelsAddVideoInfo(virDomainDefPtr def, virJSONValuePtr value)
no_memory: virReportOOMError(); -cleanup: + VIR_FREE(accel); virDomainVideoDefFree(video);
While there is not much sense in virDomainVideoDefDree() here with current code, I agree to use it rather than bare VIR_FREE() esp. when the code might change and we will forget about it. Michal
--- src/security/security_apparmor.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c index 7331c91..6dcf6be 100644 --- a/src/security/security_apparmor.c +++ b/src/security/security_apparmor.c @@ -201,6 +201,7 @@ load_profile(virSecurityManagerPtr mgr, virCommandSetInputBuffer(cmd, xml); rc = virCommandRun(cmd, NULL); + virCommandFree(cmd); clean: VIR_FREE(xml); -- 1.7.11.7
On 23.01.2013 23:04, John Ferlan wrote:
--- src/security/security_apparmor.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c index 7331c91..6dcf6be 100644 --- a/src/security/security_apparmor.c +++ b/src/security/security_apparmor.c @@ -201,6 +201,7 @@ load_profile(virSecurityManagerPtr mgr,
virCommandSetInputBuffer(cmd, xml); rc = virCommandRun(cmd, NULL); + virCommandFree(cmd);
clean: VIR_FREE(xml);
While technically, your patch is correct, I prefer having virCommandFree within cleanup label (yeah, there's just 'clean'). Michal
--- src/storage/storage_backend_fs.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index 21589a6..a582804 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -648,6 +648,8 @@ virStorageBackendExecuteMKFS(const char *device, format, device); ret = -1; } + + virCommandFree(cmd); return ret; } #else /* #ifdef MKFS */ -- 1.7.11.7
--- src/util/virnetdevopenvswitch.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/util/virnetdevopenvswitch.c b/src/util/virnetdevopenvswitch.c index 47c3db8..4fe077a 100644 --- a/src/util/virnetdevopenvswitch.c +++ b/src/util/virnetdevopenvswitch.c @@ -211,6 +211,7 @@ int virNetDevOpenvswitchGetMigrateData(char **migrate, const char *ifname) (*migrate)[strlen(*migrate) - 1] = '\0'; ret = 0; cleanup: + virCommandFree(cmd); return ret; } @@ -242,5 +243,6 @@ int virNetDevOpenvswitchSetMigrateData(char *migrate, const char *ifname) ret = 0; cleanup: + virCommandFree(cmd); return ret; } -- 1.7.11.7
--- src/parallels/parallels_utils.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/parallels/parallels_utils.c b/src/parallels/parallels_utils.c index dd2f0b7..171f5d0 100644 --- a/src/parallels/parallels_utils.c +++ b/src/parallels/parallels_utils.c @@ -42,7 +42,7 @@ parallelsDoCmdRun(char **outbuf, const char *binary, va_list list) if (outbuf) virCommandSetOutputBuffer(cmd, outbuf); - if (virCommandRun(cmd, NULL)) + if (virCommandRun(cmd, NULL) < 0) goto cleanup; ret = 0; -- 1.7.11.7
On 23.01.2013 23:04, John Ferlan wrote:
This is v3 of the parallels_driver.c changes. The most recent review pointed out that virCommandPtr's need to be free'd. I found a few other instances where they weren't free'd. I also found a few instances where the status check from virCommandRun was not < 0 and cleaned them to be more consistent.
John Ferlan (5): parallels: Resolve some resource leaks security: Need to add virCommandFree() storage: Need to add virCommandFree() util: Need to add virCommandFree() parallels_utils: Check return status properly from virCommandRun()
src/parallels/parallels_driver.c | 47 ++++++++++++++++++++++++---------------- src/parallels/parallels_utils.c | 2 +- src/security/security_apparmor.c | 1 + src/storage/storage_backend_fs.c | 2 ++ src/util/virnetdevopenvswitch.c | 2 ++ 5 files changed, 34 insertions(+), 20 deletions(-)
ACKed and pushed. Michal
participants (2)
-
John Ferlan -
Michal Privoznik