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@xxxxxxxxxx>
> > ---
> > 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
