Re: [Qemu-devel] [PATCH] linux-user: fix memory leak in failure path

[Alex Jia]

On 09/28/2011 03:55 PM, Peter Maydell wrote:
> On 28 September 2011 07:57,<address@hidden>  wrote:
> > From: Alex Jia<address@hidden>
> >
> > Haven't released memory of 'array' and 'host_mb' in failure paths.
> >
> > Signed-off-by: Alex Jia<address@hidden>
> > ---
> >   linux-user/syscall.c |    6 ++++--
> >   1 files changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> > index 7735008..922c2a0 100644
> > --- a/linux-user/syscall.c
> > +++ b/linux-user/syscall.c
> > @@ -2523,8 +2523,10 @@ static inline abi_long do_semctl(int semid, int semnum, 
> > int cmd,
> >         case GETALL:
> >         case SETALL:
> >              err = target_to_host_semarray(semid,&array, target_su.array);
> > -            if (err)
> > +            if (err) {
> > +                free(array);
> >                  return err;
> > +            }
> >              arg.array = array;
> >              ret = get_errno(semctl(semid, semnum, cmd, arg));
> >              err = host_to_target_semarray(semid, target_su.array,&array);
> This is the wrong place to try to fix this. If target_to_host_semarray
> fails it should free() the buffer it malloc()ed itself, not rely on
> its caller to do the cleanup.
Yeah, caller shouldn't do this.
> > @@ -2779,9 +2781,9 @@ static inline abi_long do_msgrcv(int msqid, abi_long msgp,
> >      }
> >
> >      target_mb->mtype = tswapl(host_mb->mtype);
> > -    free(host_mb);
> >
> >   end:
> > +    free(host_mb);
> >      if (target_mb)
> >          unlock_user_struct(target_mb, msgp, 1);
> >      return ret;
> This change is OK.
>
> Also I note that target_to_host_semarray is doing a plain malloc()
> and not checking the return value. You should fix that while you're
> doing fixes in this area.
Yeah, for return value check of malloc(), it seems many places haven't
do it.

Thanks,
Alex
> -- PMM