From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:47727) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1R8pUL-0006wB-5z for qemu-devel@nongnu.org; Wed, 28 Sep 2011 04:27:37 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1R8pUG-0005tL-FL for qemu-devel@nongnu.org; Wed, 28 Sep 2011 04:27:33 -0400 Received: from mx1.redhat.com ([209.132.183.28]:33450) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1R8pUF-0005tF-Vx for qemu-devel@nongnu.org; Wed, 28 Sep 2011 04:27:28 -0400 Message-ID: <4E82DA78.6030001@redhat.com> Date: Wed, 28 Sep 2011 16:27:36 +0800 From: Alex Jia MIME-Version: 1.0 References: <1317193022-13504-1-git-send-email-ajia@redhat.com> <1317193022-13504-2-git-send-email-ajia@redhat.com> In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] linux-user: fix memory leak in failure path List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: qemu-devel@nongnu.org On 09/28/2011 03:55 PM, Peter Maydell wrote: > On 28 September 2011 07:57, wrote: >> From: Alex Jia >> >> Haven't released memory of 'array' and 'host_mb' in failure paths. >> >> Signed-off-by: Alex Jia >> --- >> 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