Message ID | 1317193022-13504-2-git-send-email-ajia@redhat.com |
---|---|
State | New |
Headers | show |
On 28 September 2011 07:57, <ajia@redhat.com> wrote: > From: Alex Jia <ajia@redhat.com> > > Haven't released memory of 'array' and 'host_mb' in failure paths. > > Signed-off-by: Alex Jia <ajia@redhat.com> > --- > 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. > @@ -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. -- PMM
On 09/28/2011 03:55 PM, Peter Maydell wrote: > On 28 September 2011 07:57,<ajia@redhat.com> wrote: >> From: Alex Jia<ajia@redhat.com> >> >> Haven't released memory of 'array' and 'host_mb' in failure paths. >> >> Signed-off-by: Alex Jia<ajia@redhat.com> >> --- >> 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
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); @@ -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;