Patchwork [1/1] hw/9pfs: Fix memory leak in error path

login
register
mail settings
Submitter Mohan Kumar M
Date July 4, 2013, 9:21 a.m.
Message ID <1372929678-14341-1-git-send-email-mohan@in.ibm.com>
Download mbox | patch
Permalink /patch/256843/
State New
Headers show

Comments

Mohan Kumar M - July 4, 2013, 9:21 a.m.
From: "M. Mohan Kumar" <mohan@in.ibm.com>

Fix few more memory leaks in virtio-9p-device.c detected using valgrind.

Signed-off-by: M. Mohan Kumar <mohan@in.ibm.com>
---
 hw/9pfs/virtio-9p-device.c | 26 +++++++++++++++++---------
 1 file changed, 17 insertions(+), 9 deletions(-)
Aneesh Kumar K.V - July 7, 2013, 5:18 p.m.
"M. Mohan Kumar" <mohan@in.ibm.com> writes:

> From: "M. Mohan Kumar" <mohan@in.ibm.com>
>
> Fix few more memory leaks in virtio-9p-device.c detected using valgrind.
>
> Signed-off-by: M. Mohan Kumar <mohan@in.ibm.com>
> ---
>  hw/9pfs/virtio-9p-device.c | 26 +++++++++++++++++---------
>  1 file changed, 17 insertions(+), 9 deletions(-)
>
> diff --git a/hw/9pfs/virtio-9p-device.c b/hw/9pfs/virtio-9p-device.c
> index dc6f4e4..35e2af4 100644
> --- a/hw/9pfs/virtio-9p-device.c
> +++ b/hw/9pfs/virtio-9p-device.c
> @@ -68,14 +68,14 @@ static int virtio_9p_device_init(VirtIODevice *vdev)
>          fprintf(stderr, "Virtio-9p device couldn't find fsdev with the "
>                  "id = %s\n",
>                  s->fsconf.fsdev_id ? s->fsconf.fsdev_id : "NULL");
> -        return -1;
> +        goto out;
>      }
>
>      if (!s->fsconf.tag) {
>          /* we haven't specified a mount_tag */
>          fprintf(stderr, "fsdev with id %s needs mount_tag arguments\n",
>                  s->fsconf.fsdev_id);
> -        return -1;
> +        goto out;
>      }
>
>      s->ctx.export_flags = fse->export_flags;
> @@ -85,10 +85,10 @@ static int virtio_9p_device_init(VirtIODevice *vdev)
>      if (len > MAX_TAG_LEN - 1) {
>          fprintf(stderr, "mount tag '%s' (%d bytes) is longer than "
>                  "maximum (%d bytes)", s->fsconf.tag, len, MAX_TAG_LEN - 1);
> -        return -1;
> +        goto out;
>      }
>
> -    s->tag = strdup(s->fsconf.tag);
> +    s->tag = g_strdup(s->fsconf.tag);
>      s->ctx.uid = -1;
>
>      s->ops = fse->ops;
> @@ -99,11 +99,11 @@ static int virtio_9p_device_init(VirtIODevice *vdev)
>      if (s->ops->init(&s->ctx) < 0) {
>          fprintf(stderr, "Virtio-9p Failed to initialize fs-driver with id:%s"
>                  " and export path:%s\n", s->fsconf.fsdev_id, s->ctx.fs_root);
> -        return -1;
> +        goto out;
>      }
>      if (v9fs_init_worker_threads() < 0) {
>          fprintf(stderr, "worker thread initialization failed\n");
> -        return -1;
> +        goto out;
>      }
>
>      /*
> @@ -115,18 +115,26 @@ static int virtio_9p_device_init(VirtIODevice *vdev)
>      if (s->ops->name_to_path(&s->ctx, NULL, "/", &path) < 0) {
>          fprintf(stderr,
>                  "error in converting name to path %s", strerror(errno));
> -        return -1;
> +        goto out;
>      }
>      if (s->ops->lstat(&s->ctx, &path, &stat)) {
>          fprintf(stderr, "share path %s does not exist\n", fse->path);
> -        return -1;
> +        goto out;
>      } else if (!S_ISDIR(stat.st_mode)) {
>          fprintf(stderr, "share path %s is not a directory\n", fse->path);
> -        return -1;
> +        goto out;
>      }
>      v9fs_path_free(&path);
>
>      return 0;
> +out:
> +    g_free(s->ctx.fs_root);
> +    g_free(s->tag);
> +    virtio_cleanup(vdev);
> +    v9fs_path_free(&path);
> +
> +    return -1;
> +
>  }
>

The error path will result in qemu terminating right ? Do we care about
g_free in those case ?

-aneesh
Andreas Färber - July 7, 2013, 5:31 p.m.
Am 07.07.2013 19:18, schrieb Aneesh Kumar K.V:
> "M. Mohan Kumar" <mohan@in.ibm.com> writes:
> 
>> From: "M. Mohan Kumar" <mohan@in.ibm.com>
>>
>> Fix few more memory leaks in virtio-9p-device.c detected using valgrind.
>>
>> Signed-off-by: M. Mohan Kumar <mohan@in.ibm.com>
>> ---
>>  hw/9pfs/virtio-9p-device.c | 26 +++++++++++++++++---------
>>  1 file changed, 17 insertions(+), 9 deletions(-)
>>
>> diff --git a/hw/9pfs/virtio-9p-device.c b/hw/9pfs/virtio-9p-device.c
>> index dc6f4e4..35e2af4 100644
>> --- a/hw/9pfs/virtio-9p-device.c
>> +++ b/hw/9pfs/virtio-9p-device.c
>> @@ -68,14 +68,14 @@ static int virtio_9p_device_init(VirtIODevice *vdev)
>>          fprintf(stderr, "Virtio-9p device couldn't find fsdev with the "
>>                  "id = %s\n",
>>                  s->fsconf.fsdev_id ? s->fsconf.fsdev_id : "NULL");
>> -        return -1;
>> +        goto out;
>>      }
>>
>>      if (!s->fsconf.tag) {
>>          /* we haven't specified a mount_tag */
>>          fprintf(stderr, "fsdev with id %s needs mount_tag arguments\n",
>>                  s->fsconf.fsdev_id);
>> -        return -1;
>> +        goto out;
>>      }
>>
>>      s->ctx.export_flags = fse->export_flags;
>> @@ -85,10 +85,10 @@ static int virtio_9p_device_init(VirtIODevice *vdev)
>>      if (len > MAX_TAG_LEN - 1) {
>>          fprintf(stderr, "mount tag '%s' (%d bytes) is longer than "
>>                  "maximum (%d bytes)", s->fsconf.tag, len, MAX_TAG_LEN - 1);
>> -        return -1;
>> +        goto out;
>>      }
>>
>> -    s->tag = strdup(s->fsconf.tag);
>> +    s->tag = g_strdup(s->fsconf.tag);
>>      s->ctx.uid = -1;
>>
>>      s->ops = fse->ops;
>> @@ -99,11 +99,11 @@ static int virtio_9p_device_init(VirtIODevice *vdev)
>>      if (s->ops->init(&s->ctx) < 0) {
>>          fprintf(stderr, "Virtio-9p Failed to initialize fs-driver with id:%s"
>>                  " and export path:%s\n", s->fsconf.fsdev_id, s->ctx.fs_root);
>> -        return -1;
>> +        goto out;
>>      }
>>      if (v9fs_init_worker_threads() < 0) {
>>          fprintf(stderr, "worker thread initialization failed\n");
>> -        return -1;
>> +        goto out;
>>      }
>>
>>      /*
>> @@ -115,18 +115,26 @@ static int virtio_9p_device_init(VirtIODevice *vdev)
>>      if (s->ops->name_to_path(&s->ctx, NULL, "/", &path) < 0) {
>>          fprintf(stderr,
>>                  "error in converting name to path %s", strerror(errno));
>> -        return -1;
>> +        goto out;
>>      }
>>      if (s->ops->lstat(&s->ctx, &path, &stat)) {
>>          fprintf(stderr, "share path %s does not exist\n", fse->path);
>> -        return -1;
>> +        goto out;
>>      } else if (!S_ISDIR(stat.st_mode)) {
>>          fprintf(stderr, "share path %s is not a directory\n", fse->path);
>> -        return -1;
>> +        goto out;
>>      }
>>      v9fs_path_free(&path);
>>
>>      return 0;
>> +out:
>> +    g_free(s->ctx.fs_root);
>> +    g_free(s->tag);
>> +    virtio_cleanup(vdev);
>> +    v9fs_path_free(&path);
>> +
>> +    return -1;
>> +
>>  }
>>
> 
> The error path will result in qemu terminating right ? Do we care about
> g_free in those case ?

See my patches on the list converting VirtioDevices to QOM realize:
Realization grows an Error **errp argument for reporting errors, and
it's up to the caller to decide whether to continue (think device_add)
or terminate (-device).

Andreas
Anthony Liguori - July 10, 2013, 7:33 p.m.
Applied.  Thanks.

Regards,

Anthony Liguori

Patch

diff --git a/hw/9pfs/virtio-9p-device.c b/hw/9pfs/virtio-9p-device.c
index dc6f4e4..35e2af4 100644
--- a/hw/9pfs/virtio-9p-device.c
+++ b/hw/9pfs/virtio-9p-device.c
@@ -68,14 +68,14 @@  static int virtio_9p_device_init(VirtIODevice *vdev)
         fprintf(stderr, "Virtio-9p device couldn't find fsdev with the "
                 "id = %s\n",
                 s->fsconf.fsdev_id ? s->fsconf.fsdev_id : "NULL");
-        return -1;
+        goto out;
     }
 
     if (!s->fsconf.tag) {
         /* we haven't specified a mount_tag */
         fprintf(stderr, "fsdev with id %s needs mount_tag arguments\n",
                 s->fsconf.fsdev_id);
