[Qemu-devel] [PATCH] block: fix memory leak in early exit

[Qemu-devel] [PATCH] block: fix memory leak in early exit

[Stefan Hajnoczi]

The stream block job has two early exit code paths.  They do not free
s->backing_file_str.

Also, the early exits rely on the fact that the coroutine hasn't yielded
yet and was launched from the main thread.  Therefore the coroutine is
guaranteed to be running in the main thread where block_job_completed()
may be called safely.  This is very subtle so it's nice to eliminate the
assumption by unifying the early exit with the normal exit code path.

Cc: Fam Zheng <famz@redhat.com>
Cc: Jeff Cody <jcody@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/stream.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/block/stream.c b/block/stream.c
index ab0bd05..1986e9a 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -120,16 +120,16 @@ static void coroutine_fn stream_run(void *opaque)
     int ret = 0;
     int n = 0;
     void *buf;
+    bool reached_end = false;
 
     if (!bs->backing_hd) {
-        block_job_completed(&s->common, 0);
-        return;
+        goto out;
     }
 
     s->common.len = bdrv_getlength(bs);
     if (s->common.len < 0) {
-        block_job_completed(&s->common, s->common.len);
-        return;
+        ret = s->common.len;
+        goto out;
     }
 
     end = s->common.len >> BDRV_SECTOR_BITS;
@@ -207,6 +207,10 @@ wait:
         s->common.offset += n * BDRV_SECTOR_SIZE;
     }
 
+    if (sector_num == end) {
+        reached_end = true;
+    }
+
     if (!base) {
         bdrv_disable_copy_on_read(bs);
     }
@@ -216,10 +220,11 @@ wait:
 
     qemu_vfree(buf);
 
+out:
     /* Modify backing chain and close BDSes in main loop */
     data = g_malloc(sizeof(*data));
     data->ret = ret;
-    data->reached_end = sector_num == end;
+    data->reached_end = reached_end;
     block_job_defer_to_main_loop(&s->common, stream_complete, data);
 }
 
-- 
2.4.3

Re: [Qemu-devel] [PATCH] block: fix memory leak in early exit

[Fam Zheng]

On Thu, 10/15 17:54, Stefan Hajnoczi wrote:
> The stream block job has two early exit code paths.  They do not free
> s->backing_file_str.
> 
> Also, the early exits rely on the fact that the coroutine hasn't yielded
> yet and was launched from the main thread.  Therefore the coroutine is
> guaranteed to be running in the main thread where block_job_completed()
> may be called safely.  This is very subtle so it's nice to eliminate the
> assumption by unifying the early exit with the normal exit code path.
> 
> Cc: Fam Zheng <famz@redhat.com>
> Cc: Jeff Cody <jcody@redhat.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  block/stream.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/block/stream.c b/block/stream.c
> index ab0bd05..1986e9a 100644
> --- a/block/stream.c
> +++ b/block/stream.c
> @@ -120,16 +120,16 @@ static void coroutine_fn stream_run(void *opaque)
>      int ret = 0;
>      int n = 0;
>      void *buf;
> +    bool reached_end = false;
>  
>      if (!bs->backing_hd) {
> -        block_job_completed(&s->common, 0);
> -        return;
> +        goto out;
>      }
>  
>      s->common.len = bdrv_getlength(bs);
>      if (s->common.len < 0) {
> -        block_job_completed(&s->common, s->common.len);
> -        return;
> +        ret = s->common.len;
> +        goto out;
>      }
>  
>      end = s->common.len >> BDRV_SECTOR_BITS;
> @@ -207,6 +207,10 @@ wait:
>          s->common.offset += n * BDRV_SECTOR_SIZE;
>      }
>  
> +    if (sector_num == end) {
> +        reached_end = true;
> +    }
> +
>      if (!base) {
>          bdrv_disable_copy_on_read(bs);
>      }
> @@ -216,10 +220,11 @@ wait:
>  
>      qemu_vfree(buf);
>  
> +out:
>      /* Modify backing chain and close BDSes in main loop */
>      data = g_malloc(sizeof(*data));
>      data->ret = ret;
> -    data->reached_end = sector_num == end;
> +    data->reached_end = reached_end;
>      block_job_defer_to_main_loop(&s->common, stream_complete, data);
>  }
>  
> -- 
> 2.4.3
> 

Reviewed-by: Fam Zheng <famz@redhat.com>

Re: [Qemu-devel] [PATCH] block: fix memory leak in early exit

[Jeff Cody]

