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
place. You should clear only @backend here.
Pavel
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
