12:38 p.m.
Actually, the preceding patch fixed only the one leak that had been
introduced in the last month or so. Looking at the many other functions
that do the same sort of thing (call qemuMonitorJSONMakeCommand, and
later virJSONValueFree), I saw that they all had exactly the same leak.
So this amended patch fixes all of them:
From 28f820354dcae9950cad042ea78a893fd9475830 Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyering(a)redhat.com>
Date: Wed, 27 Jan 2010 09:58:12 +0100
Subject: [PATCH] qemu_monitor_json.c: avoid many unconditional leaks
* src/qemu/qemu_monitor_json.c (qemuMonitorJSONAttachDrive):
Don't leak the buffer behind a virJSONValuePtr.
(qemuMonitorJSONStartCPUs): Likewise.
(qemuMonitorJSONStopCPUs, qemuMonitorJSONSystemPowerdown): Likewise.
(qemuMonitorJSONGetCPUInfo, qemuMonitorJSONGetBalloonInfo): Likewise.
(qemuMonitorJSONGetBlockStatsInfo): Likewise.
(qemuMonitorJSONSetVNCPassword): Likewise.
(qemuMonitorJSONSetBalloon, qemuMonitorJSONEjectMedia): Likewise.
(qemuMonitorJSONChangeMedia, qemuMonitorJSONSaveMemory): Likewise.
(qemuMonitorJSONSetMigrationSpeed): Likewise.
(qemuMonitorJSONGetMigrationStatus, qemuMonitorJSONMigrate): Likewise.
(qemuMonitorJSONMigrateCancel, qemuMonitorJSONAddUSB): Likewise.
(qemuMonitorJSONAddPCIHostDevice, qemuMonitorJSONAddPCIDisk): Likewise.
(qemuMonitorJSONAddPCINetwork, qemuMonitorJSONRemovePCIDevice): Likewise.
(qemuMonitorJSONSendFileHandle): Likewise.
(qemuMonitorJSONCloseFileHandle): Likewise.
(qemuMonitorJSONAddHostNetwork): Likewise.
(qemuMonitorJSONRemoveHostNetwork): Likewise.
(qemuMonitorJSONGetPtyPaths): Likewise.
(qemuMonitorJSONAttachPCIDiskController): Likewise.
---
src/qemu/qemu_monitor_json.c | 27 +++++++++++++++++++++++++++
1 files changed, 27 insertions(+), 0 deletions(-)
diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
index 8e88c7e..b6c3449 100644
--- a/src/qemu/qemu_monitor_json.c
+++ b/src/qemu/qemu_monitor_json.c
@@ -492,6 +492,7 @@ qemuMonitorJSONStartCPUs(qemuMonitorPtr mon,
ret = qemuMonitorJSONCheckError(cmd, reply);
virJSONValueFree(cmd);
+ VIR_FREE(cmd);
virJSONValueFree(reply);
return ret;
}
@@ -512,6 +513,7 @@ qemuMonitorJSONStopCPUs(qemuMonitorPtr mon)
ret = qemuMonitorJSONCheckError(cmd, reply);
virJSONValueFree(cmd);
+ VIR_FREE(cmd);
virJSONValueFree(reply);
return ret;
}
@@ -531,6 +533,7 @@ int qemuMonitorJSONSystemPowerdown(qemuMonitorPtr mon)
ret = qemuMonitorJSONCheckError(cmd, reply);
virJSONValueFree(cmd);
+ VIR_FREE(cmd);
virJSONValueFree(reply);
return ret;
}
@@ -637,6 +640,7 @@ int qemuMonitorJSONGetCPUInfo(qemuMonitorPtr mon,
ret = qemuMonitorJSONExtractCPUInfo(reply, pids);
virJSONValueFree(cmd);
+ VIR_FREE(cmd);
virJSONValueFree(reply);
return ret;
}
@@ -696,6 +700,7 @@ int qemuMonitorJSONGetBalloonInfo(qemuMonitorPtr mon,
cleanup:
virJSONValueFree(cmd);
+ VIR_FREE(cmd);
virJSONValueFree(reply);
return ret;
}
@@ -800,6 +805,7 @@ int qemuMonitorJSONGetBlockStatsInfo(qemuMonitorPtr mon,
cleanup:
virJSONValueFree(cmd);
+ VIR_FREE(cmd);
virJSONValueFree(reply);
return ret;
}
@@ -824,6 +830,7 @@ int qemuMonitorJSONSetVNCPassword(qemuMonitorPtr mon,
ret = qemuMonitorJSONCheckError(cmd, reply);
virJSONValueFree(cmd);
+ VIR_FREE(cmd);
virJSONValueFree(reply);
return ret;
}
@@ -861,6 +868,7 @@ int qemuMonitorJSONSetBalloon(qemuMonitorPtr mon,
cleanup:
virJSONValueFree(cmd);
+ VIR_FREE(cmd);
virJSONValueFree(reply);
return ret;
}
@@ -884,6 +892,7 @@ int qemuMonitorJSONEjectMedia(qemuMonitorPtr mon,
ret = qemuMonitorJSONCheckError(cmd, reply);
virJSONValueFree(cmd);
+ VIR_FREE(cmd);
virJSONValueFree(reply);
return ret;
}
@@ -918,6 +927,7 @@ int qemuMonitorJSONChangeMedia(qemuMonitorPtr mon,
ret = qemuMonitorJSONCheckError(cmd, reply);
virJSONValueFree(cmd);
+ VIR_FREE(cmd);
virJSONValueFree(reply);
return ret;
}
@@ -945,6 +955,7 @@ static int qemuMonitorJSONSaveMemory(qemuMonitorPtr mon,
ret = qemuMonitorJSONCheckError(cmd, reply);
virJSONValueFree(cmd);
+ VIR_FREE(cmd);
virJSONValueFree(reply);
return ret;
}
@@ -991,6 +1002,7 @@ int qemuMonitorJSONSetMigrationSpeed(qemuMonitorPtr mon,
ret = qemuMonitorJSONCheckError(cmd, reply);
virJSONValueFree(cmd);
+ VIR_FREE(cmd);
virJSONValueFree(reply);
return ret;
}
@@ -1084,6 +1096,7 @@ int qemuMonitorJSONGetMigrationStatus(qemuMonitorPtr mon,
ret = -1;
virJSONValueFree(cmd);
+ VIR_FREE(cmd);
virJSONValueFree(reply);
return ret;
}
@@ -1109,6 +1122,7 @@ static int qemuMonitorJSONMigrate(qemuMonitorPtr mon,
ret = qemuMonitorJSONCheckError(cmd, reply);
virJSONValueFree(cmd);
+ VIR_FREE(cmd);
virJSONValueFree(reply);
return ret;
}
@@ -1206,6 +1220,7 @@ int qemuMonitorJSONMigrateCancel(qemuMonitorPtr mon)
ret = qemuMonitorJSONCheckError(cmd, reply);
virJSONValueFree(cmd);
+ VIR_FREE(cmd);
virJSONValueFree(reply);
return ret;
}
@@ -1229,6 +1244,7 @@ static int qemuMonitorJSONAddUSB(qemuMonitorPtr mon,
ret = qemuMonitorJSONCheckError(cmd, reply);
virJSONValueFree(cmd);
+ VIR_FREE(cmd);
virJSONValueFree(reply);
return ret;
}
@@ -1369,6 +1385,7 @@ int qemuMonitorJSONAddPCIHostDevice(qemuMonitorPtr mon,
ret = -1;
virJSONValueFree(cmd);
+ VIR_FREE(cmd);
virJSONValueFree(reply);
return ret;
}
@@ -1410,6 +1427,7 @@ int qemuMonitorJSONAddPCIDisk(qemuMonitorPtr mon,
ret = -1;
virJSONValueFree(cmd);
+ VIR_FREE(cmd);
virJSONValueFree(reply);
return ret;
}
@@ -1442,6 +1460,7 @@ int qemuMonitorJSONAddPCINetwork(qemuMonitorPtr mon,
ret = -1;
virJSONValueFree(cmd);
+ VIR_FREE(cmd);
virJSONValueFree(reply);
return ret;
}
@@ -1475,6 +1494,7 @@ int qemuMonitorJSONRemovePCIDevice(qemuMonitorPtr mon,
ret = qemuMonitorJSONCheckError(cmd, reply);
virJSONValueFree(cmd);
+ VIR_FREE(cmd);
virJSONValueFree(reply);
return ret;
}
@@ -1498,6 +1518,7 @@ int qemuMonitorJSONSendFileHandle(qemuMonitorPtr mon,
ret = qemuMonitorJSONCheckError(cmd, reply);
virJSONValueFree(cmd);
+ VIR_FREE(cmd);
virJSONValueFree(reply);
return ret;
}
@@ -1520,6 +1541,7 @@ int qemuMonitorJSONCloseFileHandle(qemuMonitorPtr mon,
ret = qemuMonitorJSONCheckError(cmd, reply);
virJSONValueFree(cmd);
+ VIR_FREE(cmd);
virJSONValueFree(reply);
return ret;
}
@@ -1542,6 +1564,7 @@ int qemuMonitorJSONAddHostNetwork(qemuMonitorPtr mon,
ret = qemuMonitorJSONCheckError(cmd, reply);
virJSONValueFree(cmd);
+ VIR_FREE(cmd);
virJSONValueFree(reply);
return ret;
}
@@ -1566,6 +1589,7 @@ int qemuMonitorJSONRemoveHostNetwork(qemuMonitorPtr mon,
ret = qemuMonitorJSONCheckError(cmd, reply);
virJSONValueFree(cmd);
+ VIR_FREE(cmd);
virJSONValueFree(reply);
return ret;
}
@@ -1665,6 +1689,7 @@ int qemuMonitorJSONGetPtyPaths(qemuMonitorPtr mon,
ret = qemuMonitorJSONExtractPtyPaths(reply, paths);
virJSONValueFree(cmd);
+ VIR_FREE(cmd);
virJSONValueFree(reply);
return ret;
}
@@ -1705,6 +1730,7 @@ int qemuMonitorJSONAttachPCIDiskController(qemuMonitorPtr mon,
ret = -1;
virJSONValueFree(cmd);
+ VIR_FREE(cmd);
virJSONValueFree(reply);
return ret;
}
@@ -1773,6 +1799,7 @@ int qemuMonitorJSONAttachDrive(qemuMonitorPtr mon,
ret = -1;
virJSONValueFree(cmd);
+ VIR_FREE(cmd);
virJSONValueFree(reply);
return ret;
}
--
1.7.0.rc0.140.gfbe7
1:11 p.m.
On Wed, Jan 27, 2010 at 10:38:55AM +0100, Jim Meyering wrote:
Actually, the preceding patch fixed only the one leak that had been
introduced in the last month or so. Looking at the many other functions
that do the same sort of thing (call qemuMonitorJSONMakeCommand, and
later virJSONValueFree), I saw that they all had exactly the same leak.
So this amended patch fixes all of them:
>From 28f820354dcae9950cad042ea78a893fd9475830 Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyering(a)redhat.com>
Date: Wed, 27 Jan 2010 09:58:12 +0100
Subject: [PATCH] qemu_monitor_json.c: avoid many unconditional leaks
The real bug is the virJSONValueFree() itself which is missing
the final VIR_FREE(value) call. By doing the free in the caller, we
still leak data for compound array/hash values.
Daniel
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
1:17 p.m.
Daniel P. Berrange wrote:
On Wed, Jan 27, 2010 at 10:38:55AM +0100, Jim Meyering wrote:
> Actually, the preceding patch fixed only the one leak that had been
> introduced in the last month or so. Looking at the many other functions
> that do the same sort of thing (call qemuMonitorJSONMakeCommand, and
> later virJSONValueFree), I saw that they all had exactly the same leak.
> So this amended patch fixes all of them:
>
> >From 28f820354dcae9950cad042ea78a893fd9475830 Mon Sep 17 00:00:00 2001
> From: Jim Meyering <meyering(a)redhat.com>
> Date: Wed, 27 Jan 2010 09:58:12 +0100
> Subject: [PATCH] qemu_monitor_json.c: avoid many unconditional leaks
The real bug is the virJSONValueFree() itself which is missing
the final VIR_FREE(value) call. By doing the free in the caller, we
still leak data for compound array/hash values.
Putting the VIR_FREE in virJSONValueFree was my first reflex, too,
but since coverity detected no leak for the adjacent
"virJSONValueFree(reply);" use, I assumed that doing so would
cause a problem:
virJSONValueFree(cmd);
VIR_FREE(cmd);
virJSONValueFree(reply);
1:24 p.m.
On Wed, Jan 27, 2010 at 11:17:32AM +0100, Jim Meyering wrote:
Daniel P. Berrange wrote:
> On Wed, Jan 27, 2010 at 10:38:55AM +0100, Jim Meyering wrote:
>> Actually, the preceding patch fixed only the one leak that had been
>> introduced in the last month or so. Looking at the many other functions
>> that do the same sort of thing (call qemuMonitorJSONMakeCommand, and
>> later virJSONValueFree), I saw that they all had exactly the same leak.
>> So this amended patch fixes all of them:
>>
>> >From 28f820354dcae9950cad042ea78a893fd9475830 Mon Sep 17 00:00:00 2001
>> From: Jim Meyering <meyering(a)redhat.com>
>> Date: Wed, 27 Jan 2010 09:58:12 +0100
>> Subject: [PATCH] qemu_monitor_json.c: avoid many unconditional leaks
>
> The real bug is the virJSONValueFree() itself which is missing
> the final VIR_FREE(value) call. By doing the free in the caller, we
> still leak data for compound array/hash values.
Putting the VIR_FREE in virJSONValueFree was my first reflex, too,
but since coverity detected no leak for the adjacent
"virJSONValueFree(reply);" use, I assumed that doing so would
cause a problem:
virJSONValueFree(cmd);
VIR_FREE(cmd);
virJSONValueFree(reply);
I think coverity must be confused then :-) The virJSONValueFree() method
is definitely intended to free any memory allocated during one of the
virJSONValueNewXXXX() methods.
Daniel
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
1:35 p.m.
Daniel P. Berrange wrote:
On Wed, Jan 27, 2010 at 11:17:32AM +0100, Jim Meyering wrote:
> Daniel P. Berrange wrote:
> > On Wed, Jan 27, 2010 at 10:38:55AM +0100, Jim Meyering wrote:
> >> Actually, the preceding patch fixed only the one leak that had been
> >> introduced in the last month or so. Looking at the many other functions
> >> that do the same sort of thing (call qemuMonitorJSONMakeCommand, and
> >> later virJSONValueFree), I saw that they all had exactly the same leak.
> >> So this amended patch fixes all of them:
> >>
> >> >From 28f820354dcae9950cad042ea78a893fd9475830 Mon Sep 17 00:00:00 2001
> >> From: Jim Meyering <meyering(a)redhat.com>
> >> Date: Wed, 27 Jan 2010 09:58:12 +0100
> >> Subject: [PATCH] qemu_monitor_json.c: avoid many unconditional leaks
> >
> > The real bug is the virJSONValueFree() itself which is missing
> > the final VIR_FREE(value) call. By doing the free in the caller, we
> > still leak data for compound array/hash values.
>
> Putting the VIR_FREE in virJSONValueFree was my first reflex, too,
> but since coverity detected no leak for the adjacent
> "virJSONValueFree(reply);" use, I assumed that doing so would
> cause a problem:
>
> virJSONValueFree(cmd);
> VIR_FREE(cmd);
> virJSONValueFree(reply);
I think coverity must be confused then :-) The virJSONValueFree() method
is definitely intended to free any memory allocated during one of the
virJSONValueNewXXXX() methods.
:-)
In that case, here's a replacement patch:
From 96196ca0b0cd71ae6b7f2dd7668432db95678e70 Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyering(a)redhat.com>
Date: Wed, 27 Jan 2010 09:58:12 +0100
Subject: [PATCH] json.c: avoid an unconditional leak from most qemuMonitorJSON* functions
* src/util/json.c (virJSONValueFree): Free the "value" pointer, too.
---
src/util/json.c | 5 +++--
1 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/src/util/json.c b/src/util/json.c
index a292e1b..1b3c359 100644
--- a/src/util/json.c
+++ b/src/util/json.c
@@ -2,7 +2,7 @@
* json.c: JSON object parsing/formatting
*
* Copyright (C) 2009 Daniel P. Berrange
- * Copyright (C) 2009 Red Hat, Inc.
+ * Copyright (C) 2009-2010 Red Hat, Inc.
*
* This library is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
@@ -82,8 +82,9 @@ void virJSONValueFree(virJSONValuePtr value)
case VIR_JSON_TYPE_NUMBER:
VIR_FREE(value->data.number);
break;
-
}
+
+ VIR_FREE(value);
}
--
1.7.0.rc0.140.gfbe7
7:01 p.m.
On Wed, Jan 27, 2010 at 11:35:15AM +0100, Jim Meyering wrote:
Daniel P. Berrange wrote:
> On Wed, Jan 27, 2010 at 11:17:32AM +0100, Jim Meyering wrote:
>> Daniel P. Berrange wrote:
>> > On Wed, Jan 27, 2010 at 10:38:55AM +0100, Jim Meyering wrote:
>> >> Actually, the preceding patch fixed only the one leak that had been
>> >> introduced in the last month or so. Looking at the many other
functions
>> >> that do the same sort of thing (call qemuMonitorJSONMakeCommand, and
>> >> later virJSONValueFree), I saw that they all had exactly the same
leak.
>> >> So this amended patch fixes all of them:
>> >>
>> >> >From 28f820354dcae9950cad042ea78a893fd9475830 Mon Sep 17 00:00:00
2001
>> >> From: Jim Meyering <meyering(a)redhat.com>
>> >> Date: Wed, 27 Jan 2010 09:58:12 +0100
>> >> Subject: [PATCH] qemu_monitor_json.c: avoid many unconditional leaks
>> >
>> > The real bug is the virJSONValueFree() itself which is missing
>> > the final VIR_FREE(value) call. By doing the free in the caller, we
>> > still leak data for compound array/hash values.
>>
>> Putting the VIR_FREE in virJSONValueFree was my first reflex, too,
>> but since coverity detected no leak for the adjacent
>> "virJSONValueFree(reply);" use, I assumed that doing so would
>> cause a problem:
>>
>> virJSONValueFree(cmd);
>> VIR_FREE(cmd);
>> virJSONValueFree(reply);
>
> I think coverity must be confused then :-) The virJSONValueFree() method
> is definitely intended to free any memory allocated during one of the
> virJSONValueNewXXXX() methods.
:-)
In that case, here's a replacement patch:
>From 96196ca0b0cd71ae6b7f2dd7668432db95678e70 Mon Sep 17 00:00:00 2001
From: Jim Meyering <meyering(a)redhat.com>
Date: Wed, 27 Jan 2010 09:58:12 +0100
Subject: [PATCH] json.c: avoid an unconditional leak from most qemuMonitorJSON*
functions
* src/util/json.c (virJSONValueFree): Free the "value" pointer, too.
---
src/util/json.c | 5 +++--
1 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/src/util/json.c b/src/util/json.c
index a292e1b..1b3c359 100644
--- a/src/util/json.c
+++ b/src/util/json.c
@@ -2,7 +2,7 @@
* json.c: JSON object parsing/formatting
*
* Copyright (C) 2009 Daniel P. Berrange
- * Copyright (C) 2009 Red Hat, Inc.
+ * Copyright (C) 2009-2010 Red Hat, Inc.
*
* This library is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
@@ -82,8 +82,9 @@ void virJSONValueFree(virJSONValuePtr value)
case VIR_JSON_TYPE_NUMBER:
VIR_FREE(value->data.number);
break;
-
}
+
+ VIR_FREE(value);
}
ACK, looks fine to me,
Daniel
--
Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/
daniel(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
9:49 p.m.
Daniel Veillard wrote:
...
> In that case, here's a replacement patch:
>
> Subject: [PATCH] json.c: avoid an unconditional leak from most qemuMonitorJSON*
functions
>
> * src/util/json.c (virJSONValueFree): Free the "value" pointer, too.
> ---
> src/util/json.c | 5 +++--
> 1 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/src/util/json.c b/src/util/json.c
...
> @@ -82,8 +82,9 @@ void virJSONValueFree(virJSONValuePtr value)
> case VIR_JSON_TYPE_NUMBER:
> VIR_FREE(value->data.number);
> break;
> -
> }
> +
> + VIR_FREE(value);
ACK, looks fine to me,
Thanks. Pushed.
5556
days inactive
5561
days old
6 comments
3 participants
participants (3)
-
Daniel P. Berrange -
Daniel Veillard -
Jim Meyering