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

[Pavel Hrdina]

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
place.  You should clear only @backend here.

Pavel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20170613/03854cdd/attachment-0001.sig>