From 1b6f85e2cb9302a8587772d04983a3ecc0ecfe68 Mon Sep 17 00:00:00 2001 From: Michael Tokarev Date: Thu, 12 Mar 2015 09:52:30 +0300 Subject: [PATCH 1/6] 9pfs-local: simplify/optimize local_mapped_attr_path() Omit one unnecessary memory allocation for components of the path and create the resulting path directly given lengths of the components. Do not use basename(3) because there are 2 versions of this function which differs when argument ends with slash character, use strrchr() instead so we have consistent result. This also makes sure the function will do the right thing in corner cases (eg, empty pathname is given), when basename(3) return entirely another string. Signed-off-by: Michael Tokarev Signed-off-by: Aneesh Kumar K.V --- hw/9pfs/virtio-9p-local.c | 24 +++++++++++------------- 1 file changed, 11 insertions(+), 13 deletions(-) diff --git a/hw/9pfs/virtio-9p-local.c b/hw/9pfs/virtio-9p-local.c index d05c91779f..84efb31cfe 100644 --- a/hw/9pfs/virtio-9p-local.c +++ b/hw/9pfs/virtio-9p-local.c @@ -45,19 +45,17 @@ static char *local_mapped_attr_path(FsContext *ctx, const char *path) { - char *dir_name; - char *tmp_path = g_strdup(path); - char *base_name = basename(tmp_path); - char *buffer; - - /* NULL terminate the directory */ - dir_name = tmp_path; - *(base_name - 1) = '\0'; - - buffer = g_strdup_printf("%s/%s/%s/%s", - ctx->fs_root, dir_name, VIRTFS_META_DIR, base_name); - g_free(tmp_path); - return buffer; + int dirlen; + const char *name = strrchr(path, '/'); + if (name) { + dirlen = name - path; + ++name; + } else { + name = path; + dirlen = 0; + } + return g_strdup_printf("%s/%.*s/%s/%s", ctx->fs_root, + dirlen, path, VIRTFS_META_DIR, name); } static FILE *local_fopen(const char *path, const char *mode) From 7752efcacf41daf34f9fefc5e78131f2afa3d4d7 Mon Sep 17 00:00:00 2001 From: Michael Tokarev Date: Thu, 12 Mar 2015 09:36:12 +0300 Subject: [PATCH 2/6] 9pfs-proxy: tiny cleanups in proxy_pwritev and proxy_preadv Don't compare syscall return with -1, use "<0" condition. Don't introduce useless local variables when we already have similar variable Rename local variable to be consistent with other usages Finally make the two methods, read and write, to be similar to each other Signed-off-by: Michael Tokarev Signed-off-by: Aneesh Kumar K.V --- hw/9pfs/virtio-9p-proxy.c | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/hw/9pfs/virtio-9p-proxy.c b/hw/9pfs/virtio-9p-proxy.c index 59c7445dea..6bb191ee6a 100644 --- a/hw/9pfs/virtio-9p-proxy.c +++ b/hw/9pfs/virtio-9p-proxy.c @@ -693,16 +693,16 @@ static ssize_t proxy_preadv(FsContext *ctx, V9fsFidOpenState *fs, const struct iovec *iov, int iovcnt, off_t offset) { + ssize_t ret; #ifdef CONFIG_PREADV - return preadv(fs->fd, iov, iovcnt, offset); + ret = preadv(fs->fd, iov, iovcnt, offset); #else - int err = lseek(fs->fd, offset, SEEK_SET); - if (err == -1) { - return err; - } else { - return readv(fs->fd, iov, iovcnt); + ret = lseek(fs->fd, offset, SEEK_SET); + if (ret >= 0) { + ret = readv(fs->fd, iov, iovcnt); } #endif + return ret; } static ssize_t proxy_pwritev(FsContext *ctx, V9fsFidOpenState *fs, @@ -714,10 +714,8 @@ static ssize_t proxy_pwritev(FsContext *ctx, V9fsFidOpenState *fs, #ifdef CONFIG_PREADV ret = pwritev(fs->fd, iov, iovcnt, offset); #else - int err = lseek(fs->fd, offset, SEEK_SET); - if (err == -1) { - return err; - } else { + ret = lseek(fs->fd, offset, SEEK_SET); + if (ret >= 0) { ret = writev(fs->fd, iov, iovcnt); } #endif From 9005c3b3efb7eb1b140d2ad0385efff6a3af59c4 Mon Sep 17 00:00:00 2001 From: Shannon Zhao Date: Fri, 13 Mar 2015 13:48:07 +0800 Subject: [PATCH 3/6] hw/9pfs/virtio-9p-posix-acl: Fix out-of-bounds access It's detected by coverity. Fix out-of-bounds access of the function mp_dacl_listxattr. Signed-off-by: Shannon Zhao Signed-off-by: Shannon Zhao Signed-off-by: Aneesh Kumar K.V --- hw/9pfs/virtio-9p-posix-acl.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hw/9pfs/virtio-9p-posix-acl.c b/hw/9pfs/virtio-9p-posix-acl.c index 803d9d94f3..09dad071e4 100644 --- a/hw/9pfs/virtio-9p-posix-acl.c +++ b/hw/9pfs/virtio-9p-posix-acl.c @@ -114,7 +114,7 @@ static ssize_t mp_dacl_listxattr(FsContext *ctx, const char *path, } /* len includes the trailing NUL */ - memcpy(value, ACL_ACCESS, len); + memcpy(value, ACL_DEFAULT, len); return 0; } From 821c447675728ca06c8d2e4ac8a0e7a1adf775b8 Mon Sep 17 00:00:00 2001 From: Shannon Zhao Date: Mon, 16 Mar 2015 09:20:29 +0800 Subject: [PATCH 4/6] fsdev/virtfs-proxy-helper: Fix improper use of negative value It's detected by coverity. Check the return value of proxy_marshal. Signed-off-by: Shannon Zhao Signed-off-by: Shannon Zhao Signed-off-by: Aneesh Kumar K.V --- fsdev/virtfs-proxy-helper.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/fsdev/virtfs-proxy-helper.c b/fsdev/virtfs-proxy-helper.c index c1da2d78e7..bf2e5f3331 100644 --- a/fsdev/virtfs-proxy-helper.c +++ b/fsdev/virtfs-proxy-helper.c @@ -262,6 +262,9 @@ static int send_status(int sockfd, struct iovec *iovec, int status) */ msg_size = proxy_marshal(iovec, 0, "ddd", header.type, header.size, status); + if (msg_size < 0) { + return msg_size; + } retval = socket_write(sockfd, iovec->iov_base, msg_size); if (retval < 0) { return retval; From 25ee9a7fa3f4e09fde48bb184447ff5651ed5fd8 Mon Sep 17 00:00:00 2001 From: Shannon Zhao Date: Sat, 14 Mar 2015 10:00:16 +0800 Subject: [PATCH 5/6] virtfs-proxy: Fix possible overflow It's detected by coverity. The socket name specified should fit in the sockadd_un.sun_path. If not abort. Signed-off-by: Shannon Zhao Signed-off-by: Shannon Zhao Signed-off-by: Aneesh Kumar K.V --- fsdev/virtfs-proxy-helper.c | 1 + hw/9pfs/virtio-9p-proxy.c | 4 ++++ 2 files changed, 5 insertions(+) diff --git a/fsdev/virtfs-proxy-helper.c b/fsdev/virtfs-proxy-helper.c index bf2e5f3331..13fe032543 100644 --- a/fsdev/virtfs-proxy-helper.c +++ b/fsdev/virtfs-proxy-helper.c @@ -738,6 +738,7 @@ static int proxy_socket(const char *path, uid_t uid, gid_t gid) return -1; } + g_assert(strlen(path) < sizeof(proxy.sun_path)); sock = socket(AF_UNIX, SOCK_STREAM, 0); if (sock < 0) { do_perror("socket"); diff --git a/hw/9pfs/virtio-9p-proxy.c b/hw/9pfs/virtio-9p-proxy.c index 6bb191ee6a..71b6198bbd 100644 --- a/hw/9pfs/virtio-9p-proxy.c +++ b/hw/9pfs/virtio-9p-proxy.c @@ -1100,6 +1100,10 @@ static int connect_namedsocket(const char *path) int sockfd, size; struct sockaddr_un helper; + if (strlen(path) >= sizeof(helper.sun_path)) { + fprintf(stderr, "Socket name too large\n"); + return -1; + } sockfd = socket(AF_UNIX, SOCK_STREAM, 0); if (sockfd < 0) { fprintf(stderr, "failed to create socket: %s\n", strerror(errno)); From 4ed7b2c3a78f785a1bcbe575e08c379b166723e3 Mon Sep 17 00:00:00 2001 From: Stefan Weil Date: Sat, 14 Mar 2015 11:45:18 +0100 Subject: [PATCH 6/6] virtio: Fix memory leaks reported by Coverity All four leaks are similar, so fix them in one patch. Success path was not doing memory free. Signed-off-by: Stefan Weil Signed-off-by: Aneesh Kumar K.V --- hw/9pfs/virtio-9p-local.c | 28 ++++++++-------------------- 1 file changed, 8 insertions(+), 20 deletions(-) diff --git a/hw/9pfs/virtio-9p-local.c b/hw/9pfs/virtio-9p-local.c index 84efb31cfe..d6b1c0cdde 100644 --- a/hw/9pfs/virtio-9p-local.c +++ b/hw/9pfs/virtio-9p-local.c @@ -486,7 +486,7 @@ static int local_mknod(FsContext *fs_ctx, V9fsPath *dir_path, int err = -1; int serrno = 0; V9fsString fullname; - char *buffer; + char *buffer = NULL; v9fs_string_init(&fullname); v9fs_string_sprintf(&fullname, "%s/%s", dir_path->data, name); @@ -497,7 +497,6 @@ static int local_mknod(FsContext *fs_ctx, V9fsPath *dir_path, buffer = rpath(fs_ctx, path); err = mknod(buffer, SM_LOCAL_MODE_BITS|S_IFREG, 0); if (err == -1) { - g_free(buffer); goto out; } err = local_set_xattr(buffer, credp); @@ -510,7 +509,6 @@ static int local_mknod(FsContext *fs_ctx, V9fsPath *dir_path, buffer = rpath(fs_ctx, path); err = mknod(buffer, SM_LOCAL_MODE_BITS|S_IFREG, 0); if (err == -1) { - g_free(buffer); goto out; } err = local_set_mapped_file_attr(fs_ctx, path, credp); @@ -523,7 +521,6 @@ static int local_mknod(FsContext *fs_ctx, V9fsPath *dir_path, buffer = rpath(fs_ctx, path); err = mknod(buffer, credp->fc_mode, credp->fc_rdev); if (err == -1) { - g_free(buffer); goto out; } err = local_post_create_passthrough(fs_ctx, path, credp); @@ -537,8 +534,8 @@ static int local_mknod(FsContext *fs_ctx, V9fsPath *dir_path, err_end: remove(buffer); errno = serrno; - g_free(buffer); out: + g_free(buffer); v9fs_string_free(&fullname); return err; } @@ -550,7 +547,7 @@ static int local_mkdir(FsContext *fs_ctx, V9fsPath *dir_path, int err = -1; int serrno = 0; V9fsString fullname; - char *buffer; + char *buffer = NULL; v9fs_string_init(&fullname); v9fs_string_sprintf(&fullname, "%s/%s", dir_path->data, name); @@ -561,7 +558,6 @@ static int local_mkdir(FsContext *fs_ctx, V9fsPath *dir_path, buffer = rpath(fs_ctx, path); err = mkdir(buffer, SM_LOCAL_DIR_MODE_BITS); if (err == -1) { - g_free(buffer); goto out; } credp->fc_mode = credp->fc_mode|S_IFDIR; @@ -574,7 +570,6 @@ static int local_mkdir(FsContext *fs_ctx, V9fsPath *dir_path, buffer = rpath(fs_ctx, path); err = mkdir(buffer, SM_LOCAL_DIR_MODE_BITS); if (err == -1) { - g_free(buffer); goto out; } credp->fc_mode = credp->fc_mode|S_IFDIR; @@ -588,7 +583,6 @@ static int local_mkdir(FsContext *fs_ctx, V9fsPath *dir_path, buffer = rpath(fs_ctx, path); err = mkdir(buffer, credp->fc_mode); if (err == -1) { - g_free(buffer); goto out; } err = local_post_create_passthrough(fs_ctx, path, credp); @@ -602,8 +596,8 @@ static int local_mkdir(FsContext *fs_ctx, V9fsPath *dir_path, err_end: remove(buffer); errno = serrno; - g_free(buffer); out: + g_free(buffer); v9fs_string_free(&fullname); return err; } @@ -657,7 +651,7 @@ static int local_open2(FsContext *fs_ctx, V9fsPath *dir_path, const char *name, int err = -1; int serrno = 0; V9fsString fullname; - char *buffer; + char *buffer = NULL; /* * Mark all the open to not follow symlinks @@ -673,7 +667,6 @@ static int local_open2(FsContext *fs_ctx, V9fsPath *dir_path, const char *name, buffer = rpath(fs_ctx, path); fd = open(buffer, flags, SM_LOCAL_MODE_BITS); if (fd == -1) { - g_free(buffer); err = fd; goto out; } @@ -688,7 +681,6 @@ static int local_open2(FsContext *fs_ctx, V9fsPath *dir_path, const char *name, buffer = rpath(fs_ctx, path); fd = open(buffer, flags, SM_LOCAL_MODE_BITS); if (fd == -1) { - g_free(buffer); err = fd; goto out; } @@ -704,7 +696,6 @@ static int local_open2(FsContext *fs_ctx, V9fsPath *dir_path, const char *name, buffer = rpath(fs_ctx, path); fd = open(buffer, flags, credp->fc_mode); if (fd == -1) { - g_free(buffer); err = fd; goto out; } @@ -722,8 +713,8 @@ err_end: close(fd); remove(buffer); errno = serrno; - g_free(buffer); out: + g_free(buffer); v9fs_string_free(&fullname); return err; } @@ -736,7 +727,7 @@ static int local_symlink(FsContext *fs_ctx, const char *oldpath, int serrno = 0; char *newpath; V9fsString fullname; - char *buffer; + char *buffer = NULL; v9fs_string_init(&fullname); v9fs_string_sprintf(&fullname, "%s/%s", dir_path->data, name); @@ -749,7 +740,6 @@ static int local_symlink(FsContext *fs_ctx, const char *oldpath, buffer = rpath(fs_ctx, newpath); fd = open(buffer, O_CREAT|O_EXCL|O_RDWR|O_NOFOLLOW, SM_LOCAL_MODE_BITS); if (fd == -1) { - g_free(buffer); err = fd; goto out; } @@ -779,7 +769,6 @@ static int local_symlink(FsContext *fs_ctx, const char *oldpath, buffer = rpath(fs_ctx, newpath); fd = open(buffer, O_CREAT|O_EXCL|O_RDWR|O_NOFOLLOW, SM_LOCAL_MODE_BITS); if (fd == -1) { - g_free(buffer); err = fd; goto out; } @@ -808,7 +797,6 @@ static int local_symlink(FsContext *fs_ctx, const char *oldpath, buffer = rpath(fs_ctx, newpath); err = symlink(oldpath, buffer); if (err) { - g_free(buffer); goto out; } err = lchown(buffer, credp->fc_uid, credp->fc_gid); @@ -829,8 +817,8 @@ static int local_symlink(FsContext *fs_ctx, const char *oldpath, err_end: remove(buffer); errno = serrno; - g_free(buffer); out: + g_free(buffer); v9fs_string_free(&fullname); return err; }