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@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