[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2] elfload: use g_new/g_malloc and g_autofree
From: |
Markus Armbruster |
Subject: |
Re: [PATCH v2] elfload: use g_new/g_malloc and g_autofree |
Date: |
Mon, 05 Oct 2020 09:57:27 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/27.1 (gnu/linux) |
Please don't post respins as replies, because our tooling will miss them
there, and even humans may. Start a new thread instead. Next time :)
Elena Afanasova <eafanasova@gmail.com> writes:
> Subject: [PATCH v2] elfload: use g_new/g_malloc and g_autofree
>
> Signed-off-by: Elena Afanasova <eafanasova@gmail.com>
> ---
> bsd-user/elfload.c | 79 ++++++++--------------------------------------
> 1 file changed, 14 insertions(+), 65 deletions(-)
>
> diff --git a/bsd-user/elfload.c b/bsd-user/elfload.c
> index 32378af7b2..bc4be4c874 100644
> --- a/bsd-user/elfload.c
> +++ b/bsd-user/elfload.c
> @@ -838,7 +838,7 @@ static abi_ulong load_elf_interp(struct elfhdr *
> interp_elf_ex,
> int interpreter_fd,
> abi_ulong *interp_load_addr)
> {
> - struct elf_phdr *elf_phdata = NULL;
> + g_autofree struct elf_phdr *elf_phdata = NULL;
> struct elf_phdr *eppnt;
> abi_ulong load_addr = 0;
> int load_addr_set = 0;
> @@ -867,18 +867,13 @@ static abi_ulong load_elf_interp(struct elfhdr *
> interp_elf_ex,
> if (sizeof(struct elf_phdr) * interp_elf_ex->e_phnum >
> TARGET_PAGE_SIZE)
> return ~(abi_ulong)0UL;
>
> - elf_phdata = (struct elf_phdr *)
> - malloc(sizeof(struct elf_phdr) * interp_elf_ex->e_phnum);
> -
> - if (!elf_phdata)
> - return ~((abi_ulong)0UL);
> + elf_phdata = g_new(struct elf_phdr, interp_elf_ex->e_phnum);
>
> /*
> * If the size of this structure has changed, then punt, since
> * we will be doing the wrong thing.
> */
> if (interp_elf_ex->e_phentsize != sizeof(struct elf_phdr)) {
> - free(elf_phdata);
> return ~((abi_ulong)0UL);
> }
>
> @@ -891,7 +886,6 @@ static abi_ulong load_elf_interp(struct elfhdr *
> interp_elf_ex,
> if (retval < 0) {
> perror("load_elf_interp");
> exit(-1);
> - free (elf_phdata);
> return retval;
> }
> #ifdef BSWAP_NEEDED
> @@ -940,7 +934,6 @@ static abi_ulong load_elf_interp(struct elfhdr *
> interp_elf_ex,
> if (error == -1) {
> /* Real error */
> close(interpreter_fd);
> - free(elf_phdata);
> return ~((abi_ulong)0UL);
> }
>
> @@ -983,7 +976,6 @@ static abi_ulong load_elf_interp(struct elfhdr *
> interp_elf_ex,
> PROT_READ|PROT_WRITE|PROT_EXEC,
> MAP_FIXED|MAP_PRIVATE|MAP_ANON, -1, 0);
> }
> - free(elf_phdata);
>
> *interp_load_addr = load_addr;
> return ((abi_ulong) interp_elf_ex->e_entry) + load_addr;
> @@ -1036,9 +1028,10 @@ static void load_symbols(struct elfhdr *hdr, int fd)
> {
> unsigned int i, nsyms;
> struct elf_shdr sechdr, symtab, strtab;
> - char *strings;
> - struct syminfo *s;
> - struct elf_sym *syms, *new_syms;
> + g_autofree char *strings = NULL;
> + g_autofree struct syminfo *s = NULL;
> + g_autofree struct elf_sym *syms = NULL;
> + struct elf_sym *new_syms;
Note for later: @strings, @syminfo and @syms are freed on return.
>
> lseek(fd, hdr->e_shoff, SEEK_SET);
> for (i = 0; i < hdr->e_shnum; i++) {
> @@ -1064,24 +1057,12 @@ static void load_symbols(struct elfhdr *hdr, int fd)
>
> found:
> /* Now know where the strtab and symtab are. Snarf them. */
> - s = malloc(sizeof(*s));
> - syms = malloc(symtab.sh_size);
> - if (!syms) {
> - free(s);
> - return;
> - }
> - s->disas_strtab = strings = malloc(strtab.sh_size);
> - if (!s->disas_strtab) {
> - free(s);
> - free(syms);
> - return;
> - }
> + s = g_new(struct syminfo, 1);
The smaller change would be
s = g_malloc(sizeof(*s));
Matter of taste.
> + syms = g_malloc(symtab.sh_size);
> + s->disas_strtab = strings = g_new(char, strtab.sh_size);
I'd prefer the simpler
s->disas_strtab = strings = g_malloc(strtab.sh_size);
Yes, it returns void * whereas g_new() returns char *, but the char * is
vanishingly unlikely to catch actual bugs.
>
> lseek(fd, symtab.sh_offset, SEEK_SET);
> if (read(fd, syms, symtab.sh_size) != symtab.sh_size) {
> - free(s);
> - free(syms);
> - free(strings);
> return;
> }
>
> @@ -1113,22 +1094,13 @@ static void load_symbols(struct elfhdr *hdr, int fd)
> that we threw away. Whether or not this has any effect on the
> memory allocation depends on the malloc implementation and how
> many symbols we managed to discard. */
> - new_syms = realloc(syms, nsyms * sizeof(*syms));
> - if (new_syms == NULL) {
> - free(s);
> - free(syms);
> - free(strings);
> - return;
> - }
> + new_syms = g_realloc(syms, nsyms * sizeof(*syms));
> syms = new_syms;
>
> qsort(syms, nsyms, sizeof(*syms), symcmp);
>
> lseek(fd, strtab.sh_offset, SEEK_SET);
> if (read(fd, strings, strtab.sh_size) != strtab.sh_size) {
> - free(s);
> - free(syms);
> - free(strings);
> return;
> }
> s->disas_num_syms = nsyms;
#if ELF_CLASS == ELFCLASS32
s->disas_symtab.elf32 = syms;
s->lookup_symbol = (lookup_symbol_t)lookup_symbolxx;
#else
s->disas_symtab.elf64 = syms;
s->lookup_symbol = (lookup_symbol_t)lookup_symbolxx;
#endif
s->next = syminfos;
syminfos = s;
@strings, @syminfo and @syms are freed here. s->disas_strtab,
s->disas_symtab.elf{32,64}, s->next become dangling pointers.
I'd expect this to blow up. Have you tested?
}
> @@ -1156,10 +1128,10 @@ int load_elf_binary(struct linux_binprm * bprm,
> struct target_pt_regs * regs,
> unsigned char ibcs2_interpreter;
> int i;
> struct elf_phdr * elf_ppnt;
> - struct elf_phdr *elf_phdata;
> + g_autofree struct elf_phdr *elf_phdata = NULL;
> abi_ulong elf_bss, k, elf_brk;
> int retval;
> - char * elf_interpreter;
> + g_autofree char *elf_interpreter = NULL;
> abi_ulong elf_entry, interp_load_addr = 0;
> abi_ulong start_code, end_code, start_data, end_data;
> abi_ulong reloc_func_desc = 0;
> @@ -1190,10 +1162,7 @@ int load_elf_binary(struct linux_binprm * bprm, struct
> target_pt_regs * regs,
> }
>
> /* Now read in all of the header information */
> - elf_phdata = (struct elf_phdr
> *)malloc(elf_ex.e_phentsize*elf_ex.e_phnum);
> - if (elf_phdata == NULL) {
> - return -ENOMEM;
> - }
> + elf_phdata = g_new(struct elf_phdr, elf_ex.e_phnum);
This assumes elf_ex.e_phentsize == sizeof(struct elf_phdr). Why is that
true?
>
> retval = lseek(bprm->fd, elf_ex.e_phoff, SEEK_SET);
> if(retval > 0) {
> @@ -1204,7 +1173,6 @@ int load_elf_binary(struct linux_binprm * bprm, struct
> target_pt_regs * regs,
> if (retval < 0) {
> perror("load_elf_binary");
> exit(-1);
> - free (elf_phdata);
> return -errno;
> }
>
> @@ -1220,7 +1188,6 @@ int load_elf_binary(struct linux_binprm * bprm, struct
> target_pt_regs * regs,
> elf_brk = 0;
>
>
> - elf_interpreter = NULL;
> start_code = ~((abi_ulong)0UL);
> end_code = 0;
> start_data = 0;
> @@ -1231,8 +1198,6 @@ int load_elf_binary(struct linux_binprm * bprm, struct
> target_pt_regs * regs,
> if (elf_ppnt->p_type == PT_INTERP) {
> if ( elf_interpreter != NULL )
> {
> - free (elf_phdata);
> - free(elf_interpreter);
> close(bprm->fd);
> return -EINVAL;
> }
> @@ -1242,13 +1207,7 @@ int load_elf_binary(struct linux_binprm * bprm, struct
> target_pt_regs * regs,
> * is an a.out format binary
> */
>
> - elf_interpreter = (char *)malloc(elf_ppnt->p_filesz);
> -
> - if (elf_interpreter == NULL) {
> - free (elf_phdata);
> - close(bprm->fd);
> - return -ENOMEM;
> - }
> + elf_interpreter = g_new(char, elf_ppnt->p_filesz);
Again, I'd prefer the simpler
elf_interpreter = g_malloc(elf_ppnt->p_filesz);
>
> retval = lseek(bprm->fd, elf_ppnt->p_offset, SEEK_SET);
> if(retval >= 0) {
> @@ -1298,8 +1257,6 @@ int load_elf_binary(struct linux_binprm * bprm, struct
> target_pt_regs * regs,
> if (retval < 0) {
> perror("load_elf_binary3");
> exit(-1);
> - free (elf_phdata);
> - free(elf_interpreter);
> close(bprm->fd);
> return retval;
> }
> @@ -1323,8 +1280,6 @@ int load_elf_binary(struct linux_binprm * bprm, struct
> target_pt_regs * regs,
> }
>
> if (!interpreter_type) {
> - free(elf_interpreter);
> - free(elf_phdata);
> close(bprm->fd);
> return -ELIBBAD;
> }
> @@ -1346,8 +1301,6 @@ int load_elf_binary(struct linux_binprm * bprm, struct
> target_pt_regs * regs,
> }
> }
> if (!bprm->p) {
> - free(elf_interpreter);
> - free (elf_phdata);
> close(bprm->fd);
> return -E2BIG;
> }
> @@ -1486,18 +1439,14 @@ int load_elf_binary(struct linux_binprm * bprm,
> struct target_pt_regs * regs,
> reloc_func_desc = interp_load_addr;
>
> close(interpreter_fd);
> - free(elf_interpreter);
>
> if (elf_entry == ~((abi_ulong)0UL)) {
> printf("Unable to load interpreter\n");
> - free(elf_phdata);
> exit(-1);
> return 0;
> }
> }
>
> - free(elf_phdata);
> -
> if (qemu_log_enabled())
> load_symbols(&elf_ex, bprm->fd);
- [PATCH] elfload: use g_new instead of malloc, Elena Afanasova, 2020/10/01
- Re: [PATCH] elfload: use g_new instead of malloc, Thomas Huth, 2020/10/01
- Re: [PATCH] elfload: use g_new instead of malloc, Markus Armbruster, 2020/10/02
- Re: [PATCH] elfload: use g_new instead of malloc, Thomas Huth, 2020/10/02
- Re: [PATCH] elfload: use g_new instead of malloc, Markus Armbruster, 2020/10/02
- Re: [PATCH] elfload: use g_new instead of malloc, Eric Blake, 2020/10/02
- Re: [PATCH v2] elfload: use g_new/g_malloc and g_autofree, Elena Afanasova, 2020/10/04
- Re: [PATCH v2] elfload: use g_new/g_malloc and g_autofree,
Markus Armbruster <=
- Re: [PATCH v2] elfload: use g_new/g_malloc and g_autofree, Peter Maydell, 2020/10/05
- Re: [PATCH v2] elfload: use g_new/g_malloc and g_autofree, Elena Afanasova, 2020/10/06