Re: [Qemu-devel] [PULL 2/3] hmp: Update info vnc

[Peter Maydell]

On Mon, 17 Jul 2017 at 10:40, Gerd Hoffmann <address@hidden> wrote:
>
> From: "Dr. David Alan Gilbert" <address@hidden>
>
> The QMP query-vnc interfaces have gained a lot more information that
> the HMP interfaces hasn't got yet. Update it.
>
> Note the output format has changed, but this is HMP so that's OK.

Hi; another "ancient change Coverity has only just noticed has
a problem" email :-)   This is CID 1421932. It looks like any
"info vnc" will leak memory if there are any VNC servers to
display info about...

>  void hmp_info_vnc(Monitor *mon, const QDict *qdict)
>  {
> -    VncInfo *info;
> +    VncInfo2List *info2l;
>      Error *err = NULL;
> -    VncClientInfoList *client;
>
> -    info = qmp_query_vnc(&err);
> +    info2l = qmp_query_vnc_servers(&err);

Here we get a list of VNC servers, which is allocated memory...

>      if (err) {
>          error_report_err(err);
>          return;
>      }
> -
> -    if (!info->enabled) {
> -        monitor_printf(mon, "Server: disabled\n");
> -        goto out;
> -    }
> -
> -    monitor_printf(mon, "Server:\n");
> -    if (info->has_host && info->has_service) {
> -        monitor_printf(mon, "     address: %s:%s\n", info->host, 
> info->service);
> -    }
> -    if (info->has_auth) {
> -        monitor_printf(mon, "        auth: %s\n", info->auth);
> +    if (!info2l) {
> +        monitor_printf(mon, "None\n");
> +        return;
>      }
>
> -    if (!info->has_clients || info->clients == NULL) {
> -        monitor_printf(mon, "Client: none\n");
> -    } else {
> -        for (client = info->clients; client; client = client->next) {
> -            monitor_printf(mon, "Client:\n");
> -            monitor_printf(mon, "     address: %s:%s\n",
> -                           client->value->host,
> -                           client->value->service);
> -            monitor_printf(mon, "  x509_dname: %s\n",
> -                           client->value->x509_dname ?
> -                           client->value->x509_dname : "none");
> -            monitor_printf(mon, "    username: %s\n",
> -                           client->value->has_sasl_username ?
> -                           client->value->sasl_username : "none");
> +    while (info2l) {
> +        VncInfo2 *info = info2l->value;
> +        monitor_printf(mon, "%s:\n", info->id);
> +        hmp_info_vnc_servers(mon, info->server);
> +        hmp_info_vnc_clients(mon, info->clients);
> +        if (!info->server) {
> +            /* The server entry displays its auth, we only
> +             * need to display in the case of 'reverse' connections
> +             * where there's no server.
> +             */
> +            hmp_info_vnc_authcrypt(mon, "  ", info->auth,
> +                               info->has_vencrypt ? &info->vencrypt : NULL);
> +        }
> +        if (info->has_display) {
> +            monitor_printf(mon, "  Display: %s\n", info->display);
>          }
> +        info2l = info2l->next;

...but the loop iteration here updates 'info2l' as it goes along...

>      }
>
> -out:
> -    qapi_free_VncInfo(info);
> +    qapi_free_VncInfo2List(info2l);

...so here we end up passing NULL to qapi_free_VncInfo2List(),
which will do nothing, leaking the whole list.

Would somebody like to send a patch?

thanks
-- PMM