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

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Peter Maydell (peter.maydell@linaro.org) wrote:
> On Mon, 17 Jul 2017 at 10:40, Gerd Hoffmann <kraxel@redhat.com> wrote:
> >
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> >
> > 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?

Oops, yes I can look at that; I guess something along the lines of an
info2l_orig  and free that at the end.

Dave

> thanks
> -- PMM
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK