[libvirt] [PATCH] qemu: monitor: Fix a memory leak in qemuMonitorJSONAttachCharDevCommand

[Erik Skultety]

On Tue, Jun 13, 2017 at 06:17:55PM +0200, Pavel Hrdina wrote:
> On Tue, Jun 13, 2017 at 06:01:17PM +0200, Erik Skultety wrote:
> > With the current logic, we only free @tlsalias as part of the error
> > label and would have to free it explicitly earlier in the code. Convert
> > the error label to cleanup, so that we have only one sink, where we
> > handle all frees. In order to do that we need to clear some JSON obj
> > pointers down the success road to avoid SIGSEGV, since JSON object
> > append operation consumes pointers.
> >
> > Signed-off-by: Erik Skultety <eskultet at redhat.com>
> > ---
> >  src/qemu/qemu_monitor_json.c | 47 ++++++++++++++++++++++----------------------
> >  1 file changed, 23 insertions(+), 24 deletions(-)
> >
> > diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
> > index f208dd05a..b8b73926f 100644
> > --- a/src/qemu/qemu_monitor_json.c
> > +++ b/src/qemu/qemu_monitor_json.c
> > @@ -6430,8 +6430,8 @@ static virJSONValuePtr
> >  qemuMonitorJSONAttachCharDevCommand(const char *chrID,
> >                                      const virDomainChrSourceDef *chr)
> >  {
> > -    virJSONValuePtr ret;
> > -    virJSONValuePtr backend;
> > +    virJSONValuePtr ret = NULL;
> > +    virJSONValuePtr backend = NULL;
> >      virJSONValuePtr data = NULL;
> >      virJSONValuePtr addr = NULL;
> >      const char *backend_type = NULL;
> > @@ -6440,7 +6440,7 @@ qemuMonitorJSONAttachCharDevCommand(const char *chrID,
> >
> >      if (!(backend = virJSONValueNewObject()) ||
> >          !(data = virJSONValueNewObject())) {
> > -        goto error;
> > +        goto cleanup;
> >      }
> >
> >      switch ((virDomainChrType) chr->type) {
> > @@ -6456,14 +6456,14 @@ qemuMonitorJSONAttachCharDevCommand(const char *chrID,
> >      case VIR_DOMAIN_CHR_TYPE_FILE:
> >          backend_type = "file";
> >          if (virJSONValueObjectAppendString(data, "out", chr->data.file.path) < 0)
> > -            goto error;
> > +            goto cleanup;
> >          break;
> >
> >      case VIR_DOMAIN_CHR_TYPE_DEV:
> >          backend_type = STRPREFIX(chrID, "parallel") ? "parallel" : "serial";
> >          if (virJSONValueObjectAppendString(data, "device",
> >                                             chr->data.file.path) < 0)
> > -            goto error;
> > +            goto cleanup;
> >          break;
> >
> >      case VIR_DOMAIN_CHR_TYPE_TCP:
> > @@ -6472,21 +6472,20 @@ qemuMonitorJSONAttachCharDevCommand(const char *chrID,
> >                                                       chr->data.tcp.service);
> >          if (!addr ||
> >              virJSONValueObjectAppend(data, "addr", addr) < 0)
> > -            goto error;
> > -        addr = NULL;
> > +            goto cleanup;
> >
> >          telnet = chr->data.tcp.protocol == VIR_DOMAIN_CHR_TCP_PROTOCOL_TELNET;
> >
> >          if (virJSONValueObjectAppendBoolean(data, "wait", false) < 0 ||
> >              virJSONValueObjectAppendBoolean(data, "telnet", telnet) < 0 ||
> >              virJSONValueObjectAppendBoolean(data, "server", chr->data.tcp.listen) < 0)
> > -            goto error;
> > +            goto cleanup;
> >          if (chr->data.tcp.tlscreds) {
> >              if (!(tlsalias = qemuAliasTLSObjFromSrcAlias(chrID)))
> > -                goto error;
> > +                goto cleanup;
> >
> >              if (virJSONValueObjectAppendString(data, "tls-creds", tlsalias) < 0)
> > -                goto error;
> > +                goto cleanup;
> >          }
> >          break;
> >
> > @@ -6496,16 +6495,15 @@ qemuMonitorJSONAttachCharDevCommand(const char *chrID,
> >                                                       chr->data.udp.connectService);
> >          if (!addr ||
> >              virJSONValueObjectAppend(data, "remote", addr) < 0)
> > -            goto error;
> > +            goto cleanup;
> >
> >          if (chr->data.udp.bindHost) {
> >              addr = qemuMonitorJSONBuildInetSocketAddress(chr->data.udp.bindHost,
> >                                                           chr->data.udp.bindService);
> >              if (!addr ||
> >                  virJSONValueObjectAppend(data, "local", addr) < 0)
> > -                goto error;
> > +                goto cleanup;
> >          }
> > -        addr = NULL;
> >          break;
> >
> >      case VIR_DOMAIN_CHR_TYPE_UNIX:
> > @@ -6514,12 +6512,11 @@ qemuMonitorJSONAttachCharDevCommand(const char *chrID,
> >
> >          if (!addr ||
> >              virJSONValueObjectAppend(data, "addr", addr) < 0)
> > -            goto error;
> > -        addr = NULL;
> > +            goto cleanup;
> >
> >          if (virJSONValueObjectAppendBoolean(data, "wait", false) < 0 ||
> >              virJSONValueObjectAppendBoolean(data, "server", chr->data.nix.listen) < 0)
> > -            goto error;
> > +            goto cleanup;
> >          break;
> >
> >      case VIR_DOMAIN_CHR_TYPE_SPICEVMC:
> > @@ -6527,7 +6524,7 @@ qemuMonitorJSONAttachCharDevCommand(const char *chrID,
> >
> >          if (virJSONValueObjectAppendString(data, "type",
> >                                             virDomainChrSpicevmcTypeToString(chr->data.spicevmc)) < 0)
> > -            goto error;
> > +            goto cleanup;
> >          break;
> >
> >      case VIR_DOMAIN_CHR_TYPE_SPICEPORT:
> > @@ -6544,28 +6541,30 @@ qemuMonitorJSONAttachCharDevCommand(const char *chrID,
> >                             _("Hotplug unsupported for char device type '%d'"),
> >                             chr->type);
> >          }
> > -        goto error;
> > +        goto cleanup;
> >      }
> >
> >      if (virJSONValueObjectAppendString(backend, "type", backend_type) < 0 ||
> >          virJSONValueObjectAppend(backend, "data", data) < 0)
> > -        goto error;
> > -    data = NULL;
> > +        goto cleanup;
> >
> >      if (!(ret = qemuMonitorJSONMakeCommand("chardev-add",
> >                                             "s:id", chrID,
> >                                             "a:backend", backend,
> >                                             NULL)))
> > -        goto error;
> > +        goto cleanup;
> >
> > -    return ret;
> > +    /* we must not free the following pointers as they've been collectively
> > +     * consumed by @ret, so clear them first
> > +     */
> > +    addr = data = backend = NULL;
>
> Eww, this is not a good idea, just leave the clearing at the original
Since you didn't elaborate more on ^^why, I'll just add it here for reference -
it might happen that @addr or @data would be both freed --> double free, so I'll
drop this hunk in v2 and clear only @backend as suggested.

Erik

> place.  You should clear only @backend here.
>
> Pavel