5:33 p.m.
Patch 1 was already sent, I'd like to get it in for the release,
it should be quite safe.
Remaining patches centralize some cleanup code and fix a potential
crasher, but since it should be rare I'd like to wait till after
the release.
Previous thread: http://www.redhat.com/archives/libvir-list/2016-April/msg01618.html
Ben Gray (1):
rpc: Don't leak fd via CreateXMLWithFiles
Cole Robinson (3):
rpc: Add virNetMessageClearPayload
rpc: Clear more in virNetMessageClearPayload
rpc: use virNetMessageClearPayload in client
src/libvirt_remote.syms | 1 +
src/rpc/virnetclient.c | 5 +----
src/rpc/virnetmessage.c | 28 +++++++++++++++++++---------
src/rpc/virnetmessage.h | 2 ++
4 files changed, 23 insertions(+), 13 deletions(-)
--
2.7.4
5:33 p.m.
New subject: [libvirt] [PATCH 1/4] rpc: Don't leak fd via CreateXMLWithFiles
From: Ben Gray <ben.r.gray(a)gmail.com>
FD passing APIs like CreateXMLWithFiles or OpenGraphicsFD will leak
file descriptors. The user passes in an fd, which is dup()'d in
virNetClientProgramCall. The new fd is what is transfered to the
server virNetClientIOWriteMessage.
Once all the fds have been written though, the parent msg->fds list
is immediately free'd, so the individual fds are never closed.
This closes each FD as its send to the server, so all fds have been
closed by the time msg->fds is free'd.
https://bugzilla.redhat.com/show_bug.cgi?id=1159766
---
src/rpc/virnetclient.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/src/rpc/virnetclient.c b/src/rpc/virnetclient.c
index 781e74c..d8ed15b 100644
--- a/src/rpc/virnetclient.c
+++ b/src/rpc/virnetclient.c
@@ -1184,6 +1184,7 @@ virNetClientIOWriteMessage(virNetClientPtr client,
if (rv == 0) /* Blocking */
return 0;
thecall->msg->donefds++;
+ VIR_FORCE_CLOSE(thecall->msg->fds[i]);
}
thecall->msg->donefds = 0;
thecall->msg->bufferOffset = thecall->msg->bufferLength = 0;
--
2.7.4
1:55 a.m.
New subject: [libvirt] [PATCH 1/4] rpc: Don't leak fd via CreateXMLWithFiles
On Wed, Apr 27, 2016 at 06:33:58PM -0400, Cole Robinson wrote:
From: Ben Gray <ben.r.gray(a)gmail.com>
FD passing APIs like CreateXMLWithFiles or OpenGraphicsFD will leak
file descriptors. The user passes in an fd, which is dup()'d in
virNetClientProgramCall. The new fd is what is transfered to the
server virNetClientIOWriteMessage.
Once all the fds have been written though, the parent msg->fds list
is immediately free'd, so the individual fds are never closed.
This closes each FD as its send to the server, so all fds have been
closed by the time msg->fds is free'd.
https://bugzilla.redhat.com/show_bug.cgi?id=1159766
---
ACK and safe for release. Sorry for the noise with previous patch. I've
reviewed the code closely and we are leaking the FD for the client side.
5:47 a.m.
New subject: [libvirt] [PATCH 1/4] rpc: Don't leak fd via CreateXMLWithFiles
On 04/28/2016 02:55 AM, Pavel Hrdina wrote:
On Wed, Apr 27, 2016 at 06:33:58PM -0400, Cole Robinson wrote:
> From: Ben Gray <ben.r.gray(a)gmail.com>
>
> FD passing APIs like CreateXMLWithFiles or OpenGraphicsFD will leak
> file descriptors. The user passes in an fd, which is dup()'d in
> virNetClientProgramCall. The new fd is what is transfered to the
> server virNetClientIOWriteMessage.
>
> Once all the fds have been written though, the parent msg->fds list
> is immediately free'd, so the individual fds are never closed.
>
> This closes each FD as its send to the server, so all fds have been
> closed by the time msg->fds is free'd.
>
> https://bugzilla.redhat.com/show_bug.cgi?id=1159766
> ---
ACK and safe for release. Sorry for the noise with previous patch. I've
reviewed the code closely and we are leaking the FD for the client side.
No worries, pushed now
- Cole
5:33 p.m.
New subject: [libvirt] [PATCH 2/4] rpc: Add virNetMessageClearPayload
Handles freeing the buffer and fds, but not the message details.
Use it to drop some duplicate code.
---
src/rpc/virnetmessage.c | 22 +++++++++++++---------
1 file changed, 13 insertions(+), 9 deletions(-)
diff --git a/src/rpc/virnetmessage.c b/src/rpc/virnetmessage.c
index 5c57128..3df502a 100644
--- a/src/rpc/virnetmessage.c
+++ b/src/rpc/virnetmessage.c
@@ -49,17 +49,25 @@ virNetMessagePtr virNetMessageNew(bool tracked)
}
-void virNetMessageClear(virNetMessagePtr msg)
+static void
+virNetMessageClearPayload(virNetMessagePtr msg)
{
- bool tracked = msg->tracked;
size_t i;
- VIR_DEBUG("msg=%p nfds=%zu", msg, msg->nfds);
-
for (i = 0; i < msg->nfds; i++)
VIR_FORCE_CLOSE(msg->fds[i]);
VIR_FREE(msg->fds);
VIR_FREE(msg->buffer);
+}
+
+
+void virNetMessageClear(virNetMessagePtr msg)
+{
+ bool tracked = msg->tracked;
+
+ VIR_DEBUG("msg=%p nfds=%zu", msg, msg->nfds);
+
+ virNetMessageClearPayload(msg);
memset(msg, 0, sizeof(*msg));
msg->tracked = tracked;
}
@@ -67,7 +75,6 @@ void virNetMessageClear(virNetMessagePtr msg)
void virNetMessageFree(virNetMessagePtr msg)
{
- size_t i;
if (!msg)
return;
@@ -76,10 +83,7 @@ void virNetMessageFree(virNetMessagePtr msg)
if (msg->cb)
msg->cb(msg, msg->opaque);
- for (i = 0; i < msg->nfds; i++)
- VIR_FORCE_CLOSE(msg->fds[i]);
- VIR_FREE(msg->buffer);
- VIR_FREE(msg->fds);
+ virNetMessageClearPayload(msg);
VIR_FREE(msg);
}
--
2.7.4
5:34 p.m.
New subject: [libvirt] [PATCH 3/4] rpc: Clear more in virNetMessageClearPayload
Set all counters to 0. This doesn't impact current users, but
future users will want this
---
src/rpc/virnetmessage.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/src/rpc/virnetmessage.c b/src/rpc/virnetmessage.c
index 3df502a..673fb8d 100644
--- a/src/rpc/virnetmessage.c
+++ b/src/rpc/virnetmessage.c
@@ -56,7 +56,13 @@ virNetMessageClearPayload(virNetMessagePtr msg)
for (i = 0; i < msg->nfds; i++)
VIR_FORCE_CLOSE(msg->fds[i]);
+
+ msg->donefds = 0;
+ msg->nfds = 0;
VIR_FREE(msg->fds);
+
+ msg->bufferOffset = 0;
+ msg->bufferLength = 0;
VIR_FREE(msg->buffer);
}
--
2.7.4
5:34 p.m.
New subject: [libvirt] [PATCH 4/4] rpc: use virNetMessageClearPayload in client
This removes the opencoded payload freeing in the client, to use
the shared virNetMessageClearPayload call. Two changes:
- ClearPayload sets nfds=0, which fixes a potential crash if
an error path called virNetMessageFree/Clear on the message
after fds was free'd
- We drop the inner loop VIR_FORCE_CLOSE... this may mean fds are
kept open a little bit longer if the call is blocking but in
practice I don't think it will have any effect
---
src/libvirt_remote.syms | 1 +
src/rpc/virnetclient.c | 6 +-----
src/rpc/virnetmessage.c | 2 +-
src/rpc/virnetmessage.h | 2 ++
4 files changed, 5 insertions(+), 6 deletions(-)
diff --git a/src/libvirt_remote.syms b/src/libvirt_remote.syms
index 66f9383..2c0b7f3 100644
--- a/src/libvirt_remote.syms
+++ b/src/libvirt_remote.syms
@@ -79,6 +79,7 @@ virNetDaemonUpdateServices;
# rpc/virnetmessage.h
virNetMessageClear;
+virNetMessageClearPayload;
virNetMessageDecodeHeader;
virNetMessageDecodeLength;
virNetMessageDecodeNumFDs;
diff --git a/src/rpc/virnetclient.c b/src/rpc/virnetclient.c
index d8ed15b..9c0d190 100644
--- a/src/rpc/virnetclient.c
+++ b/src/rpc/virnetclient.c
@@ -1184,12 +1184,8 @@ virNetClientIOWriteMessage(virNetClientPtr client,
if (rv == 0) /* Blocking */
return 0;
thecall->msg->donefds++;
- VIR_FORCE_CLOSE(thecall->msg->fds[i]);
}
- thecall->msg->donefds = 0;
- thecall->msg->bufferOffset = thecall->msg->bufferLength = 0;
- VIR_FREE(thecall->msg->fds);
- VIR_FREE(thecall->msg->buffer);
+ virNetMessageClearPayload(thecall->msg);
if (thecall->expectReply)
thecall->mode = VIR_NET_CLIENT_MODE_WAIT_RX;
else
diff --git a/src/rpc/virnetmessage.c b/src/rpc/virnetmessage.c
index 673fb8d..c3a2e59 100644
--- a/src/rpc/virnetmessage.c
+++ b/src/rpc/virnetmessage.c
@@ -49,7 +49,7 @@ virNetMessagePtr virNetMessageNew(bool tracked)
}
-static void
+void
virNetMessageClearPayload(virNetMessagePtr msg)
{
size_t i;
diff --git a/src/rpc/virnetmessage.h b/src/rpc/virnetmessage.h
index 89a2ebf..d7406fc 100644
--- a/src/rpc/virnetmessage.h
+++ b/src/rpc/virnetmessage.h
@@ -54,6 +54,8 @@ struct _virNetMessage {
virNetMessagePtr virNetMessageNew(bool tracked);
+void virNetMessageClearPayload(virNetMessagePtr msg);
+
void virNetMessageClear(virNetMessagePtr);
void virNetMessageFree(virNetMessagePtr msg);
--
2.7.4
12:30 p.m.
New subject: [libvirt] [PATCH 4/4] rpc: use virNetMessageClearPayload in client
On Wed, Apr 27, 2016 at 06:34:01PM -0400, Cole Robinson wrote:
This removes the opencoded payload freeing in the client, to use
the shared virNetMessageClearPayload call. Two changes:
- ClearPayload sets nfds=0, which fixes a potential crash if
an error path called virNetMessageFree/Clear on the message
after fds was free'd
- We drop the inner loop VIR_FORCE_CLOSE... this may mean fds are
kept open a little bit longer if the call is blocking but in
practice I don't think it will have any effect
---
ACK
10:15 a.m.
On 04/27/2016 06:33 PM, Cole Robinson wrote:
Patch 1 was already sent, I'd like to get it in for the release,
it should be quite safe.
Remaining patches centralize some cleanup code and fix a potential
crasher, but since it should be rare I'd like to wait till after
the release.
Previous thread: http://www.redhat.com/archives/libvir-list/2016-April/msg01618.html
Ben Gray (1):
rpc: Don't leak fd via CreateXMLWithFiles
Cole Robinson (3):
rpc: Add virNetMessageClearPayload
rpc: Clear more in virNetMessageClearPayload
rpc: use virNetMessageClearPayload in client
src/libvirt_remote.syms | 1 +
src/rpc/virnetclient.c | 5 +----
src/rpc/virnetmessage.c | 28 +++++++++++++++++++---------
src/rpc/virnetmessage.h | 2 ++
4 files changed, 23 insertions(+), 13 deletions(-)
ping? (for patch 2-4)
- Cole
3158
days inactive
3166
days old
10 comments
2 participants
participants (2)
-
Cole Robinson -
Pavel Hrdina