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

[Markus Armbruster]

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?

           /* 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?

>                  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.

       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.