Re: [PATCH 2/4] kvm: qemu: Fix leak of ioperm data

[Mark McLoughlin <markmc@redhat.com>]

On Wed, 2008-12-10 at 17:44 +0800, Han, Weidong wrote:

> diff --git a/qemu/qemu-kvm.c b/qemu/qemu-kvm.c
> index 067cf03..283bfa9 100644
> --- a/qemu/qemu-kvm.c
> +++ b/qemu/qemu-kvm.c
> @@ -1145,6 +1145,18 @@ void kvm_add_ioperm_data(struct ioperm_data *data)
>      LIST_INSERT_HEAD(&ioperm_head, data, entries);
>  }
>  
> +void kvm_remove_ioperm_data(unsigned long start_port, unsigned long num)
> +{
> +    struct ioperm_data *data;
> +
> +    LIST_FOREACH(data, &ioperm_head, entries) {
> +        if (data->start_port == start_port &&
> +            data->num == num)
> +            LIST_REMOVE(data, entries);
> +            qemu_free(data);

Two issues here:

  1) You've a "use after free bug" here; you free the structure and
     LIST_FOREACH de-references the pointer to it in order to obtain the
     pointer to the next structure.

  2) Even more serious, you're missing a set of braces so you're 
     actually freeing every structure.

What you need is:

{
    struct ioperm_data *data;

    data = LIST_FIRST(&ioperm_head);
    while (data) {
        struct ioperm_data *next = LIST_NEXT(data, entries);

        if (data->start_port == start_port && data->num == num) {
            LIST_REMOVE(data, entries);
            qemu_free(data);
        }

        data = next;
    }
}

Cheers,
Mark.