| Message ID | 1323270109-24265-3-git-send-email-stefanha@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
On Wed, Dec 7, 2011 at 11:01 PM, Stefan Hajnoczi <stefanha@linux.vnet.ibm.com> wrote: > Today net/socket.c has no consistent policy for closing the socket file > descriptor when initialization fails. This means we leak the file > descriptor in some cases or we could also try to close it twice. > > Make error paths consistent by taking ownership of the file descriptor > and closing it on error. > > Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com> > --- > net/socket.c | 17 +++++++++-------- > 1 files changed, 9 insertions(+), 8 deletions(-) > > diff --git a/net/socket.c b/net/socket.c > index 613a7ef..f999c26 100644 > --- a/net/socket.c > +++ b/net/socket.c > @@ -266,14 +266,13 @@ static NetSocketState *net_socket_fd_init_dgram(VLANState *vlan, > if (saddr.sin_addr.s_addr == 0) { > fprintf(stderr, "qemu: error: init_dgram: fd=%d unbound, " > "cannot setup multicast dst addr\n", fd); > - return NULL; > + goto err; > } > /* clone dgram socket */ > newfd = net_socket_mcast_create(&saddr, NULL); > if (newfd < 0) { > /* error already reported by net_socket_mcast_create() */ > - close(fd); > - return NULL; > + goto err; > } > /* clone newfd to fd, close newfd */ > dup2(newfd, fd); > @@ -283,7 +282,7 @@ static NetSocketState *net_socket_fd_init_dgram(VLANState *vlan, > fprintf(stderr, > "qemu: error: init_dgram: fd=%d failed getsockname(): %s\n", > fd, strerror(errno)); > - return NULL; > + goto err; > } > } > > @@ -304,6 +303,10 @@ static NetSocketState *net_socket_fd_init_dgram(VLANState *vlan, > if (is_connected) s->dgram_dst=saddr; > > return s; > + > +err: > + closesocket(fd); > + return NULL; > } > > static void net_socket_connect(void *opaque) > @@ -353,6 +356,7 @@ static NetSocketState *net_socket_fd_init(VLANState *vlan, > (socklen_t *)&optlen)< 0) { > fprintf(stderr, "qemu: error: getsockopt(SO_TYPE) for fd=%d failed\n", > fd); > + closesocket(fd); > return NULL; > } > switch(so_type) { > @@ -386,9 +390,7 @@ static void net_socket_accept(void *opaque) > } > } > s1 = net_socket_fd_init(s->vlan, s->model, s->name, fd, 1); > - if (!s1) { > - closesocket(fd); > - } else { Why is it not handled when s1 is NULL? > + if (s1) { > snprintf(s1->nc.info_str, sizeof(s1->nc.info_str), > "socket: connection from %s:%d", > inet_ntoa(saddr.sin_addr), ntohs(saddr.sin_port)); > @@ -549,7 +551,6 @@ int net_init_socket(QemuOpts *opts, > } > > if (!net_socket_fd_init(vlan, "socket", name, fd, 1)) { > - close(fd); > return -1; > } > } else if (qemu_opt_get(opts, "listen")) { > -- > 1.7.7.3 > >
On Thu, Dec 8, 2011 at 12:29 PM, Zhi Yong Wu <zwu.kernel@gmail.com> wrote: > On Wed, Dec 7, 2011 at 11:01 PM, Stefan Hajnoczi > <stefanha@linux.vnet.ibm.com> wrote: >> Today net/socket.c has no consistent policy for closing the socket file >> descriptor when initialization fails. This means we leak the file >> descriptor in some cases or we could also try to close it twice. >> >> Make error paths consistent by taking ownership of the file descriptor >> and closing it on error. >> >> Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com> >> --- >> net/socket.c | 17 +++++++++-------- >> 1 files changed, 9 insertions(+), 8 deletions(-) >> >> diff --git a/net/socket.c b/net/socket.c >> index 613a7ef..f999c26 100644 >> --- a/net/socket.c >> +++ b/net/socket.c >> @@ -266,14 +266,13 @@ static NetSocketState *net_socket_fd_init_dgram(VLANState *vlan, >> if (saddr.sin_addr.s_addr == 0) { >> fprintf(stderr, "qemu: error: init_dgram: fd=%d unbound, " >> "cannot setup multicast dst addr\n", fd); >> - return NULL; >> + goto err; >> } >> /* clone dgram socket */ >> newfd = net_socket_mcast_create(&saddr, NULL); >> if (newfd < 0) { >> /* error already reported by net_socket_mcast_create() */ >> - close(fd); >> - return NULL; >> + goto err; >> } >> /* clone newfd to fd, close newfd */ >> dup2(newfd, fd); >> @@ -283,7 +282,7 @@ static NetSocketState *net_socket_fd_init_dgram(VLANState *vlan, >> fprintf(stderr, >> "qemu: error: init_dgram: fd=%d failed getsockname(): %s\n", >> fd, strerror(errno)); >> - return NULL; >> + goto err; >> } >> } >> >> @@ -304,6 +303,10 @@ static NetSocketState *net_socket_fd_init_dgram(VLANState *vlan, >> if (is_connected) s->dgram_dst=saddr; >> >> return s; >> + >> +err: >> + closesocket(fd); >> + return NULL; >> } >> >> static void net_socket_connect(void *opaque) >> @@ -353,6 +356,7 @@ static NetSocketState *net_socket_fd_init(VLANState *vlan, >> (socklen_t *)&optlen)< 0) { >> fprintf(stderr, "qemu: error: getsockopt(SO_TYPE) for fd=%d failed\n", >> fd); >> + closesocket(fd); >> return NULL; >> } >> switch(so_type) { >> @@ -386,9 +390,7 @@ static void net_socket_accept(void *opaque) >> } >> } >> s1 = net_socket_fd_init(s->vlan, s->model, s->name, fd, 1); >> - if (!s1) { >> - closesocket(fd); >> - } else { > Why is it not handled when s1 is NULL? The point of the patch is to introduce consistent error behavior - net_socket_fd_init() will close the socket on error so we no longer have to do that. If you look at net_socket_accept() there is nothing else to do on failure it was possible to just remove the if (!s1) check. Stefan
On Thu, Dec 8, 2011 at 8:57 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote: > On Thu, Dec 8, 2011 at 12:29 PM, Zhi Yong Wu <zwu.kernel@gmail.com> wrote: >> On Wed, Dec 7, 2011 at 11:01 PM, Stefan Hajnoczi >> <stefanha@linux.vnet.ibm.com> wrote: >>> Today net/socket.c has no consistent policy for closing the socket file >>> descriptor when initialization fails. This means we leak the file >>> descriptor in some cases or we could also try to close it twice. >>> >>> Make error paths consistent by taking ownership of the file descriptor >>> and closing it on error. >>> >>> Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com> >>> --- >>> net/socket.c | 17 +++++++++-------- >>> 1 files changed, 9 insertions(+), 8 deletions(-) >>> >>> diff --git a/net/socket.c b/net/socket.c >>> index 613a7ef..f999c26 100644 >>> --- a/net/socket.c >>> +++ b/net/socket.c >>> @@ -266,14 +266,13 @@ static NetSocketState *net_socket_fd_init_dgram(VLANState *vlan, >>> if (saddr.sin_addr.s_addr == 0) { >>> fprintf(stderr, "qemu: error: init_dgram: fd=%d unbound, " >>> "cannot setup multicast dst addr\n", fd); >>> - return NULL; >>> + goto err; >>> } >>> /* clone dgram socket */ >>> newfd = net_socket_mcast_create(&saddr, NULL); >>> if (newfd < 0) { >>> /* error already reported by net_socket_mcast_create() */ >>> - close(fd); >>> - return NULL; >>> + goto err; >>> } >>> /* clone newfd to fd, close newfd */ >>> dup2(newfd, fd); >>> @@ -283,7 +282,7 @@ static NetSocketState *net_socket_fd_init_dgram(VLANState *vlan, >>> fprintf(stderr, >>> "qemu: error: init_dgram: fd=%d failed getsockname(): %s\n", >>> fd, strerror(errno)); >>> - return NULL; >>> + goto err; >>> } >>> } >>> >>> @@ -304,6 +303,10 @@ static NetSocketState *net_socket_fd_init_dgram(VLANState *vlan, >>> if (is_connected) s->dgram_dst=saddr; >>> >>> return s; >>> + >>> +err: >>> + closesocket(fd); >>> + return NULL; >>> } >>> >>> static void net_socket_connect(void *opaque) >>> @@ -353,6 +356,7 @@ static NetSocketState *net_socket_fd_init(VLANState *vlan, >>> (socklen_t *)&optlen)< 0) { >>> fprintf(stderr, "qemu: error: getsockopt(SO_TYPE) for fd=%d failed\n", >>> fd); >>> + closesocket(fd); >>> return NULL; >>> } >>> switch(so_type) { >>> @@ -386,9 +390,7 @@ static void net_socket_accept(void *opaque) >>> } >>> } >>> s1 = net_socket_fd_init(s->vlan, s->model, s->name, fd, 1); >>> - if (!s1) { >>> - closesocket(fd); >>> - } else { >> Why is it not handled when s1 is NULL? > > The point of the patch is to introduce consistent error behavior - > net_socket_fd_init() will close the socket on error so we no longer > have to do that. If you look at net_socket_accept() there is nothing > else to do on failure it was possible to just remove the if (!s1) > check. You are right, thanks. > > Stefan
diff --git a/net/socket.c b/net/socket.c index 613a7ef..f999c26 100644 --- a/net/socket.c +++ b/net/socket.c @@ -266,14 +266,13 @@ static NetSocketState *net_socket_fd_init_dgram(VLANState *vlan, if (saddr.sin_addr.s_addr == 0) { fprintf(stderr, "qemu: error: init_dgram: fd=%d unbound, " "cannot setup multicast dst addr\n", fd); - return NULL; + goto err; } /* clone dgram socket */ newfd = net_socket_mcast_create(&saddr, NULL); if (newfd < 0) { /* error already reported by net_socket_mcast_create() */ - close(fd); - return NULL; + goto err; } /* clone newfd to fd, close newfd */ dup2(newfd, fd); @@ -283,7 +282,7 @@ static NetSocketState *net_socket_fd_init_dgram(VLANState *vlan, fprintf(stderr, "qemu: error: init_dgram: fd=%d failed getsockname(): %s\n", fd, strerror(errno)); - return NULL; + goto err; } } @@ -304,6 +303,10 @@ static NetSocketState *net_socket_fd_init_dgram(VLANState *vlan, if (is_connected) s->dgram_dst=saddr; return s; + +err: + closesocket(fd); + return NULL; } static void net_socket_connect(void *opaque) @@ -353,6 +356,7 @@ static NetSocketState *net_socket_fd_init(VLANState *vlan, (socklen_t *)&optlen)< 0) { fprintf(stderr, "qemu: error: getsockopt(SO_TYPE) for fd=%d failed\n", fd); + closesocket(fd); return NULL; } switch(so_type) { @@ -386,9 +390,7 @@ static void net_socket_accept(void *opaque) } } s1 = net_socket_fd_init(s->vlan, s->model, s->name, fd, 1); - if (!s1) { - closesocket(fd); - } else { + if (s1) { snprintf(s1->nc.info_str, sizeof(s1->nc.info_str), "socket: connection from %s:%d", inet_ntoa(saddr.sin_addr), ntohs(saddr.sin_port)); @@ -549,7 +551,6 @@ int net_init_socket(QemuOpts *opts, } if (!net_socket_fd_init(vlan, "socket", name, fd, 1)) { - close(fd); return -1; } } else if (qemu_opt_get(opts, "listen")) {
Today net/socket.c has no consistent policy for closing the socket file descriptor when initialization fails. This means we leak the file descriptor in some cases or we could also try to close it twice. Make error paths consistent by taking ownership of the file descriptor and closing it on error. Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com> --- net/socket.c | 17 +++++++++-------- 1 files changed, 9 insertions(+), 8 deletions(-)