Re: [PATCH v2 1/7 for-5.2?] net: Fix memory leak on error

[Eric Blake]

On 11/16/20 8:22 AM, Markus Armbruster wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
>> If qmp_query_rx_filter() encounters an error on a second iteration, it
>> leaks the memory from the first.
>>
>> Fixes: 9083da1d4c
>> Signed-off-by: Eric Blake <eblake@redhat.com>
>> ---
>>  net/net.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/net/net.c b/net/net.c
>> index 794c652282cb..eb65e110871a 100644
>> --- a/net/net.c
>> +++ b/net/net.c
>> @@ -1213,6 +1213,7 @@ RxFilterInfoList *qmp_query_rx_filter(bool has_name, 
>> const char *name,
>        NetClientState *nc;
>        RxFilterInfoList *filter_list = NULL, *last_entry = NULL;
> 
>        QTAILQ_FOREACH(nc, &net_clients, next) {
>            RxFilterInfoList *entry;
>            RxFilterInfo *info;
> 
>            if (has_name && strcmp(nc->name, name) != 0) {
>                continue;
>            }
> 
> If @has_name and we get here more than once, then multiple @net_clients
> have the same name.  How can that be?  We are not supposed to return
> multiple replies with the same @name, are we?

Oh, good spot. I was going off of the fact that the loop had an early
exit, but did not further note that early exit is only possible in the
case where the bulk of the loop executes exactly once.

> 
>            /* only query rx-filter information of NIC */
>>          if (nc->info->type != NET_CLIENT_DRIVER_NIC) {
>>              if (has_name) {
>>                  error_setg(errp, "net client(%s) isn't a NIC", name);
>> +                qapi_free_RxFilterInfoList(filter_list);
> 
> Unless multiple @net_clients are named @name, @filter_list is null,
> isn't it?

Correct.

> 
>>                  return NULL;
>>              }
>>              continue;
>            }
> 
>            /* only query information on queue 0 since the info is per nic,
>             * not per queue
>             */
>            if (nc->queue_index != 0)
>                continue;
> 
>            if (nc->info->query_rx_filter) {
>                info = nc->info->query_rx_filter(nc);
>                entry = g_malloc0(sizeof(*entry));
>                entry->value = info;
> 
>                if (!filter_list) {
>                    filter_list = entry;
> 
>>From now on, we must either return or free @filter_list.
> 
>                } else {
>                    last_entry->next = entry;
>                }
>                last_entry = entry;
>> @@ -1238,6 +1239,7 @@ RxFilterInfoList *qmp_query_rx_filter(bool has_name, 
>> const char *name,
>>          } else if (has_name) {
>>              error_setg(errp, "net client(%s) doesn't support"
>>                         " rx-filter querying", name);
>> +            qapi_free_RxFilterInfoList(filter_list);
> 
> Unless multiple @net_clients are named @name, @filter_list is null,
> isn't it?
> 
>>              return NULL;
>>          }
> 
>            if (has_name) {
>                break;
>            }
>        }
> 
> I dislike this loop.

No joke.  But you've confirmed that this patch is unnecessary; and the
rest of my series is 6.0 material, so there's nothing left to worry
about for 5.2 from this series.

> 
>        if (filter_list == NULL && has_name) {
>            error_setg(errp, "invalid net client name: %s", name);
>        }
> 
>        return filter_list;
> 
> I should've strangled the optional @name parameter in the crib.
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org