Re: [Qemu-trivial] [PATCH] dump.c: Fix memory leak issue in cleanup processing for dump_init()

[Chen Gang]

On 08/03/2014 11:56 PM, Laszlo Ersek wrote:
> comments below
> 

Excuse me for replying late, firstly.

> On 08/03/14 17:28, Chen Gang wrote:
>> In dump_init(), when failure occurs, need notice about 'fd' and memory
>> mapping. So call dump_cleanup() for it (need let all initializations at
>> front).
>>
>> Also simplify dump_cleanup(): remove redundant 'ret' and redundant 'fd'
>> checking.
>>
>> Signed-off-by: Chen Gang <address@hidden>
>> ---
>>  dump.c | 18 +++++-------------
>>  1 file changed, 5 insertions(+), 13 deletions(-)
> 
> Please explain what is leaked and how.
> 

Oh, sorry for the title misleading, need change to "Fix resource leak"
instead of "Fix memory leak".

> The only possibility I can see (without digging very hard) is that
> qemu_get_guest_memory_mapping() succeeds and lzo_init() fails (which
> should never happen in practice).
>

Yeah, what you said sounds reasonable to me.
 
> Regarding s->fd itself, I'm beyond trying to understand its lifecycle.
> Qemu uses a bad ownership model wherein functions, in case of an
> internal error, release resources they got from their callers. I'm
> unable to reason in such a model.

Yeah, what you said sounds reasonable to me.

>                                   The only function to close fd *ever*
> should be qmp_dump_guest_memory() (and that one should close fd with a
> direct close() call). Currently fd is basically a global variable,
> because the entire dump function tree has access to it (and closes it if
> there's an error).
> 

Although 's->fd' seems a global variable, but it is only have effect
within this file. It starts from qemu_open() or monitor_get_fd() in
qmp_dump_guest_memory(), and also end in qmp_dump_guest_memory().

dump_cleanup() is a static function, and qmp_dump_guest_memory() is
the only extern function which related with dump_cleanup() (I assume
'dump.c' will not be included directly by other C files).


> Anyway I guess it's OK to call dump_cleanup() to close s->fd just in case.
> 
> If you have a Coverity report, please share it.
> 

Excuse me, I only found it by reading source code (vi, grep, find ...), no
other additional tools.

> Then,
> 
>> diff --git a/dump.c b/dump.c
>> index ce646bc..71d3e94 100644
>> --- a/dump.c
>> +++ b/dump.c
>> @@ -71,18 +71,14 @@ uint64_t cpu_to_dump64(DumpState *s, uint64_t val)
>>  
>>  static int dump_cleanup(DumpState *s)
>>  {
>> -    int ret = 0;
>> -
> 
> I agree with this change.
> 

Thanks.

>>      guest_phys_blocks_free(&s->guest_phys_blocks);
>>      memory_mapping_list_free(&s->list);
>> -    if (s->fd != -1) {
>> -        close(s->fd);
>> -    }
>> +    close(s->fd);
> 
> I disagree. It clobbers errno if s->fd is -1. Even though we don't
> particularly care about errno, it sort of disturbs be. Or can you prove
> s->fd is never -1 here?
> 

In our case, s->fd must be valid, or will return with failure in
qmp_dump_guest_memory().

For dump_cleanup(), at present, it is only a static function for share
code which need assume many things (e.g. only can be called once), not
generic enough.

But for simplify thinking, for me, it will be OK to let it be a generic
function, e.g: ('guest_phys_blocks_free' and 'memory_mapping_list_free'
know about NULL)

diff --git a/dump.c b/dump.c
index ce646bc..3f1ec5b 100644
--- a/dump.c
+++ b/dump.c
@@ -71,18 +71,18 @@ uint64_t cpu_to_dump64(DumpState *s, uint64_t val)
 
 static int dump_cleanup(DumpState *s)
 {
-    int ret = 0;
-
     guest_phys_blocks_free(&s->guest_phys_blocks);
     memory_mapping_list_free(&s->list);
     if (s->fd != -1) {
         close(s->fd);
+        s->fd = -1;
     }
     if (s->resume) {
         vm_start();
+        s->resume = false;
     }
 
-    return ret;
+    return 0;
 }
 
 static void dump_error(DumpState *s, const char *reason)


>>      if (s->resume) {
>>          vm_start();
>>      }
>>  
>> -    return ret;
>> +    return 0;
>>  }
>>  
>>  static void dump_error(DumpState *s, const char *reason)
>> @@ -1499,6 +1495,8 @@ static int dump_init(DumpState *s, int fd, bool 
>> has_format,
>>      s->begin = begin;
>>      s->length = length;
>>  
>> +    memory_mapping_list_init(&s->list);
>> +
>>      guest_phys_blocks_init(&s->guest_phys_blocks);
>>      guest_phys_blocks_append(&s->guest_phys_blocks);
>>  
>> @@ -1526,7 +1524,6 @@ static int dump_init(DumpState *s, int fd, bool 
>> has_format,
>>      }
>>  
>>      /* get memory mapping */
>> -    memory_mapping_list_init(&s->list);
>>      if (paging) {
>>          qemu_get_guest_memory_mapping(&s->list, &s->guest_phys_blocks, 
>> &err);
>>          if (err != NULL) {
>> @@ -1622,12 +1619,7 @@ static int dump_init(DumpState *s, int fd, bool 
>> has_format,
>>      return 0;
>>  
>>  cleanup:
>> -    guest_phys_blocks_free(&s->guest_phys_blocks);
>> -
>> -    if (s->resume) {
>> -        vm_start();
>> -    }
>> -
>> +    dump_cleanup(s);
>>      return -1;
>>  }
>>  
>>
> 
> This code is ripe for a generic lifecycle tracking overhaul, but since
> my view of ownership tracking is marginal in the qemu developer
> community, I'm not motivated.
>
> NB: I'm not nacking your patch, just please explain it better.
> 

OK, I can understand, and still thank you for your checking.

 
> Thanks
> Laszlo
> 

Thanks.
-- 
Chen Gang

Open share and attitude like air water and life which God blessed