Re: [PATCH] elfload: use g_new instead of malloc

[Markus Armbruster <armbru@redhat.com>]

Thomas Huth <thuth@redhat.com> writes:

> On 02/10/2020 07.05, Markus Armbruster wrote:
>> Elena Afanasova <eafanasova@gmail.com> writes:
>> 
>>> Signed-off-by: Elena Afanasova <eafanasova@gmail.com>
>>> ---
>>>  bsd-user/elfload.c | 92 +++++++++++++++-------------------------------
>>>  1 file changed, 30 insertions(+), 62 deletions(-)
>>>
>>> diff --git a/bsd-user/elfload.c b/bsd-user/elfload.c
>>> index 32378af7b2..e10ca54eb7 100644
>>> --- a/bsd-user/elfload.c
>>> +++ b/bsd-user/elfload.c
>>> @@ -867,18 +867,14 @@ 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);
>>> +            g_free(elf_phdata);
>>>              return ~((abi_ulong)0UL);
>>>          }
>>>  
>>> @@ -890,9 +886,8 @@ static abi_ulong load_elf_interp(struct elfhdr * interp_elf_ex,
>>>          }
>>>          if (retval < 0) {
>>>                  perror("load_elf_interp");
>>> +                g_free(elf_phdata);
>>>                  exit(-1);
>>> -                free (elf_phdata);
>>> -                return retval;
>> 
>> Deleting return looks wrong.
>
> Why? There is an exit(-1) right in front of it, so this is dead code...
> well, maybe that should be done in a separate patch, or at least
> mentioned in the patch description, though.

You're right; I missed the exit(-1).

Following the unpleasant odour spread by exit(-1)...  Aha, the
function's behavior on error is inconsistent: sometimes it returns zero,
sometimes it exits.

Since the problem is bigger than just one dead return, I recommend
leaving it to a separate patch, and keeping this one focused on g_new().