[Qemu-devel] [PULL] VirtFS update

[Qemu-devel] [PULL] VirtFS update

[Aneesh Kumar K.V]

Hi,

Please pull the below update for VirtFS

The following changes since commit ee74801035b0b5f1fdfd4e31d3a53f511f91c804:

  Merge remote-tracking branch 'remotes/lalrae/tags/mips-20150311' into staging (2015-03-11 18:22:15 +0000)

are available in the git repository at:

  https://github.com/kvaneesh/qemu.git for-upstream

for you to fetch changes up to 4ed7b2c3a78f785a1bcbe575e08c379b166723e3:

  virtio: Fix memory leaks reported by Coverity (2015-03-16 13:32:24 +0530)

----------------------------------------------------------------
Michael Tokarev (2):
      9pfs-local: simplify/optimize local_mapped_attr_path()
      9pfs-proxy: tiny cleanups in proxy_pwritev and proxy_preadv

Shannon Zhao (3):
      hw/9pfs/virtio-9p-posix-acl: Fix out-of-bounds access
      fsdev/virtfs-proxy-helper: Fix improper use of negative value
      virtfs-proxy: Fix possible overflow

Stefan Weil (1):
      virtio: Fix memory leaks reported by Coverity

 fsdev/virtfs-proxy-helper.c   |  4 ++++
 hw/9pfs/virtio-9p-local.c     | 52 ++++++++++++++++---------------------------
 hw/9pfs/virtio-9p-posix-acl.c |  2 +-
 hw/9pfs/virtio-9p-proxy.c     | 22 +++++++++---------
 4 files changed, 36 insertions(+), 44 deletions(-)

-- 
2.1.0

[Qemu-devel] [PATCH 1/6] 9pfs-local: simplify/optimize local_mapped_attr_path()

[Aneesh Kumar K.V]

From: Michael Tokarev <mjt@tls.msk.ru>

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 <mjt@tls.msk.ru>
Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
---
 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 d05c91779f2c..84efb31cfec4 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)
-- 
2.1.0

[Qemu-devel] [PATCH 2/6] 9pfs-proxy: tiny cleanups in proxy_pwritev and proxy_preadv

[Aneesh Kumar K.V]

From: Michael Tokarev <mjt@tls.msk.ru>

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 <mjt@tls.msk.ru>
Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
---
 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 59c7445deab9..6bb191ee6ab8 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
-- 
2.1.0

[Qemu-devel] [PATCH 3/6] hw/9pfs/virtio-9p-posix-acl: Fix out-of-bounds access

[Aneesh Kumar K.V]

From: Shannon Zhao <zhaoshenglong@huawei.com>

It's detected by coverity. Fix out-of-bounds access of the function mp_dacl_listxattr.

Signed-off-by: Shannon Zhao <zhaoshenglong@huawei.com>
Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
---
 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 803d9d94f3b8..09dad071e487 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;
 }
 
-- 
2.1.0

[Qemu-devel] [PATCH 4/6] fsdev/virtfs-proxy-helper: Fix improper use of negative value

[Aneesh Kumar K.V]

From: Shannon Zhao <zhaoshenglong@huawei.com>

It's detected by coverity. Check the return value of proxy_marshal.

Signed-off-by: Shannon Zhao <zhaoshenglong@huawei.com>
Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
---
 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 c1da2d78e78b..bf2e5f333121 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;
-- 
2.1.0

[Qemu-devel] [PATCH 5/6] virtfs-proxy: Fix possible overflow

[Aneesh Kumar K.V]

From: Shannon Zhao <zhaoshenglong@huawei.com>

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 <zhaoshenglong@huawei.com>
Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
---
 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 bf2e5f333121..13fe032543bc 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 6bb191ee6ab8..71b6198bbd22 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));
-- 
2.1.0

[Qemu-devel] [PATCH 6/6] virtio: Fix memory leaks reported by Coverity

[Aneesh Kumar K.V]

From: Stefan Weil <sw@weilnetz.de>

All four leaks are similar, so fix them in one patch.
Success path was not doing memory free.

Signed-off-by: Stefan Weil <sw@weilnetz.de>
Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
---
 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 84efb31cfec4..d6b1c0cddef9 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;
 }
-- 
2.1.0

Re: [Qemu-devel] [PULL] VirtFS update

[Peter Maydell]

On 16 March 2015 at 10:09, Aneesh Kumar K.V
<aneesh.kumar@linux.vnet.ibm.com> wrote:
>
> Hi,
>
> Please pull the below update for VirtFS
>
> The following changes since commit ee74801035b0b5f1fdfd4e31d3a53f511f91c804:
>
>   Merge remote-tracking branch 'remotes/lalrae/tags/mips-20150311' into staging (2015-03-11 18:22:15 +0000)
>
> are available in the git repository at:
>
>   https://github.com/kvaneesh/qemu.git for-upstream
>
> for you to fetch changes up to 4ed7b2c3a78f785a1bcbe575e08c379b166723e3:

Nudge: this is not a signed pull request. I am currently still
accepting and applying these, but after 2.3 is out I plan to
start rejecting them. You should update your workflow (ask me
if you're not sure how, it's not too much different).

>   virtio: Fix memory leaks reported by Coverity (2015-03-16 13:32:24 +0530)

Applied, thanks.

-- PMM