-        return -1;
+        goto out;
     }
 
     s->ctx.export_flags = fse->export_flags;
@@ -85,10 +85,10 @@  static int virtio_9p_device_init(VirtIODevice *vdev)
     if (len > MAX_TAG_LEN - 1) {
         fprintf(stderr, "mount tag '%s' (%d bytes) is longer than "
                 "maximum (%d bytes)", s->fsconf.tag, len, MAX_TAG_LEN - 1);
-        return -1;
+        goto out;
     }
 
-    s->tag = strdup(s->fsconf.tag);
+    s->tag = g_strdup(s->fsconf.tag);
     s->ctx.uid = -1;
 
     s->ops = fse->ops;
@@ -99,11 +99,11 @@  static int virtio_9p_device_init(VirtIODevice *vdev)
     if (s->ops->init(&s->ctx) < 0) {
         fprintf(stderr, "Virtio-9p Failed to initialize fs-driver with id:%s"
                 " and export path:%s\n", s->fsconf.fsdev_id, s->ctx.fs_root);
-        return -1;
+        goto out;
     }
     if (v9fs_init_worker_threads() < 0) {
         fprintf(stderr, "worker thread initialization failed\n");
-        return -1;
+        goto out;
     }
 
     /*
@@ -115,18 +115,26 @@  static int virtio_9p_device_init(VirtIODevice *vdev)
     if (s->ops->name_to_path(&s->ctx, NULL, "/", &path) < 0) {
         fprintf(stderr,
                 "error in converting name to path %s", strerror(errno));
-        return -1;
+        goto out;
     }
     if (s->ops->lstat(&s->ctx, &path, &stat)) {
         fprintf(stderr, "share path %s does not exist\n", fse->path);
-        return -1;
+        goto out;
     } else if (!S_ISDIR(stat.st_mode)) {
         fprintf(stderr, "share path %s is not a directory\n", fse->path);
-        return -1;
+        goto out;
     }
     v9fs_path_free(&path);
 
     return 0;
+out:
+    g_free(s->ctx.fs_root);
+    g_free(s->tag);
+    virtio_cleanup(vdev);
+    v9fs_path_free(&path);
+
+    return -1;
+
 }
 
 /* virtio-9p device */