* [Qemu-devel] [PATCH] sockets: avoid leak of listen file descriptor
@ 2017-10-20 9:28 Daniel P. Berrange
2017-10-20 9:54 ` Daniel P. Berrange
2017-10-20 10:02 ` Darren Kenny
0 siblings, 2 replies; 4+ messages in thread
From: Daniel P. Berrange @ 2017-10-20 9:28 UTC (permalink / raw)
To: qemu-devel; +Cc: Paolo Bonzini, Eric Blake, Daniel P. Berrange
If we iterate over the full port range without successfully binding+listening
on the socket, we'll try the next address, whereupon we overwrite the slisten
file descriptor variable without closing it.
Rather than having two places where we open + close socket FDs on different
iterations of nested for loops, re-arrange the code to always open+close
within the same loop iteration.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
util/qemu-sockets.c | 31 +++++++++++++++++++------------
1 file changed, 19 insertions(+), 12 deletions(-)
diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index b47fb45885..a319338cca 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -207,7 +207,7 @@ static int inet_listen_saddr(InetSocketAddress *saddr,
char uaddr[INET6_ADDRSTRLEN+1];
char uport[33];
int rc, port_min, port_max, p;
- int slisten = 0;
+ int slisten = -1;
int saved_errno = 0;
bool socket_created = false;
Error *err = NULL;
@@ -267,16 +267,28 @@ static int inet_listen_saddr(InetSocketAddress *saddr,
uaddr,INET6_ADDRSTRLEN,uport,32,
NI_NUMERICHOST | NI_NUMERICSERV);
- slisten = create_fast_reuse_socket(e);
- if (slisten < 0) {
- continue;
- }
-
socket_created = true;
port_min = inet_getport(e);
port_max = saddr->has_to ? saddr->to + port_offset : port_min;
for (p = port_min; p <= port_max; p++) {
inet_setport(e, p);
+
+ slisten = create_fast_reuse_socket(e);
+ if (slisten < 0) {
+ /* First time we expect we might fail to create the socket
+ * eg if 'e' has AF_INET6 but ipv6 kmod is not loaded.
+ * Later iterations should always succeeed if first iteration
+ * worked though, so treat that as fatal.
+ */
+ if (p == port_min) {
+ continue;
+ } else {
+ error_setg_errno(errp, errno,
+ "Failed to recreate failed listening socket");
+ goto listen_failed;
+ }
+ }
+
rc = try_bind(slisten, saddr, e);
if (rc) {
if (errno == EADDRINUSE) {
@@ -299,12 +311,7 @@ static int inet_listen_saddr(InetSocketAddress *saddr,
* socket to allow bind attempts for subsequent ports:
*/
closesocket(slisten);
- slisten = create_fast_reuse_socket(e);
- if (slisten < 0) {
- error_setg_errno(errp, errno,
- "Failed to recreate failed listening socket");
- goto listen_failed;
- }
+ slisten = -1;
}
}
error_setg_errno(errp, errno,
--
2.13.6
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH] sockets: avoid leak of listen file descriptor
2017-10-20 9:28 [Qemu-devel] [PATCH] sockets: avoid leak of listen file descriptor Daniel P. Berrange
@ 2017-10-20 9:54 ` Daniel P. Berrange
2017-10-20 10:03 ` Darren Kenny
2017-10-20 10:02 ` Darren Kenny
1 sibling, 1 reply; 4+ messages in thread
From: Daniel P. Berrange @ 2017-10-20 9:54 UTC (permalink / raw)
To: qemu-devel; +Cc: Paolo Bonzini, Eric Blake
On Fri, Oct 20, 2017 at 10:28:44AM +0100, Daniel P. Berrange wrote:
> If we iterate over the full port range without successfully binding+listening
> on the socket, we'll try the next address, whereupon we overwrite the slisten
> file descriptor variable without closing it.
>
> Rather than having two places where we open + close socket FDs on different
> iterations of nested for loops, re-arrange the code to always open+close
> within the same loop iteration.
>
> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> ---
> util/qemu-sockets.c | 31 +++++++++++++++++++------------
> 1 file changed, 19 insertions(+), 12 deletions(-)
Self-NACK this has a bug too
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH] sockets: avoid leak of listen file descriptor
2017-10-20 9:28 [Qemu-devel] [PATCH] sockets: avoid leak of listen file descriptor Daniel P. Berrange
2017-10-20 9:54 ` Daniel P. Berrange
@ 2017-10-20 10:02 ` Darren Kenny
1 sibling, 0 replies; 4+ messages in thread
From: Darren Kenny @ 2017-10-20 10:02 UTC (permalink / raw)
To: Daniel P. Berrange; +Cc: qemu-devel, Paolo Bonzini
Hi Daniel,
On Fri, Oct 20, 2017 at 10:28:44AM +0100, Daniel P. Berrange wrote:
>If we iterate over the full port range without successfully binding+listening
>on the socket, we'll try the next address, whereupon we overwrite the slisten
>file descriptor variable without closing it.
>
>Rather than having two places where we open + close socket FDs on different
>iterations of nested for loops, re-arrange the code to always open+close
>within the same loop iteration.
>
>Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
>---
> util/qemu-sockets.c | 31 +++++++++++++++++++------------
> 1 file changed, 19 insertions(+), 12 deletions(-)
>
>diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
>index b47fb45885..a319338cca 100644
>--- a/util/qemu-sockets.c
>+++ b/util/qemu-sockets.c
>@@ -207,7 +207,7 @@ static int inet_listen_saddr(InetSocketAddress *saddr,
> char uaddr[INET6_ADDRSTRLEN+1];
> char uport[33];
> int rc, port_min, port_max, p;
>- int slisten = 0;
>+ int slisten = -1;
> int saved_errno = 0;
> bool socket_created = false;
> Error *err = NULL;
>@@ -267,16 +267,28 @@ static int inet_listen_saddr(InetSocketAddress *saddr,
> uaddr,INET6_ADDRSTRLEN,uport,32,
> NI_NUMERICHOST | NI_NUMERICSERV);
>
>- slisten = create_fast_reuse_socket(e);
>- if (slisten < 0) {
>- continue;
>- }
>-
> socket_created = true;
Is this setting of socket_created correct now? Maybe it should be moved to
before the try_bind() below?
> port_min = inet_getport(e);
> port_max = saddr->has_to ? saddr->to + port_offset : port_min;
> for (p = port_min; p <= port_max; p++) {
> inet_setport(e, p);
>+
>+ slisten = create_fast_reuse_socket(e);
>+ if (slisten < 0) {
>+ /* First time we expect we might fail to create the socket
>+ * eg if 'e' has AF_INET6 but ipv6 kmod is not loaded.
>+ * Later iterations should always succeeed if first iteration
>+ * worked though, so treat that as fatal.
>+ */
>+ if (p == port_min) {
>+ continue;
>+ } else {
>+ error_setg_errno(errp, errno,
>+ "Failed to recreate failed listening socket");
>+ goto listen_failed;
>+ }
>+ }
>+
> rc = try_bind(slisten, saddr, e);
> if (rc) {
> if (errno == EADDRINUSE) {
>@@ -299,12 +311,7 @@ static int inet_listen_saddr(InetSocketAddress *saddr,
> * socket to allow bind attempts for subsequent ports:
> */
> closesocket(slisten);
>- slisten = create_fast_reuse_socket(e);
>- if (slisten < 0) {
>- error_setg_errno(errp, errno,
>- "Failed to recreate failed listening socket");
>- goto listen_failed;
>- }
>+ slisten = -1;
> }
> }
> error_setg_errno(errp, errno,
Thanks,
Darren.
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH] sockets: avoid leak of listen file descriptor
2017-10-20 9:54 ` Daniel P. Berrange
@ 2017-10-20 10:03 ` Darren Kenny
0 siblings, 0 replies; 4+ messages in thread
From: Darren Kenny @ 2017-10-20 10:03 UTC (permalink / raw)
To: Daniel P. Berrange; +Cc: qemu-devel, Paolo Bonzini
On Fri, Oct 20, 2017 at 10:54:31AM +0100, Daniel P. Berrange wrote:
>On Fri, Oct 20, 2017 at 10:28:44AM +0100, Daniel P. Berrange wrote:
>> If we iterate over the full port range without successfully binding+listening
>> on the socket, we'll try the next address, whereupon we overwrite the slisten
>> file descriptor variable without closing it.
>>
>> Rather than having two places where we open + close socket FDs on different
>> iterations of nested for loops, re-arrange the code to always open+close
>> within the same loop iteration.
>>
>> Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
>> ---
>> util/qemu-sockets.c | 31 +++++++++++++++++++------------
>> 1 file changed, 19 insertions(+), 12 deletions(-)
>
>Self-NACK this has a bug too
Missed this...
Darren.
>
>
>Regards,
>Daniel
>--
>|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
>|: https://libvirt.org -o- https://fstop138.berrange.com :|
>|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2017-10-20 10:03 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-10-20 9:28 [Qemu-devel] [PATCH] sockets: avoid leak of listen file descriptor Daniel P. Berrange
2017-10-20 9:54 ` Daniel P. Berrange
2017-10-20 10:03 ` Darren Kenny
2017-10-20 10:02 ` Darren Kenny
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).