* Peter Maydell (peter.mayd...@linaro.org) wrote:
> On Mon, 17 Jul 2017 at 10:40, Gerd Hoffmann <kra...@redhat.com> wrote:
> >
> > From: "Dr. David Alan Gilbert" <dgilb...@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 / dgilb...@redhat.com / Manchester, UK
