| Message ID | 1456120999-5639-3-git-send-email-jgross@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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()? Daniel
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
22.02.2016 11:24, Daniel Kiper ?????: > > Are you sure that grub_errno is always set to GRUB_ERR_NONE > if any GRUB2 function finished successfully? grub_errno is reset by command parser before command execution (or after previous command finished actually). During command execution there is no guarantee that this happens; sometimes grub_errno is explicitly reset to suppress subsequent errors but in general errors are just passed through. > Maybe you should > set initialize grub_errno with GRUB_ERR_NONE at the beginning > of parse_xen_guest()? > > Daniel > > _______________________________________________ > Grub-devel mailing list > Grub-devel@gnu.org > https://lists.gnu.org/mailman/listinfo/grub-devel >
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; } #pragma GCC diagnostic ignored "-Wcast-align"
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(-)