[libvirt] [PATCH] admin_server: fix memory leak
If caller of adminConnectListServers() pass NULL instead of servers we need to free the list we've received from virNetDaemonGetServers(). Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- daemon/admin_server.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/daemon/admin_server.c b/daemon/admin_server.c index 1d16bc9..2f26488 100644 --- a/daemon/admin_server.c +++ b/daemon/admin_server.c @@ -52,6 +52,8 @@ adminConnectListServers(virNetDaemonPtr dmn, if (servers) { *servers = srvs; srvs = NULL; + } else { + virObjectListFreeCount(srvs, ret); } cleanup: return ret; -- 2.7.4
On 24/03/16 10:52, Pavel Hrdina wrote:
If caller of adminConnectListServers() pass NULL instead of servers we need to free the list we've received from virNetDaemonGetServers().
Good catch.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- daemon/admin_server.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/daemon/admin_server.c b/daemon/admin_server.c index 1d16bc9..2f26488 100644 --- a/daemon/admin_server.c +++ b/daemon/admin_server.c @@ -52,6 +52,8 @@ adminConnectListServers(virNetDaemonPtr dmn, if (servers) { *servers = srvs; srvs = NULL; + } else { + virObjectListFreeCount(srvs, ret); }
No need to do it here ^^ actually, you can safely 'free' it in cleanup. "srvs = NULL" guarantees that even if list was requested and non-null, calling free in cleanup section will finish without any harm.
cleanup: return ret;
ACK with that adjustment. Erik
On Thu, Mar 24, 2016 at 01:34:04PM +0100, Erik Skultety wrote:
On 24/03/16 10:52, Pavel Hrdina wrote:
If caller of adminConnectListServers() pass NULL instead of servers we need to free the list we've received from virNetDaemonGetServers().
Good catch.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- daemon/admin_server.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/daemon/admin_server.c b/daemon/admin_server.c index 1d16bc9..2f26488 100644 --- a/daemon/admin_server.c +++ b/daemon/admin_server.c @@ -52,6 +52,8 @@ adminConnectListServers(virNetDaemonPtr dmn, if (servers) { *servers = srvs; srvs = NULL; + } else { + virObjectListFreeCount(srvs, ret); }
No need to do it here ^^ actually, you can safely 'free' it in cleanup. "srvs = NULL" guarantees that even if list was requested and non-null, calling free in cleanup section will finish without any harm.
It was my intention to make it clear that only in this case we would free the srvs, I know, that it's safe to pass NULL to that function.
cleanup: return ret;
ACK with that adjustment.
Ok, I'll move it to cleanup :) thanks. Pavel
On 24.03.2016 14:02, Pavel Hrdina wrote:
On Thu, Mar 24, 2016 at 01:34:04PM +0100, Erik Skultety wrote:
On 24/03/16 10:52, Pavel Hrdina wrote:
If caller of adminConnectListServers() pass NULL instead of servers we need to free the list we've received from virNetDaemonGetServers().
Good catch.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- daemon/admin_server.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/daemon/admin_server.c b/daemon/admin_server.c index 1d16bc9..2f26488 100644 --- a/daemon/admin_server.c +++ b/daemon/admin_server.c @@ -52,6 +52,8 @@ adminConnectListServers(virNetDaemonPtr dmn, if (servers) { *servers = srvs; srvs = NULL; + } else { + virObjectListFreeCount(srvs, ret); }
No need to do it here ^^ actually, you can safely 'free' it in cleanup. "srvs = NULL" guarantees that even if list was requested and non-null, calling free in cleanup section will finish without any harm.
It was my intention to make it clear that only in this case we would free the srvs, I know, that it's safe to pass NULL to that function.
cleanup: return ret;
ACK with that adjustment.
Ok, I'll move it to cleanup :) thanks.
Oh, you forgot to guard it with check for ret > 0. Otherwise a terrible thing will happen. Michal
participants (3)
-
Erik Skultety -
Michal Privoznik -
Pavel Hrdina