From 403a905b03d48187df1e1392bfa3e303eeefe892 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Philippe=20Mathieu-Daud=C3=A9?= Date: Wed, 9 Aug 2017 16:32:46 +0200 Subject: [PATCH 1/4] 9pfs: avoid sign conversion error simplifying the code MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit (note this is how other functions also handle the errors). hw/9pfs/9p.c:948:18: warning: Loss of sign in implicit conversion offset = err; ^~~ Reported-by: Clang Static Analyzer Signed-off-by: Philippe Mathieu-Daudé Signed-off-by: Greg Kurz --- hw/9pfs/9p.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c index 333dbb6f8e..0a37c8bd13 100644 --- a/hw/9pfs/9p.c +++ b/hw/9pfs/9p.c @@ -945,7 +945,6 @@ static void coroutine_fn v9fs_version(void *opaque) v9fs_string_init(&version); err = pdu_unmarshal(pdu, offset, "ds", &s->msize, &version); if (err < 0) { - offset = err; goto out; } trace_v9fs_version(pdu->tag, pdu->id, s->msize, version.data); @@ -962,13 +961,12 @@ static void coroutine_fn v9fs_version(void *opaque) err = pdu_marshal(pdu, offset, "ds", s->msize, &version); if (err < 0) { - offset = err; goto out; } - offset += err; + err += offset; trace_v9fs_version_return(pdu->tag, pdu->id, s->msize, version.data); out: - pdu_complete(pdu, offset); + pdu_complete(pdu, err); v9fs_string_free(&version); } From 3c08f4a4335c325295d738d4090665b8617ab599 Mon Sep 17 00:00:00 2001 From: ZhiPeng Lu Date: Wed, 9 Aug 2017 16:32:46 +0200 Subject: [PATCH 2/4] fsdev: fix memory leak in main() @rpath and @sock_name are not freed and leaked. [groug, not really leaked since the program exits just after that. But it is always good practice to free allocated memory] Signed-off-by: Zhipeng Lu Signed-off-by: Greg Kurz --- fsdev/virtfs-proxy-helper.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/fsdev/virtfs-proxy-helper.c b/fsdev/virtfs-proxy-helper.c index 6c066ec9a0..8e48500dd5 100644 --- a/fsdev/virtfs-proxy-helper.c +++ b/fsdev/virtfs-proxy-helper.c @@ -1162,6 +1162,8 @@ int main(int argc, char **argv) process_requests(sock); error: + g_free(rpath); + g_free(sock_name); do_log(LOG_INFO, "Done\n"); closelog(); return 0; From aa5e85a10846636165592c5a46d797c100c68529 Mon Sep 17 00:00:00 2001 From: Greg Kurz Date: Mon, 4 Sep 2017 09:24:53 +0200 Subject: [PATCH 3/4] 9pfs: local: clarify fchmodat_nofollow() implementation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Since fchmodat(2) on Linux doesn't support AT_SYMLINK_NOFOLLOW, we have to implement it using workarounds. There are two different ways, depending on whether the system supports O_PATH or not. In the case O_PATH is supported, we rely on the behavhior of openat(2) when passing O_NOFOLLOW | O_PATH and the file is a symbolic link. Even if openat_file() already adds O_NOFOLLOW to the flags, this patch makes it explicit that we need both creation flags to obtain the expected behavior. This is only cleanup, no functional change. Signed-off-by: Greg Kurz Reviewed-by: Philippe Mathieu-Daudé --- hw/9pfs/9p-local.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/hw/9pfs/9p-local.c b/hw/9pfs/9p-local.c index efb0b79a74..e51af87309 100644 --- a/hw/9pfs/9p-local.c +++ b/hw/9pfs/9p-local.c @@ -349,11 +349,11 @@ static int fchmodat_nofollow(int dirfd, const char *name, mode_t mode) return -1; } - /* Access modes are ignored when O_PATH is supported. We try O_RDONLY and - * O_WRONLY for old-systems that don't support O_PATH. - */ - fd = openat_file(dirfd, name, O_RDONLY | O_PATH_9P_UTIL, 0); + fd = openat_file(dirfd, name, O_RDONLY | O_PATH_9P_UTIL | O_NOFOLLOW, 0); #if O_PATH_9P_UTIL == 0 + /* Fallback for systems that don't support O_PATH: we depend on the file + * being readable or writable. + */ if (fd == -1) { /* In case the file is writable-only and isn't a directory. */ if (errno == EACCES) { @@ -368,6 +368,10 @@ static int fchmodat_nofollow(int dirfd, const char *name, mode_t mode) } ret = fchmod(fd, mode); #else + /* Access modes are ignored when O_PATH is supported. If name is a symbolic + * link, O_PATH | O_NOFOLLOW causes openat(2) to return a file descriptor + * referring to the symbolic link. + */ if (fd == -1) { return -1; } From 32b6943699948f7adc35ada233fbd25daffad5e9 Mon Sep 17 00:00:00 2001 From: Greg Kurz Date: Mon, 4 Sep 2017 09:59:01 +0200 Subject: [PATCH 4/4] virtfs: error out gracefully when mandatory suboptions are missing We internally convert -virtfs to -fsdev/-device. If the user doesn't provide the path or security_model suboptions, and the fsdev backend requires them, we hit an assertion when populating the internal -fsdev option: util/qemu-option.c:547: opt_set: Assertion `opt->str' failed. Aborted (core dumped) Let's test the suboption presence on the command line before trying to set it in the internal -fsdev option, and let the backend code error out gracefully (ie, like it already does when the user passes -fsdev on the command line). Reported-by: Thomas Huth Signed-off-by: Greg Kurz Reviewed-by: Thomas Huth --- vl.c | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/vl.c b/vl.c index 0b45e1b6fa..e75757f977 100644 --- a/vl.c +++ b/vl.c @@ -3557,7 +3557,7 @@ int main(int argc, char **argv, char **envp) case QEMU_OPTION_virtfs: { QemuOpts *fsdev; QemuOpts *device; - const char *writeout, *sock_fd, *socket; + const char *writeout, *sock_fd, *socket, *path, *security_model; olist = qemu_find_opts("virtfs"); if (!olist) { @@ -3596,11 +3596,15 @@ int main(int argc, char **argv, char **envp) } qemu_opt_set(fsdev, "fsdriver", qemu_opt_get(opts, "fsdriver"), &error_abort); - qemu_opt_set(fsdev, "path", qemu_opt_get(opts, "path"), - &error_abort); - qemu_opt_set(fsdev, "security_model", - qemu_opt_get(opts, "security_model"), - &error_abort); + path = qemu_opt_get(opts, "path"); + if (path) { + qemu_opt_set(fsdev, "path", path, &error_abort); + } + security_model = qemu_opt_get(opts, "security_model"); + if (security_model) { + qemu_opt_set(fsdev, "security_model", security_model, + &error_abort); + } socket = qemu_opt_get(opts, "socket"); if (socket) { qemu_opt_set(fsdev, "socket", socket, &error_abort);