[libvirt] [PATCH] Remove & ban use of select() for waiting for I/O

From: "Daniel P. Berrange" <berrange@redhat.com> Use of the select() system call is inherantly dangerous since applications will hit a buffer overrun if any FD number exceeds the size of the select set size (typically 1024). Replace the two uses of select() with poll() and use cfg.mk to ban any future use of select(). NB: This changes the phyp driver so that it uses an infinite timeout, instead of busy-waiting for 1ms at a time. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- cfg.mk | 8 ++++++++ src/phyp/phyp_driver.c | 20 +++++++------------- src/util/virnetlink.c | 16 +++++++--------- 3 files changed, 22 insertions(+), 22 deletions(-) diff --git a/cfg.mk b/cfg.mk index 0bf5bfc..0e809fc 100644 --- a/cfg.mk +++ b/cfg.mk @@ -444,6 +444,14 @@ sc_prohibit_nonreentrant: done ; \ exit $$fail +sc_prohibit_select: + @fail=0 ; \ + (prohibit="\\<select *\\(" \ + halt="use poll(), not select()" \ + $(_sc_search_regexp) \ + ) || fail=1; \ + exit $$fail + # Prohibit the inclusion of <ctype.h>. sc_prohibit_ctype_h: @prohibit='^# *include *<ctype\.h>' \ diff --git a/src/phyp/phyp_driver.c b/src/phyp/phyp_driver.c index f0007c0..4594cbf 100644 --- a/src/phyp/phyp_driver.c +++ b/src/phyp/phyp_driver.c @@ -40,6 +40,7 @@ #include <netdb.h> #include <fcntl.h> #include <domain_event.h> +#include <poll.h> #include "internal.h" #include "virauth.h" @@ -72,29 +73,22 @@ static unsigned const int PHYP_MAC_SIZE= 12; static int waitsocket(int socket_fd, LIBSSH2_SESSION * session) { - struct timeval timeout; - fd_set fd; - fd_set *writefd = NULL; - fd_set *readfd = NULL; + struct pollfd fds[1]; int dir; - timeout.tv_sec = 0; - timeout.tv_usec = 1000; - - FD_ZERO(&fd); - - FD_SET(socket_fd, &fd); + memset(fds, 0, sizeof(fds)); + fds[0].fd = socket_fd; /* now make sure we wait in the correct direction */ dir = libssh2_session_block_directions(session); if (dir & LIBSSH2_SESSION_BLOCK_INBOUND) - readfd = &fd; + fds[0].events |= POLLIN; if (dir & LIBSSH2_SESSION_BLOCK_OUTBOUND) - writefd = &fd; + fds[0].events |= POLLOUT; - return select(socket_fd + 1, readfd, writefd, NULL, &timeout); + return poll(fds, ARRAY_CARDINALITY(fds), -1); } /* this function is the layer that manipulates the ssh channel itself diff --git a/src/util/virnetlink.c b/src/util/virnetlink.c index d340eda..f5865a0 100644 --- a/src/util/virnetlink.c +++ b/src/util/virnetlink.c @@ -48,7 +48,7 @@ #define VIR_FROM_THIS VIR_FROM_NET -#define NETLINK_ACK_TIMEOUT_S 2 +#define NETLINK_ACK_TIMEOUT_S 2*1000 #if defined(__linux__) && defined(HAVE_LIBNL) /* State for a single netlink event handle */ @@ -185,10 +185,7 @@ int virNetlinkCommand(struct nl_msg *nl_msg, .nl_groups = 0, }; ssize_t nbytes; - struct timeval tv = { - .tv_sec = NETLINK_ACK_TIMEOUT_S, - }; - fd_set readfds; + struct pollfd fds[1]; int fd; int n; struct nlmsghdr *nlmsg = nlmsg_hdr(nl_msg); @@ -242,14 +239,15 @@ int virNetlinkCommand(struct nl_msg *nl_msg, goto error; } - FD_ZERO(&readfds); - FD_SET(fd, &readfds); + memset(fds, 0, sizeof(fds)); + fds[0].fd = fd; + fds[0].events = POLLIN; - n = select(fd + 1, &readfds, NULL, NULL, &tv); + n = poll(fds, ARRAY_CARDINALITY(fds), NETLINK_ACK_TIMEOUT_S); if (n <= 0) { if (n < 0) virReportSystemError(errno, "%s", - _("error in select call")); + _("error in poll call")); if (n == 0) virReportSystemError(ETIMEDOUT, "%s", _("no valid netlink response was received")); -- 1.8.2.1

On 05/13/2013 07:45 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
Use of the select() system call is inherantly dangerous since applications will hit a buffer overrun if any FD number exceeds the size of the select set size (typically 1024). Replace the two uses of select() with poll() and use cfg.mk to ban any future use of select().
NB: This changes the phyp driver so that it uses an infinite timeout, instead of busy-waiting for 1ms at a time.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> ---
+++ b/cfg.mk @@ -444,6 +444,14 @@ sc_prohibit_nonreentrant: done ; \ exit $$fail
+sc_prohibit_select: + @fail=0 ; \ + (prohibit="\\<select *\\(" \ + halt="use poll(), not select()" \ + $(_sc_search_regexp) \ + ) || fail=1; \ + exit $$fail
Overkill. $(_sc_search_regexp) exits with non-zero status, so it is sufficient to write: +sc_prohibit_select: + @prohibit="\\<select *\\(" \ + halt="use poll(), not select()" \ + $(_sc_search_regexp)
+++ b/src/util/virnetlink.c @@ -48,7 +48,7 @@
#define VIR_FROM_THIS VIR_FROM_NET
-#define NETLINK_ACK_TIMEOUT_S 2 +#define NETLINK_ACK_TIMEOUT_S 2*1000
Needs parenthesis. ACK with those fixes. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (2)
-
Daniel P. Berrange
-
Eric Blake