[PATCH] kexec-tools: Add some missing free() calls
Bhupesh SHARMA
bhupesh.linux at gmail.com
Wed Sep 16 16:20:05 EDT 2020
Hi Youling,
See some comments inline:
On Sat, Sep 12, 2020 at 7:11 AM Youling Tang <tangyouling at loongson.cn> wrote:
>
> Add some missing free() calls.
>
> Signed-off-by: Youling Tang <tangyouling at loongson.cn>
> ---
> kexec/arch/i386/crashdump-x86.c | 22 +++++++++++++++++-----
> kexec/arch/mips/crashdump-mips.c | 5 ++++-
> kexec/arch/ppc64/crashdump-ppc64.c | 8 ++++++--
> 3 files changed, 27 insertions(+), 8 deletions(-)
First, I think this is a step in the right direction, however, earlier
also while running 'valgrind' on an x86_64 kexec elf I saw the
following memory leaks reported:
==596886== 15,604 bytes in 1 blocks are indirectly lost in loss record 4 of 12
==596886== at 0x483A809: malloc (vg_replace_malloc.c:307)
==596886== by 0x40396D: xmalloc (kexec.c:101)
==596886== by 0x410D35: do_bzImage64_load (kexec-bzImage64.c:182)
==596886== by 0x410D35: bzImage64_load (kexec-bzImage64.c:391)
==596886== by 0x404410: my_load (kexec.c:774)
==596886== by 0x402D2D: main (kexec.c:1605)
==596886== 15,732 (128 direct, 15,604 indirect) bytes in 1 blocks are
definitely lost in loss record 5 of 12
==596886== at 0x483CCE8: realloc (vg_replace_malloc.c:834)
==596886== by 0x403FB9: xrealloc (kexec.c:112)
==596886== by 0x403FB9: add_segment_phys_virt (kexec.c:357)
==596886== by 0x40410F: add_buffer_phys_virt (kexec.c:392)
==596886== by 0x404153: add_buffer_virt (kexec.c:401)
==596886== by 0x40CD11: setup_linux_bootloader_parameters_high
(x86-linux-setup.c:80)
==596886== by 0x410E6A: do_bzImage64_load (kexec-bzImage64.c:214)
==596886== by 0x410E6A: bzImage64_load (kexec-bzImage64.c:391)
==596886== by 0x404410: my_load (kexec.c:774)
==596886== by 0x402D2D: main (kexec.c:1605)
==596886== 28,896 bytes in 1 blocks are indirectly lost in loss record 7 of 12
==596886== at 0x483A809: malloc (vg_replace_malloc.c:307)
==596886== by 0x40396D: xmalloc (kexec.c:101)
==596886== by 0x406781: elf_rel_load (kexec-elf-rel.c:254)
==596886== by 0x406EEA: elf_rel_build_load (kexec-elf-rel.c:432)
==596886== by 0x410CFE: do_bzImage64_load (kexec-bzImage64.c:173)
==596886== by 0x410CFE: bzImage64_load (kexec-bzImage64.c:391)
==596886== by 0x404410: my_load (kexec.c:774)
==596886== by 0x402D2D: main (kexec.c:1605)
==596886== 30,048 (1,152 direct, 28,896 indirect) bytes in 1 blocks
are definitely lost in loss record 8 of 12
==596886== at 0x483A809: malloc (vg_replace_malloc.c:307)
==596886== by 0x40396D: xmalloc (kexec.c:101)
==596886== by 0x405735: build_mem_shdrs (kexec-elf.c:618)
==596886== by 0x405735: build_elf_info (kexec-elf.c:774)
==596886== by 0x406EB9: build_elf_rel_info (kexec-elf-rel.c:142)
==596886== by 0x406EB9: elf_rel_build_load (kexec-elf-rel.c:427)
==596886== by 0x410CFE: do_bzImage64_load (kexec-bzImage64.c:173)
==596886== by 0x410CFE: bzImage64_load (kexec-bzImage64.c:391)
==596886== by 0x404410: my_load (kexec.c:774)
==596886== by 0x402D2D: main (kexec.c:1605)
==596886==
Note that there were 12 issues highlighted via valgrind out of which I
have removed the zlib related issue reports.
You can run 'valgrind' on mips, i386 and ppc64 executables (as shown
below) to see if all such issues are fixed by your patch:
$ sudo valgrind --leak-check=full --show-leak-kinds=all
--track-origins=yes --verbose --log-file=valgrind-out.txt ./kexec -l
/boot/vmlinuz-`uname -r` --initrd=/boot/initramfs-`uname -r`.img
--reuse-cmdline -d
> diff --git a/kexec/arch/i386/crashdump-x86.c b/kexec/arch/i386/crashdump-x86.c
> index c79791f..d5b5b68 100644
> --- a/kexec/arch/i386/crashdump-x86.c
> +++ b/kexec/arch/i386/crashdump-x86.c
> @@ -913,8 +913,11 @@ int load_crashdump_segments(struct kexec_info *info, char* mod_cmdline,
> add_memmap(memmap_p, &nr_memmap, info->backup_src_start, info->backup_src_size, RANGE_RAM);
> for (i = 0; i < crash_reserved_mem_nr; i++) {
> sz = crash_reserved_mem[i].end - crash_reserved_mem[i].start +1;
> - if (add_memmap(memmap_p, &nr_memmap, crash_reserved_mem[i].start, sz, RANGE_RAM) < 0)
> + if (add_memmap(memmap_p, &nr_memmap, crash_reserved_mem[i].start,
> + sz, RANGE_RAM) < 0) {
> + free(memmap_p);
> return ENOCRASHKERNEL;
> + }
> }
>
> /* Create a backup region segment to store backup data*/
> @@ -926,22 +929,29 @@ int load_crashdump_segments(struct kexec_info *info, char* mod_cmdline,
> 0, max_addr, -1);
> dbgprintf("Created backup segment at 0x%lx\n",
> info->backup_start);
> - if (delete_memmap(memmap_p, &nr_memmap, info->backup_start, sz) < 0)
> + if (delete_memmap(memmap_p, &nr_memmap, info->backup_start, sz) < 0) {
> + free(tmp);
> + free(memmap_p);
> return EFAILED;
> + }
> }
Can we use the error handler path we use in other kexec source files
(see 'kexec/arch/arm64/kexec-arm64.c') for freeing up the resources,
something like:
on_error_tmp:
if (tmp)
free(tmp);
on_error_memmap_p:
if (memmap_p)
free(memmap_p);
as same for the changes done below in this patch ...
> /* Create elf header segment and store crash image data. */
> if (arch_options.core_header_type == CORE_TYPE_ELF64) {
> if (crash_create_elf64_headers(info, &elf_info, mem_range,
> nr_ranges, &tmp, &bufsz,
> - ELF_CORE_HEADER_ALIGN) < 0)
> + ELF_CORE_HEADER_ALIGN) < 0) {
> + free(memmap_p);
> return EFAILED;
> + }
> }
> else {
> if (crash_create_elf32_headers(info, &elf_info, mem_range,
> nr_ranges, &tmp, &bufsz,
> - ELF_CORE_HEADER_ALIGN) < 0)
> + ELF_CORE_HEADER_ALIGN) < 0) {
> + free(memmap_p);
> return EFAILED;
> + }
.. see above ..
> }
> /* the size of the elf headers allocated is returned in 'bufsz' */
>
> @@ -962,8 +972,10 @@ int load_crashdump_segments(struct kexec_info *info, char* mod_cmdline,
> elfcorehdr = add_buffer(info, tmp, bufsz, memsz, align, min_base,
> max_addr, -1);
> dbgprintf("Created elf header segment at 0x%lx\n", elfcorehdr);
> - if (delete_memmap(memmap_p, &nr_memmap, elfcorehdr, memsz) < 0)
> + if (delete_memmap(memmap_p, &nr_memmap, elfcorehdr, memsz) < 0) {
> + free(memmap_p);
> return -1;
> + }
> if (!bzImage_support_efi_boot || arch_options.noefi ||
> !sysfs_efi_runtime_map_exist())
> cmdline_add_efi(mod_cmdline);
> diff --git a/kexec/arch/mips/crashdump-mips.c b/kexec/arch/mips/crashdump-mips.c
> index fc92e64..c1603d9 100644
> --- a/kexec/arch/mips/crashdump-mips.c
> +++ b/kexec/arch/mips/crashdump-mips.c
> @@ -387,8 +387,11 @@ int load_crashdump_segments(struct kexec_info *info, char* mod_cmdline,
> crash_reserved_mem.end, -1);
>
> if (crash_create(info, elf_info, crash_memory_range, nr_ranges,
> - &tmp, &sz, ELF_CORE_HEADER_ALIGN) < 0)
> + &tmp, &sz, ELF_CORE_HEADER_ALIGN) < 0) {
> + free(tmp);
> return -1;
> + }
> +
> elfcorehdr = add_buffer(info, tmp, sz, sz, align,
> crash_reserved_mem.start,
> crash_reserved_mem.end, -1);
> diff --git a/kexec/arch/ppc64/crashdump-ppc64.c b/kexec/arch/ppc64/crashdump-ppc64.c
> index b2787d5..26f9a01 100644
> --- a/kexec/arch/ppc64/crashdump-ppc64.c
> +++ b/kexec/arch/ppc64/crashdump-ppc64.c
> @@ -535,15 +535,19 @@ int load_crashdump_segments(struct kexec_info *info, char* mod_cmdline,
> if (crash_create_elf64_headers(info, &elf_info64,
> crash_memory_range, nr_ranges,
> &tmp, &sz,
> - ELF_CORE_HEADER_ALIGN) < 0)
> + ELF_CORE_HEADER_ALIGN) < 0) {
> + free (tmp);
> return -1;
> + }
> }
> else {
> if (crash_create_elf32_headers(info, &elf_info32,
> crash_memory_range, nr_ranges,
> &tmp, &sz,
> - ELF_CORE_HEADER_ALIGN) < 0)
> + ELF_CORE_HEADER_ALIGN) < 0) {
> + free(tmp);
> return -1;
> + }
> }
... see above ...
>
> elfcorehdr = add_buffer(info, tmp, sz, sz, align, min_base,
> --
> 2.1.0
Thanks,
Bhupesh
More information about the kexec
mailing list