From cc75a50c68ccea2068e76fc59e5492899abd3bdb Mon Sep 17 00:00:00 2001 From: "Daniel P. Berrange" Date: Mon, 11 Jan 2016 12:59:44 +0000 Subject: [PATCH 1/5] io: fix sign of errno value passed to error report When reporting the number of FDs has been exceeded, pass EINVAL to error_setg_errno, rather than -EINVAL. Signed-off-by: Daniel P. Berrange --- io/channel-socket.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/io/channel-socket.c b/io/channel-socket.c index 10a5b3136e..eaa411f3d2 100644 --- a/io/channel-socket.c +++ b/io/channel-socket.c @@ -502,7 +502,7 @@ static ssize_t qio_channel_socket_writev(QIOChannel *ioc, if (nfds) { if (nfds > SOCKET_MAX_FDS) { - error_setg_errno(errp, -EINVAL, + error_setg_errno(errp, EINVAL, "Only %d FDs can be sent, got %zu", SOCKET_MAX_FDS, nfds); return -1; From 0c0a55b229bb6d61408394766bcbb04beecd5fa5 Mon Sep 17 00:00:00 2001 From: "Daniel P. Berrange" Date: Mon, 11 Jan 2016 13:00:36 +0000 Subject: [PATCH 2/5] io: increment counter when killing off subcommand When killing the subcommand, it is intended to first send SIGTERM, then SIGKILL and only report an error if it still doesn't die after SIGKILL. The 'step' counter was not being incremented though, so the code never got past the SIGTERM stage. Signed-off-by: Daniel P. Berrange --- io/channel-command.c | 1 + 1 file changed, 1 insertion(+) diff --git a/io/channel-command.c b/io/channel-command.c index 598fdab5a3..a220fe886b 100644 --- a/io/channel-command.c +++ b/io/channel-command.c @@ -179,6 +179,7 @@ static int qio_channel_command_abort(QIOChannelCommand *ioc, (unsigned long long)ioc->pid); return -1; } + step++; usleep(10 * 1000); goto rewait; } From e155494cf0b876c45c3c68a9ab6c641aac22dfdf Mon Sep 17 00:00:00 2001 From: "Daniel P. Berrange" Date: Mon, 11 Jan 2016 13:02:16 +0000 Subject: [PATCH 3/5] io: some fixes to handling of /dev/null when running commands The /dev/null file handle was leaked in a couple of places. There is also the possibility that both readfd and writefd point to the same /dev/null file handle, so care must be taken not to close the same file handle twice. Reviewed-by: Paolo Bonzini Signed-off-by: Daniel P. Berrange --- io/channel-command.c | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/io/channel-command.c b/io/channel-command.c index a220fe886b..a9c67aa221 100644 --- a/io/channel-command.c +++ b/io/channel-command.c @@ -66,7 +66,7 @@ qio_channel_command_new_spawn(const char *const argv[], if (stdinnull || stdoutnull) { devnull = open("/dev/null", O_RDWR); - if (!devnull) { + if (devnull < 0) { error_setg_errno(errp, errno, "Unable to open /dev/null"); goto error; @@ -98,6 +98,9 @@ qio_channel_command_new_spawn(const char *const argv[], close(stdoutfd[0]); close(stdoutfd[1]); } + if (devnull != -1) { + close(devnull); + } execv(argv[0], (char * const *)argv); _exit(1); @@ -117,6 +120,9 @@ qio_channel_command_new_spawn(const char *const argv[], return ioc; error: + if (devnull != -1) { + close(devnull); + } if (stdinfd[0] != -1) { close(stdinfd[0]); } @@ -202,12 +208,12 @@ static void qio_channel_command_finalize(Object *obj) QIOChannelCommand *ioc = QIO_CHANNEL_COMMAND(obj); if (ioc->readfd != -1) { close(ioc->readfd); - ioc->readfd = -1; } - if (ioc->writefd != -1) { + if (ioc->writefd != -1 && + ioc->writefd != ioc->readfd) { close(ioc->writefd); - ioc->writefd = -1; } + ioc->writefd = ioc->readfd = -1; if (ioc->pid > 0) { #ifndef WIN32 qio_channel_command_abort(ioc, NULL); @@ -299,12 +305,16 @@ static int qio_channel_command_close(QIOChannel *ioc, /* We close FDs before killing, because that * gives a better chance of clean shutdown */ - if (close(cioc->writefd) < 0) { + if (cioc->readfd != -1 && + close(cioc->readfd) < 0) { rv = -1; } - if (close(cioc->readfd) < 0) { + if (cioc->writefd != -1 && + cioc->writefd != cioc->readfd && + close(cioc->writefd) < 0) { rv = -1; } + cioc->writefd = cioc->readfd = -1; #ifndef WIN32 if (qio_channel_command_abort(cioc, errp) < 0) { return -1; From 821791b5055788937ddfa67a183c0ff54129efe4 Mon Sep 17 00:00:00 2001 From: "Daniel P. Berrange" Date: Wed, 13 Jan 2016 12:22:33 +0000 Subject: [PATCH 4/5] io: fix description of @errp parameter initialization The "Error **errp" parameters must be NULL initialized not uninitialized. Signed-off-by: Daniel P. Berrange --- include/io/channel-command.h | 4 ++-- include/io/channel-socket.h | 14 +++++++------- include/io/channel-tls.h | 4 ++-- include/io/channel.h | 20 ++++++++++---------- 4 files changed, 21 insertions(+), 21 deletions(-) diff --git a/include/io/channel-command.h b/include/io/channel-command.h index bd3c59946f..cfc177e786 100644 --- a/include/io/channel-command.h +++ b/include/io/channel-command.h @@ -51,7 +51,7 @@ struct QIOChannelCommand { * @writefd: the FD connected to the command's stdin * @readfd: the FD connected to the command's stdout * @pid: the PID of the running child command - * @errp: pointer to an uninitialized error object + * @errp: pointer to a NULL-initialized error object * * Create a channel for performing I/O with the * previously spawned command identified by @pid. @@ -75,7 +75,7 @@ qio_channel_command_new_pid(int writefd, * qio_channel_command_new_spawn: * @argv: the NULL terminated list of command arguments * @flags: the I/O mode, one of O_RDONLY, O_WRONLY, O_RDWR - * @errp: pointer to an uninitialized error object + * @errp: pointer to a NULL-initialized error object * * Create a channel for performing I/O with the * command to be spawned with arguments @argv. diff --git a/include/io/channel-socket.h b/include/io/channel-socket.h index 0719757b0f..810537ef6f 100644 --- a/include/io/channel-socket.h +++ b/include/io/channel-socket.h @@ -67,7 +67,7 @@ qio_channel_socket_new(void); /** * qio_channel_socket_new_fd: * @fd: the socket file descriptor - * @errp: pointer to an uninitialized error object + * @errp: pointer to a NULL-initialized error object * * Create a channel for performing I/O on the socket * connection represented by the file descriptor @fd. @@ -83,7 +83,7 @@ qio_channel_socket_new_fd(int fd, * qio_channel_socket_connect_sync: * @ioc: the socket channel object * @addr: the address to connect to - * @errp: pointer to an uninitialized error object + * @errp: pointer to a NULL-initialized error object * * Attempt to connect to the address @addr. This method * will run in the foreground so the caller will not regain @@ -118,7 +118,7 @@ void qio_channel_socket_connect_async(QIOChannelSocket *ioc, * qio_channel_socket_listen_sync: * @ioc: the socket channel object * @addr: the address to listen to - * @errp: pointer to an uninitialized error object + * @errp: pointer to a NULL-initialized error object * * Attempt to listen to the address @addr. This method * will run in the foreground so the caller will not regain @@ -154,7 +154,7 @@ void qio_channel_socket_listen_async(QIOChannelSocket *ioc, * @ioc: the socket channel object * @localAddr: the address to local bind address * @remoteAddr: the address to remote peer address - * @errp: pointer to an uninitialized error object + * @errp: pointer to a NULL-initialized error object * * Attempt to initialize a datagram socket bound to * @localAddr and communicating with peer @remoteAddr. @@ -193,7 +193,7 @@ void qio_channel_socket_dgram_async(QIOChannelSocket *ioc, /** * qio_channel_socket_get_local_address: * @ioc: the socket channel object - * @errp: pointer to an uninitialized error object + * @errp: pointer to a NULL-initialized error object * * Get the string representation of the local socket * address. A pointer to the allocated address information @@ -210,7 +210,7 @@ qio_channel_socket_get_local_address(QIOChannelSocket *ioc, /** * qio_channel_socket_get_remote_address: * @ioc: the socket channel object - * @errp: pointer to an uninitialized error object + * @errp: pointer to a NULL-initialized error object * * Get the string representation of the local socket * address. A pointer to the allocated address information @@ -228,7 +228,7 @@ qio_channel_socket_get_remote_address(QIOChannelSocket *ioc, /** * qio_channel_socket_accept: * @ioc: the socket channel object - * @errp: pointer to an uninitialized error object + * @errp: pointer to a NULL-initialized error object * * If the socket represents a server, then this accepts * a new client connection. The returned channel will diff --git a/include/io/channel-tls.h b/include/io/channel-tls.h index 0298b1770e..322eccbaae 100644 --- a/include/io/channel-tls.h +++ b/include/io/channel-tls.h @@ -55,7 +55,7 @@ struct QIOChannelTLS { * @master: the underlying channel object * @creds: the credentials to use for TLS handshake * @aclname: the access control list for validating clients - * @errp: pointer to an uninitialized error object + * @errp: pointer to a NULL-initialized error object * * Create a new TLS channel that runs the server side of * a TLS session. The TLS session handshake will use the @@ -85,7 +85,7 @@ qio_channel_tls_new_server(QIOChannel *master, * @master: the underlying channel object * @creds: the credentials to use for TLS handshake * @hostname: the user specified server hostname - * @errp: pointer to an uninitialized error object + * @errp: pointer to a NULL-initialized error object * * Create a new TLS channel that runs the client side of * a TLS session. The TLS session handshake will use the diff --git a/include/io/channel.h b/include/io/channel.h index 4ecec98bf4..3e17fe7129 100644 --- a/include/io/channel.h +++ b/include/io/channel.h @@ -152,7 +152,7 @@ bool qio_channel_has_feature(QIOChannel *ioc, * @niov: the length of the @iov array * @fds: pointer to an array that will received file handles * @nfds: pointer filled with number of elements in @fds on return - * @errp: pointer to an uninitialized error object + * @errp: pointer to a NULL-initialized error object * * Read data from the IO channel, storing it in the * memory regions referenced by @iov. Each element @@ -198,7 +198,7 @@ ssize_t qio_channel_readv_full(QIOChannel *ioc, * @niov: the length of the @iov array * @fds: an array of file handles to send * @nfds: number of file handles in @fds - * @errp: pointer to an uninitialized error object + * @errp: pointer to a NULL-initialized error object * * Write data to the IO channel, reading it from the * memory regions referenced by @iov. Each element @@ -237,7 +237,7 @@ ssize_t qio_channel_writev_full(QIOChannel *ioc, * @ioc: the channel object * @iov: the array of memory regions to read data into * @niov: the length of the @iov array - * @errp: pointer to an uninitialized error object + * @errp: pointer to a NULL-initialized error object * * Behaves as qio_channel_readv_full() but does not support * receiving of file handles. @@ -252,7 +252,7 @@ ssize_t qio_channel_readv(QIOChannel *ioc, * @ioc: the channel object * @iov: the array of memory regions to write data from * @niov: the length of the @iov array - * @errp: pointer to an uninitialized error object + * @errp: pointer to a NULL-initialized error object * * Behaves as qio_channel_writev_full() but does not support * sending of file handles. @@ -267,7 +267,7 @@ ssize_t qio_channel_writev(QIOChannel *ioc, * @ioc: the channel object * @buf: the memory region to read data into * @buflen: the length of @buf - * @errp: pointer to an uninitialized error object + * @errp: pointer to a NULL-initialized error object * * Behaves as qio_channel_readv_full() but does not support * receiving of file handles, and only supports reading into @@ -283,7 +283,7 @@ ssize_t qio_channel_read(QIOChannel *ioc, * @ioc: the channel object * @buf: the memory regions to send data from * @buflen: the length of @buf - * @errp: pointer to an uninitialized error object + * @errp: pointer to a NULL-initialized error object * * Behaves as qio_channel_writev_full() but does not support * sending of file handles, and only supports writing from a @@ -298,7 +298,7 @@ ssize_t qio_channel_write(QIOChannel *ioc, * qio_channel_set_blocking: * @ioc: the channel object * @enabled: the blocking flag state - * @errp: pointer to an uninitialized error object + * @errp: pointer to a NULL-initialized error object * * If @enabled is true, then the channel is put into * blocking mode, otherwise it will be non-blocking. @@ -314,7 +314,7 @@ int qio_channel_set_blocking(QIOChannel *ioc, /** * qio_channel_close: * @ioc: the channel object - * @errp: pointer to an uninitialized error object + * @errp: pointer to a NULL-initialized error object * * Close the channel, flushing any pending I/O * @@ -327,7 +327,7 @@ int qio_channel_close(QIOChannel *ioc, * qio_channel_shutdown: * @ioc: the channel object * @how: the direction to shutdown - * @errp: pointer to an uninitialized error object + * @errp: pointer to a NULL-initialized error object * * Shutdowns transmission and/or receiving of data * without closing the underlying transport. @@ -403,7 +403,7 @@ void qio_channel_set_cork(QIOChannel *ioc, * @ioc: the channel object * @offset: the position to seek to, relative to @whence * @whence: one of the (POSIX) SEEK_* constants listed below - * @errp: pointer to an uninitialized error object + * @errp: pointer to a NULL-initialized error object * * Moves the current I/O position within the channel * @ioc, to be @offset. The value of @offset is From ccf1e2dcd6091eea1fc2341c63201aa1a6094978 Mon Sep 17 00:00:00 2001 From: "Daniel P. Berrange" Date: Mon, 18 Jan 2016 10:37:21 +0000 Subject: [PATCH 5/5] io: use memset instead of { 0 } for initializing array Some versions of GCC on OS-X complain about CMSG_SPACE not being constant size, which prevents use of { 0 } io/channel-socket.c: In function 'qio_channel_socket_writev': io/channel-socket.c:497:18: error: variable-sized object may not be initialized char control[CMSG_SPACE(sizeof(int) * SOCKET_MAX_FDS)] = { 0 }; The compiler is at fault here, but it is nicer to avoid tickling this compiler bug by using memset instead. Reviewed-By: John Arbuckle Signed-off-by: Daniel P. Berrange --- io/channel-socket.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/io/channel-socket.c b/io/channel-socket.c index eaa411f3d2..bc117b1115 100644 --- a/io/channel-socket.c +++ b/io/channel-socket.c @@ -449,6 +449,8 @@ static ssize_t qio_channel_socket_readv(QIOChannel *ioc, char control[CMSG_SPACE(sizeof(int) * SOCKET_MAX_FDS)]; int sflags = 0; + memset(control, 0, CMSG_SPACE(sizeof(int) * SOCKET_MAX_FDS)); + #ifdef MSG_CMSG_CLOEXEC sflags |= MSG_CMSG_CLOEXEC; #endif @@ -493,10 +495,12 @@ static ssize_t qio_channel_socket_writev(QIOChannel *ioc, QIOChannelSocket *sioc = QIO_CHANNEL_SOCKET(ioc); ssize_t ret; struct msghdr msg = { NULL, }; - char control[CMSG_SPACE(sizeof(int) * SOCKET_MAX_FDS)] = { 0 }; + char control[CMSG_SPACE(sizeof(int) * SOCKET_MAX_FDS)]; size_t fdsize = sizeof(int) * nfds; struct cmsghdr *cmsg; + memset(control, 0, CMSG_SPACE(sizeof(int) * SOCKET_MAX_FDS)); + msg.msg_iov = (struct iovec *)iov; msg.msg_iovlen = niov;