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

[Chen Gang]

If this patch need still be improvement (e.g. need let dump_cleanup
function as a generic one, or other cases), please let me know, and I
shall send patch v2 for it.

Thanks.

On 08/04/2014 09:51 PM, Chen Gang wrote:
> 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