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

[Juergen Gross <jgross@suse.com>]

On 22/02/16 09:24, Daniel Kiper wrote:
> 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@suse.com>
>> ---
>>  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()?

Yes, probably a good idea.

Juergen