From 6ffacc5d3ddf2e3227aae2a8cc5c15627265f727 Mon Sep 17 00:00:00 2001 From: Luiz Capitulino Date: Tue, 8 Jan 2013 19:26:25 -0200 Subject: [PATCH 1/8] qemu-ga: ga_open_pidfile(): use qemu_open() This ensures that O_CLOEXEC is passed to open(), this way the pid file fd is not leaked to executed processes. Signed-off-by: Luiz Capitulino Reviewed-by: Eric Blake Acked-by: Amos Kong Tested-by: Amos Kong Signed-off-by: Michael Roth --- qga/main.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/qga/main.c b/qga/main.c index a9b968c507..4239150d4e 100644 --- a/qga/main.c +++ b/qga/main.c @@ -267,7 +267,7 @@ static bool ga_open_pidfile(const char *pidfile) int pidfd; char pidstr[32]; - pidfd = open(pidfile, O_CREAT|O_WRONLY, S_IRUSR|S_IWUSR); + pidfd = qemu_open(pidfile, O_CREAT|O_WRONLY, S_IRUSR|S_IWUSR); if (pidfd == -1 || lockf(pidfd, F_TLOCK, 0)) { g_critical("Cannot lock pid file, %s", strerror(errno)); if (pidfd != -1) { From 9e92f6d46233171898fc7d0487a04e5b78e44234 Mon Sep 17 00:00:00 2001 From: Luiz Capitulino Date: Tue, 8 Jan 2013 19:26:26 -0200 Subject: [PATCH 2/8] qemu-ga: add ga_open_logfile() This function sets O_CLOEXEC on the log file fd so that it isn't leaked to executed processes. Signed-off-by: Luiz Capitulino Reviewed-by: Eric Blake Acked-by: Amos Kong Tested-by: Amos Kong Signed-off-by: Michael Roth --- qga/main.c | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/qga/main.c b/qga/main.c index 4239150d4e..bd70ead126 100644 --- a/qga/main.c +++ b/qga/main.c @@ -261,6 +261,19 @@ void ga_set_response_delimited(GAState *s) s->delimit_response = true; } +static FILE *ga_open_logfile(const char *logfile) +{ + FILE *f; + + f = fopen(logfile, "a"); + if (!f) { + return NULL; + } + + qemu_set_cloexec(fileno(f)); + return f; +} + #ifndef _WIN32 static bool ga_open_pidfile(const char *pidfile) { @@ -402,7 +415,7 @@ void ga_unset_frozen(GAState *s) * in a frozen state at start up, do it now */ if (s->deferred_options.log_filepath) { - s->log_file = fopen(s->deferred_options.log_filepath, "a"); + s->log_file = ga_open_logfile(s->deferred_options.log_filepath); if (!s->log_file) { s->log_file = stderr; } @@ -884,7 +897,7 @@ int main(int argc, char **argv) become_daemon(pid_filepath); } if (log_filepath) { - FILE *log_file = fopen(log_filepath, "a"); + FILE *log_file = ga_open_logfile(log_filepath); if (!log_file) { g_critical("unable to open specified log file: %s", strerror(errno)); From f5b795787864ddde1104a4f7c061dcb0e58e45c0 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Fri, 11 Jan 2013 11:24:57 +0100 Subject: [PATCH 3/8] qemu-ga: Document intentional fall through in channel_event_cb() For clarity, and to hush up Coverity. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Reviewed-by: Michael Roth Reviewed-by: Luiz Capitulino Signed-off-by: Michael Roth --- qga/main.c | 1 + 1 file changed, 1 insertion(+) diff --git a/qga/main.c b/qga/main.c index bd70ead126..e8a9a9ee7e 100644 --- a/qga/main.c +++ b/qga/main.c @@ -618,6 +618,7 @@ static gboolean channel_event_cb(GIOCondition condition, gpointer data) if (!s->virtio) { return false; } + /* fall through */ case G_IO_STATUS_AGAIN: /* virtio causes us to spin here when no process is attached to * host-side chardev. sleep a bit to mitigate this From 5d27f9ce3de424207883d84352d76150e9707394 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Fri, 11 Jan 2013 11:24:58 +0100 Subject: [PATCH 4/8] qemu-ga: Drop pointless lseek() from ga_open_pidfile() After open(), the file offset is already zero, and neither lockf() nor ftruncate() change it. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Reviewed-by: Michael Roth Reviewed-by: Luiz Capitulino Signed-off-by: Michael Roth --- qga/main.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/qga/main.c b/qga/main.c index e8a9a9ee7e..96d3cfa381 100644 --- a/qga/main.c +++ b/qga/main.c @@ -289,7 +289,7 @@ static bool ga_open_pidfile(const char *pidfile) return false; } - if (ftruncate(pidfd, 0) || lseek(pidfd, 0, SEEK_SET)) { + if (ftruncate(pidfd, 0)) { g_critical("Failed to truncate pid file"); goto fail; } From 03ac10f166b790cb66804e512abec6d002cd8481 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Fri, 11 Jan 2013 11:24:59 +0100 Subject: [PATCH 5/8] qemu-ga: Plug file descriptor leak on ga_open_pidfile() error path Spotted by Coverity. Also document why we keep it open on success. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Reviewed-by: Michael Roth Reviewed-by: Luiz Capitulino Signed-off-by: Michael Roth --- qga/main.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/qga/main.c b/qga/main.c index 96d3cfa381..db281a508b 100644 --- a/qga/main.c +++ b/qga/main.c @@ -299,10 +299,12 @@ static bool ga_open_pidfile(const char *pidfile) goto fail; } + /* keep pidfile open & locked forever */ return true; fail: unlink(pidfile); + close(pidfd); return false; } #else /* _WIN32 */ From 32c16620dda8ba16f6d6bcd20efefdec8975af77 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Fri, 11 Jan 2013 11:25:00 +0100 Subject: [PATCH 6/8] qemu-ga: Plug fd leak on ga_channel_listen_accept() error path Spotted by Coverity. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Reviewed-by: Michael Roth Reviewed-by: Luiz Capitulino Signed-off-by: Michael Roth --- qga/channel-posix.c | 1 + 1 file changed, 1 insertion(+) diff --git a/qga/channel-posix.c b/qga/channel-posix.c index ca9e4aaaf9..9a5c05d666 100644 --- a/qga/channel-posix.c +++ b/qga/channel-posix.c @@ -46,6 +46,7 @@ static gboolean ga_channel_listen_accept(GIOChannel *channel, ret = ga_channel_client_add(c, client_fd); if (ret) { g_warning("error setting up connection"); + close(client_fd); goto out; } accepted = true; From d4f4a3efdf0a71621ae5351176f5f15b522d0026 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Fri, 11 Jan 2013 11:25:01 +0100 Subject: [PATCH 7/8] qemu-ga: Plug fd leak on ga_channel_open() error paths Spotted by Coverity. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Reviewed-by: Michael Roth Reviewed-by: Luiz Capitulino Signed-off-by: Michael Roth --- qga/channel-posix.c | 1 + 1 file changed, 1 insertion(+) diff --git a/qga/channel-posix.c b/qga/channel-posix.c index 9a5c05d666..05e83860ca 100644 --- a/qga/channel-posix.c +++ b/qga/channel-posix.c @@ -154,6 +154,7 @@ static gboolean ga_channel_open(GAChannel *c, const gchar *path, GAChannelMethod ret = ga_channel_client_add(c, fd); if (ret) { g_critical("error adding channel to main loop"); + close(fd); return false; } break; From 7868181f98ff1fbcd7f7034153eec5e03615d023 Mon Sep 17 00:00:00 2001 From: Markus Armbruster Date: Fri, 11 Jan 2013 11:25:02 +0100 Subject: [PATCH 8/8] qemu-ga: Handle errors uniformely in ga_channel_open() We detect errors in several places. One reports with g_error(), which calls abort(), the others report with g_critical(). Three of them exit(), three return false. Always report with g_critical(), and return false. Signed-off-by: Markus Armbruster Reviewed-by: Eric Blake Reviewed-by: Michael Roth Reviewed-by: Luiz Capitulino *minor fix-up of commit msg Signed-off-by: Michael Roth --- qga/channel-posix.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/qga/channel-posix.c b/qga/channel-posix.c index 05e83860ca..e65dda3822 100644 --- a/qga/channel-posix.c +++ b/qga/channel-posix.c @@ -141,14 +141,15 @@ static gboolean ga_channel_open(GAChannel *c, const gchar *path, GAChannelMethod ); if (fd == -1) { g_critical("error opening channel: %s", strerror(errno)); - exit(EXIT_FAILURE); + return false; } #ifdef CONFIG_SOLARIS ret = ioctl(fd, I_SETSIG, S_OUTPUT | S_INPUT | S_HIPRI); if (ret == -1) { g_critical("error setting event mask for channel: %s", strerror(errno)); - exit(EXIT_FAILURE); + close(fd); + return false; } #endif ret = ga_channel_client_add(c, fd); @@ -164,7 +165,7 @@ static gboolean ga_channel_open(GAChannel *c, const gchar *path, GAChannelMethod int fd = qemu_open(path, O_RDWR | O_NOCTTY | O_NONBLOCK); if (fd == -1) { g_critical("error opening channel: %s", strerror(errno)); - exit(EXIT_FAILURE); + return false; } tcgetattr(fd, &tio); /* set up serial port for non-canonical, dumb byte streaming */ @@ -184,7 +185,9 @@ static gboolean ga_channel_open(GAChannel *c, const gchar *path, GAChannelMethod tcsetattr(fd, TCSANOW, &tio); ret = ga_channel_client_add(c, fd); if (ret) { - g_error("error adding channel to main loop"); + g_critical("error adding channel to main loop"); + close(fd); + return false; } break; }