[libvirt] [PATCH 0/4] rpc: Fix fd leak and potential crash
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
From: Ben Gray <ben.r.gray@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
On Wed, Apr 27, 2016 at 06:33:58PM -0400, Cole Robinson wrote:
From: Ben Gray <ben.r.gray@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.
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.
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@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.
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
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
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
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
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
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
participants (2)
-
Cole Robinson -
Pavel Hrdina