[libvirt] [PATCH] virsh: plug memory leaks on failure path
Leaks are introduced in commit 1cf0e3d and fe383bb. * tools/virsh.c (cmdSnapshotList): fix memory leaks. * How to reproduce? % virsh snapshot-list foo --parent --roots error: --parent and --roots are mutually exclusive error: Failed to disconnect from the hypervisor, 1 leaked reference(s) % virsh snapshot-list foo --parent --tree error: --parent and --tree are mutually exclusive error: Failed to disconnect from the hypervisor, 1 leaked reference(s) % virsh snapshot-list foo --roots --tree error: --roots and --tree are mutually exclusive error: Failed to disconnect from the hypervisor, 1 leaked reference(s) ...... Signed-off-by: Alex Jia <ajia@redhat.com> --- tools/virsh.c | 5 +++++ 1 files changed, 5 insertions(+), 0 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index 5009b6b..f5c3b60 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -16357,11 +16357,13 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd) if (vshCommandOptBool(cmd, "roots")) { vshError(ctl, "%s", _("--parent and --roots are mutually exclusive")); + virDomainFree(dom); return false; } if (tree) { vshError(ctl, "%s", _("--parent and --tree are mutually exclusive")); + virDomainFree(dom); return false; } parent_filter = 1; @@ -16369,11 +16371,13 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd) if (tree) { vshError(ctl, "%s", _("--roots and --tree are mutually exclusive")); + virDomainFree(dom); return false; } if (from) { vshError(ctl, "%s", _("--roots and --from are mutually exclusive")); + virDomainFree(dom); } flags |= VIR_DOMAIN_SNAPSHOT_LIST_ROOTS; } @@ -16381,6 +16385,7 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd) if (tree) { vshError(ctl, "%s", _("--leaves and --tree are mutually exclusive")); + virDomainFree(dom); return false; } flags |= VIR_DOMAIN_SNAPSHOT_LIST_LEAVES; -- 1.7.1
On 03/28/2012 02:58 PM, Alex Jia wrote:
Leaks are introduced in commit 1cf0e3d and fe383bb.
* tools/virsh.c (cmdSnapshotList): fix memory leaks.
* How to reproduce?
% virsh snapshot-list foo --parent --roots error: --parent and --roots are mutually exclusive
error: Failed to disconnect from the hypervisor, 1 leaked reference(s)
% virsh snapshot-list foo --parent --tree error: --parent and --tree are mutually exclusive
error: Failed to disconnect from the hypervisor, 1 leaked reference(s)
% virsh snapshot-list foo --roots --tree error: --roots and --tree are mutually exclusive
error: Failed to disconnect from the hypervisor, 1 leaked reference(s)
......
Signed-off-by: Alex Jia<ajia@redhat.com> --- tools/virsh.c | 5 +++++ 1 files changed, 5 insertions(+), 0 deletions(-)
diff --git a/tools/virsh.c b/tools/virsh.c index 5009b6b..f5c3b60 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -16357,11 +16357,13 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd) if (vshCommandOptBool(cmd, "roots")) { vshError(ctl, "%s", _("--parent and --roots are mutually exclusive")); + virDomainFree(dom); return false;
Why not "goto cleanup;" There is a "goto cleanup" right before, so either change the previous "goto cleanup" into "virDomainFree(dom); return false;" too, or uses the "goto cleanup;" following.
} if (tree) { vshError(ctl, "%s", _("--parent and --tree are mutually exclusive")); + virDomainFree(dom); return false; } parent_filter = 1; @@ -16369,11 +16371,13 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd) if (tree) { vshError(ctl, "%s", _("--roots and --tree are mutually exclusive")); + virDomainFree(dom); return false; } if (from) { vshError(ctl, "%s", _("--roots and --from are mutually exclusive")); + virDomainFree(dom);
This should be a bug, "return" or "goto cleanup;" is missed. But it can be a seperate patch.
} flags |= VIR_DOMAIN_SNAPSHOT_LIST_ROOTS; } @@ -16381,6 +16385,7 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd) if (tree) { vshError(ctl, "%s", _("--leaves and --tree are mutually exclusive")); + virDomainFree(dom); return false; } flags |= VIR_DOMAIN_SNAPSHOT_LIST_LEAVES;
ACK with "goto cleanup;"
On 03/28/2012 04:12 PM, Osier Yang wrote:
On 03/28/2012 02:58 PM, Alex Jia wrote:
Leaks are introduced in commit 1cf0e3d and fe383bb.
* tools/virsh.c (cmdSnapshotList): fix memory leaks.
* How to reproduce?
% virsh snapshot-list foo --parent --roots error: --parent and --roots are mutually exclusive
error: Failed to disconnect from the hypervisor, 1 leaked reference(s)
% virsh snapshot-list foo --parent --tree error: --parent and --tree are mutually exclusive
error: Failed to disconnect from the hypervisor, 1 leaked reference(s)
% virsh snapshot-list foo --roots --tree error: --roots and --tree are mutually exclusive
error: Failed to disconnect from the hypervisor, 1 leaked reference(s)
......
Signed-off-by: Alex Jia<ajia@redhat.com> --- tools/virsh.c | 5 +++++ 1 files changed, 5 insertions(+), 0 deletions(-)
diff --git a/tools/virsh.c b/tools/virsh.c index 5009b6b..f5c3b60 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -16357,11 +16357,13 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd) if (vshCommandOptBool(cmd, "roots")) { vshError(ctl, "%s", _("--parent and --roots are mutually exclusive")); + virDomainFree(dom); return false;
Why not "goto cleanup;" There is a "goto cleanup" right before, Yeah, I forgot 'ret' has a 'false' initial value. so either change the previous "goto cleanup" into "virDomainFree(dom); return false;" too, or uses the "goto cleanup;" following.
} if (tree) { vshError(ctl, "%s", _("--parent and --tree are mutually exclusive")); + virDomainFree(dom); return false; } parent_filter = 1; @@ -16369,11 +16371,13 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd) if (tree) { vshError(ctl, "%s", _("--roots and --tree are mutually exclusive")); + virDomainFree(dom); return false; } if (from) { vshError(ctl, "%s", _("--roots and --from are mutually exclusive")); + virDomainFree(dom);
This should be a bug, "return" or "goto cleanup;" is missed. But it can be a seperate patch.
Yeah, I should include previous change in here.
} flags |= VIR_DOMAIN_SNAPSHOT_LIST_ROOTS; } @@ -16381,6 +16385,7 @@ cmdSnapshotList(vshControl *ctl, const vshCmd *cmd) if (tree) { vshError(ctl, "%s", _("--leaves and --tree are mutually exclusive")); + virDomainFree(dom); return false; } flags |= VIR_DOMAIN_SNAPSHOT_LIST_LEAVES;
ACK with "goto cleanup;"
Thanks, Alex
participants (2)
-
Alex Jia -
Osier Yang