On Mon, Oct 11, 2021 at 04:03:01PM +0100, Dr. David Alan Gilbert wrote:
> * Stefan Reiter (s.rei...@proxmox.com) wrote:
> > It is possible to specify more than one VNC server on the command line,
> > either with an explicit ID or the auto-generated ones à la "default",
> > "vnc2", "vnc3", ...
> >
> > +++ b/monitor/hmp-cmds.c
> > @@ -1451,10 +1451,41 @@ void hmp_set_password(Monitor *mon, const QDict
> > *qdict)
> > {
> > const char *protocol = qdict_get_str(qdict, "protocol");
> > const char *password = qdict_get_str(qdict, "password");
> > + const char *display = qdict_get_try_str(qdict, "display");
> > const char *connected = qdict_get_try_str(qdict, "connected");
> > Error *err = NULL;
> > + DisplayProtocol proto;
> >
> > - qmp_set_password(protocol, password, !!connected, connected, &err);
> > + SetPasswordOptions opts = {
> > + .password = g_strdup(password),
>
> You're leaking that strdup on the error returns; you probably want to
> add an error: exit and goto it to do all the cleanup.
Or maybe there's a way to use g_autofree to let the compiler clean it
up automatically.
>
> > + .u.vnc.display = NULL,
> > + };
> > +
> > + proto = qapi_enum_parse(&DisplayProtocol_lookup, protocol,
> > + DISPLAY_PROTOCOL_VNC, &err);
> > + if (err) {
> > + hmp_handle_error(mon, err);
> > + return;
> > + }
> > + opts.protocol = proto;
> > +
> > + if (proto == DISPLAY_PROTOCOL_VNC) {
> > + opts.u.vnc.has_display = !!display;
> > + opts.u.vnc.display = g_strdup(display);
> > + } else if (proto == DISPLAY_PROTOCOL_SPICE) {
> > + opts.u.spice.has_connected = !!connected;
> > + opts.u.spice.connected =
> > + qapi_enum_parse(&SetPasswordAction_lookup, connected,
> > + SET_PASSWORD_ACTION_KEEP, &err);
> > + if (err) {
> > + hmp_handle_error(mon, err);
> > + return;
> > + }
> > + }
> > +
> > + qmp_set_password(&opts, &err);
> > + g_free(opts.password);
> > + g_free(opts.u.vnc.display);
Hmm. Why are we hand-cleaning only portions of the QAPI type instead
of using the generated qapi_free_SetPasswordOptions() do to things?
> > hmp_handle_error(mon, err);
> > }
> >
> > @@ -1462,9 +1493,31 @@ void hmp_expire_password(Monitor *mon, const QDict
> > *qdict)
> > {
> > const char *protocol = qdict_get_str(qdict, "protocol");
> > const char *whenstr = qdict_get_str(qdict, "time");
> > + const char *display = qdict_get_try_str(qdict, "display");
> > Error *err = NULL;
> > + DisplayProtocol proto;
> >
> > - qmp_expire_password(protocol, whenstr, &err);
> > + ExpirePasswordOptions opts = {
> > + .time = g_strdup(whenstr),
> > + .u.vnc.display = NULL,
> > + };
>
> Same here; that 'whenstr' gets leaked on errors.
>
> > + proto = qapi_enum_parse(&DisplayProtocol_lookup, protocol,
> > + DISPLAY_PROTOCOL_VNC, &err);
> > + if (err) {
> > + hmp_handle_error(mon, err);
> > + return;
> > + }
> > + opts.protocol = proto;
> > +
> > + if (proto == DISPLAY_PROTOCOL_VNC) {
> > + opts.u.vnc.has_display = !!display;
> > + opts.u.vnc.display = g_strdup(display);
> > + }
> > +
> > + qmp_expire_password(&opts, &err);
> > + g_free(opts.time);
> > + g_free(opts.u.vnc.display);
> > hmp_handle_error(mon, err);
Same here - using the generated qapi_free_ function rather than doing
things by hand, and/or smart use of g_autofree, may make this easier
to maintain.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
