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,<a...@redhat.com>  wrote:

From: Alex Jia<a...@redhat.com>

Haven't released memory of 'array' and 'host_mb' in failure paths.

Signed-off-by: Alex Jia<a...@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