Re: [Qemu-devel] [PATCH 2/2] ahci: Fix sglist memleak in ahci_dma_rw_buf()

[Paolo Bonzini <pbonzini@redhat.com>]

Il 26/07/2012 21:40, Jason Baron ha scritto:
> I noticed that in hw/ide/ahci:ahci_dma_rw_buf() does not appear to free the
> sglist. Thus, I've added a call to qemu_sglist_destroy() to fix this memory
> leak.
> 
> I'm wondering though if 'ahci_populate_sglist()' can return 0, and not
> populate the sglist, thus causing us to call free on NULL pointer. However, I
> see that ahci_start_transfer() always calls the free if the return is 0.

A free(NULL) is ok, but a double-free would not be.  Something like this
would make me feel better:

diff --git a/dma-helpers.c b/dma-helpers.c
index 35cb500..57725d0 100644
--- a/dma-helpers.c
+++ b/dma-helpers.c
@@ -65,6 +65,7 @@
 void qemu_sglist_destroy(QEMUSGList *qsg)
 {
     g_free(qsg->sg);
+    memset(qsg, 0, sizeof(qsg));
 }

 typedef struct {

Paolo

> Signed-off-by: Jason Baron <jbaron@redhat.com>
> ---
>  hw/ide/ahci.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
> index 9c95714..b48401d 100644
> --- a/hw/ide/ahci.c
> +++ b/hw/ide/ahci.c
> @@ -1073,6 +1073,9 @@ static int ahci_dma_rw_buf(IDEDMA *dma, int is_write)
>          dma_buf_write(p, l, &s->sg);
>      }
>  
> +    /* free sglist that was created in ahci_populate_sglist() */
> +    qemu_sglist_destroy(&s->sg);
> +
>      /* update number of transferred bytes */
>      ad->cur_cmd->status = cpu_to_le32(le32_to_cpu(ad->cur_cmd->status) + l);
>      s->io_buffer_index += l;
>