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

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(a)redhat.com&gt; > --- > 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