On Thu, Oct 15, 2015 at 05:54:27PM +0200, Stefan Hajnoczi wrote:
> The stream block job has two early exit code paths.  They do not free
> s->backing_file_str.
> 
> Also, the early exits rely on the fact that the coroutine hasn't yielded
> yet and was launched from the main thread.  Therefore the coroutine is
> guaranteed to be running in the main thread where block_job_completed()
> may be called safely.  This is very subtle so it's nice to eliminate the
> assumption by unifying the early exit with the normal exit code path.
> 
> Cc: Fam Zheng <famz@redhat.com>
> Cc: Jeff Cody <jcody@redhat.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  block/stream.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/block/stream.c b/block/stream.c
> index ab0bd05..1986e9a 100644
> --- a/block/stream.c
> +++ b/block/stream.c
> @@ -120,16 +120,16 @@ static void coroutine_fn stream_run(void *opaque)
>      int ret = 0;
>      int n = 0;
>      void *buf;
> +    bool reached_end = false;
>  
>      if (!bs->backing_hd) {
> -        block_job_completed(&s->common, 0);
> -        return;
> +        goto out;
>      }
>  
>      s->common.len = bdrv_getlength(bs);
>      if (s->common.len < 0) {
> -        block_job_completed(&s->common, s->common.len);
> -        return;
> +        ret = s->common.len;
> +        goto out;
>      }
>  
>      end = s->common.len >> BDRV_SECTOR_BITS;
> @@ -207,6 +207,10 @@ wait:
>          s->common.offset += n * BDRV_SECTOR_SIZE;
>      }
>  
> +    if (sector_num == end) {
> +        reached_end = true;
> +    }
> +
>      if (!base) {
>          bdrv_disable_copy_on_read(bs);
>      }
> @@ -216,10 +220,11 @@ wait:
>  
>      qemu_vfree(buf);
>  
> +out:
>      /* Modify backing chain and close BDSes in main loop */
>      data = g_malloc(sizeof(*data));
>      data->ret = ret;
> -    data->reached_end = sector_num == end;
> +    data->reached_end = reached_end;
>      block_job_defer_to_main_loop(&s->common, stream_complete, data);
>  }
>  
> -- 
> 2.4.3
>

Reviewed-by: Jeff Cody <jcody@redhat.com>

Re: [Qemu-devel] [Qemu-block] [PATCH] block: fix memory leak in early exit

[Alberto Garcia]

On Thu 15 Oct 2015 05:54:27 PM CEST, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> The stream block job has two early exit code paths.  They do not free
> s->backing_file_str.
>
> Also, the early exits rely on the fact that the coroutine hasn't yielded
> yet and was launched from the main thread.  Therefore the coroutine is
> guaranteed to be running in the main thread where block_job_completed()
> may be called safely.  This is very subtle so it's nice to eliminate the
> assumption by unifying the early exit with the normal exit code path.
>
> Cc: Fam Zheng <famz@redhat.com>
> Cc: Jeff Cody <jcody@redhat.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>

I had a slightly simpler version of this in my intermediate block
streaming series in case you're interested:

   https://patchwork.ozlabs.org/patch/471881/

But this one looks good to me too, so:

Reviewed-by: Alberto Garcia <berto@igalia.com>

Berto

Re: [Qemu-devel] [Qemu-block] [PATCH] block: fix memory leak in early exit

[Stefan Hajnoczi]

On Fri, Oct 16, 2015 at 08:58:12AM +0200, Alberto Garcia wrote:
> On Thu 15 Oct 2015 05:54:27 PM CEST, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> > The stream block job has two early exit code paths.  They do not free
> > s->backing_file_str.
> >
> > Also, the early exits rely on the fact that the coroutine hasn't yielded
> > yet and was launched from the main thread.  Therefore the coroutine is
> > guaranteed to be running in the main thread where block_job_completed()
> > may be called safely.  This is very subtle so it's nice to eliminate the
> > assumption by unifying the early exit with the normal exit code path.
> >
> > Cc: Fam Zheng <famz@redhat.com>
> > Cc: Jeff Cody <jcody@redhat.com>
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> 
> I had a slightly simpler version of this in my intermediate block
> streaming series in case you're interested:
> 
>    https://patchwork.ozlabs.org/patch/471881/
> 
> But this one looks good to me too, so:
> 
> Reviewed-by: Alberto Garcia <berto@igalia.com>

Kevin, please take Alberto's patch instead of mine.  The Message-ID is:

  d575a576c18d8972ac1a200c4022b39cbbce2507.1435008395.git.berto@igalia.com

BTW, I notice that Jeff isn't listed as maintainer for block/stream.c.
So according to MAINTAINERS this patch goes through you.

Stefan

Re: [Qemu-devel] [Qemu-block] [PATCH] block: fix memory leak in early exit

[Jeff Cody]

On Fri, Oct 16, 2015 at 10:37:17AM +0200, Stefan Hajnoczi wrote:
> On Fri, Oct 16, 2015 at 08:58:12AM +0200, Alberto Garcia wrote:
> > On Thu 15 Oct 2015 05:54:27 PM CEST, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> > > The stream block job has two early exit code paths.  They do not free
> > > s->backing_file_str.
> > >
> > > Also, the early exits rely on the fact that the coroutine hasn't yielded
> > > yet and was launched from the main thread.  Therefore the coroutine is
> > > guaranteed to be running in the main thread where block_job_completed()
> > > may be called safely.  This is very subtle so it's nice to eliminate the
> > > assumption by unifying the early exit with the normal exit code path.
> > >
> > > Cc: Fam Zheng <famz@redhat.com>
> > > Cc: Jeff Cody <jcody@redhat.com>
> > > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > 
> > I had a slightly simpler version of this in my intermediate block
> > streaming series in case you're interested:
> > 
> >    https://patchwork.ozlabs.org/patch/471881/
> > 
> > But this one looks good to me too, so:
> > 
> > Reviewed-by: Alberto Garcia <berto@igalia.com>
> 
> Kevin, please take Alberto's patch instead of mine.  The Message-ID is:
> 
>   d575a576c18d8972ac1a200c4022b39cbbce2507.1435008395.git.berto@igalia.com
> 
> BTW, I notice that Jeff isn't listed as maintainer for block/stream.c.
> So according to MAINTAINERS this patch goes through you.
> 
> Stefan

I think that is probably just a typo - it lists block/stream.h in
MAINTAINERS, not block/stream.c.