Re: [Xen-devel] [PATCH v4 02/11] xen: avoid memleaks on error

[Daniel Kiper <daniel.kiper@xxxxxxxxxx>]

On Mon, Feb 22, 2016 at 07:03:10AM +0100, Juergen Gross wrote:
> When loading a Xen pv-kernel avoid memory leaks in case of errors.
>
> Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
> ---
>  grub-core/loader/i386/xen.c        |  2 +-
>  grub-core/loader/i386/xen_fileXX.c | 17 +++++++++++------
>  2 files changed, 12 insertions(+), 7 deletions(-)
>
> diff --git a/grub-core/loader/i386/xen.c b/grub-core/loader/i386/xen.c
> index 179e89c..f45f70f 100644
> --- a/grub-core/loader/i386/xen.c
> +++ b/grub-core/loader/i386/xen.c
> @@ -563,7 +563,7 @@ grub_cmd_initrd (grub_command_t cmd __attribute__ 
> ((unused)),
>      {
>        err = grub_relocator_alloc_chunk_addr (relocator, &ch, max_addr, size);
>        if (err)
> -     return err;
> +     goto fail;
>
>        if (grub_initrd_load (&initrd_ctx, argv,
>                           get_virtual_current_address (ch)))
> diff --git a/grub-core/loader/i386/xen_fileXX.c 
> b/grub-core/loader/i386/xen_fileXX.c
> index 1ba5649..5475819 100644
> --- a/grub-core/loader/i386/xen_fileXX.c
> +++ b/grub-core/loader/i386/xen_fileXX.c
> @@ -35,7 +35,8 @@ parse_xen_guest (grub_elf_t elf, struct grub_xen_file_info 
> *xi,
>    if (grub_file_read (elf->file, buf, sz) != (grub_ssize_t) sz)
>      {
>        if (grub_errno)
> -     return grub_errno;
> +     goto out;
> +      grub_free (buf);
>        return grub_error (GRUB_ERR_BAD_OS, N_("premature end of file %s"),
>                        elf->file->name);
>      }
> @@ -123,14 +124,14 @@ parse_xen_guest (grub_elf_t elf, struct 
> grub_xen_file_info *xi,
>       {
>         xi->virt_base = grub_strtoull (ptr + sizeof ("VIRT_BASE=") - 1, &ptr, 
> 16);
>         if (grub_errno)
> -         return grub_errno;
> +         goto out;
>         continue;
>       }
>        if (grub_strncmp (ptr, "VIRT_ENTRY=", sizeof ("VIRT_ENTRY=") - 1) == 0)
>       {
>         xi->entry_point = grub_strtoull (ptr + sizeof ("VIRT_ENTRY=") - 1, 
> &ptr, 16);
>         if (grub_errno)
> -         return grub_errno;
> +         goto out;
>         continue;
>       }
>        if (grub_strncmp (ptr, "HYPERCALL_PAGE=", sizeof ("HYPERCALL_PAGE=") - 
> 1) == 0)
> @@ -138,7 +139,7 @@ parse_xen_guest (grub_elf_t elf, struct 
> grub_xen_file_info *xi,
>         xi->hypercall_page = grub_strtoull (ptr + sizeof ("HYPERCALL_PAGE=") 
> - 1, &ptr, 16);
>         xi->has_hypercall_page = 1;
>         if (grub_errno)
> -         return grub_errno;
> +         goto out;
>         continue;
>       }
>        if (grub_strncmp (ptr, "ELF_PADDR_OFFSET=", sizeof 
> ("ELF_PADDR_OFFSET=") - 1) == 0)
> @@ -146,7 +147,7 @@ parse_xen_guest (grub_elf_t elf, struct 
> grub_xen_file_info *xi,
>         xi->paddr_offset = grub_strtoull (ptr + sizeof ("ELF_PADDR_OFFSET=") 
> - 1, &ptr, 16);
>         has_paddr = 1;
>         if (grub_errno)
> -         return grub_errno;
> +         goto out;
>         continue;
>       }
>      }
> @@ -154,7 +155,11 @@ parse_xen_guest (grub_elf_t elf, struct 
> grub_xen_file_info *xi,
>      xi->hypercall_page = (xi->hypercall_page << 12) + xi->virt_base;
>    if (!has_paddr)
>      xi->paddr_offset = xi->virt_base;
> -  return GRUB_ERR_NONE;
> +
> +out:
> +  grub_free (buf);
> +
> +  return grub_errno;

Are you sure that grub_errno is always set to GRUB_ERR_NONE
if any GRUB2 function finished successfully? Maybe you should
set initialize grub_errno with GRUB_ERR_NONE at the beginning
of parse_xen_guest()?

Daniel

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel