From 157e94e8a2f7d3e14060d833bd1519a83099eaa9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Lureau?= Date: Wed, 13 Jul 2016 02:39:13 +0200 Subject: [PATCH 1/9] numa: do not leak NumaOptions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In all cases, call qapi_free_NumaOptions(), by using a common ending block. Signed-off-by: Marc-André Lureau Reviewed-by: Eric Blake --- numa.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/numa.c b/numa.c index 72861713e5..6289f469bd 100644 --- a/numa.c +++ b/numa.c @@ -223,14 +223,14 @@ static int parse_numa(void *opaque, QemuOpts *opts, Error **errp) } if (err) { - goto error; + goto end; } switch (object->type) { case NUMA_OPTIONS_KIND_NODE: numa_node_parse(object->u.node.data, opts, &err); if (err) { - goto error; + goto end; } nb_numa_nodes++; break; @@ -238,13 +238,14 @@ static int parse_numa(void *opaque, QemuOpts *opts, Error **errp) abort(); } - return 0; - -error: - error_report_err(err); +end: qapi_free_NumaOptions(object); + if (err) { + error_report_err(err); + return -1; + } - return -1; + return 0; } static char *enumerate_cpus(unsigned long *cpus, int max_cpus) From 5b498459b441f4639fd4c39c35345637bfc6c16c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Lureau?= Date: Thu, 14 Jul 2016 03:21:37 +0200 Subject: [PATCH 2/9] char: free the tcp connection data when closing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Make sure the connection data got freed when closing the chardev, to avoid leaks. Introduce tcp_chr_free_connection() to clean all connection related data, and move some tcp_chr_close() clean-ups there. (while at it, set write_msgfds_num to 0 when clearing array in tcp_set_msgfds()) Signed-off-by: Marc-André Lureau Reviewed-by: Paolo Bonzini --- qemu-char.c | 56 ++++++++++++++++++++++++++++++++--------------------- 1 file changed, 34 insertions(+), 22 deletions(-) diff --git a/qemu-char.c b/qemu-char.c index 27f2dbbbb5..7c529b2b48 100644 --- a/qemu-char.c +++ b/qemu-char.c @@ -2763,6 +2763,7 @@ static int tcp_set_msgfds(CharDriverState *chr, int *fds, int num) /* clear old pending fd array */ g_free(s->write_msgfds); s->write_msgfds = NULL; + s->write_msgfds_num = 0; if (!s->connected || !qio_channel_has_feature(s->ioc, @@ -2843,6 +2844,35 @@ static GSource *tcp_chr_add_watch(CharDriverState *chr, GIOCondition cond) return qio_channel_create_watch(s->ioc, cond); } +static void tcp_chr_free_connection(CharDriverState *chr) +{ + TCPCharDriver *s = chr->opaque; + int i; + + if (!s->connected) { + return; + } + + if (s->read_msgfds_num) { + for (i = 0; i < s->read_msgfds_num; i++) { + close(s->read_msgfds[i]); + } + g_free(s->read_msgfds); + s->read_msgfds = NULL; + s->read_msgfds_num = 0; + } + + tcp_set_msgfds(chr, NULL, 0); + remove_fd_in_watch(chr); + object_unref(OBJECT(s->sioc)); + s->sioc = NULL; + object_unref(OBJECT(s->ioc)); + s->ioc = NULL; + g_free(chr->filename); + chr->filename = NULL; + s->connected = 0; +} + static void tcp_chr_disconnect(CharDriverState *chr) { TCPCharDriver *s = chr->opaque; @@ -2851,18 +2881,12 @@ static void tcp_chr_disconnect(CharDriverState *chr) return; } - s->connected = 0; + tcp_chr_free_connection(chr); + if (s->listen_ioc) { s->listen_tag = qio_channel_add_watch( QIO_CHANNEL(s->listen_ioc), G_IO_IN, tcp_chr_accept, chr, NULL); } - tcp_set_msgfds(chr, NULL, 0); - remove_fd_in_watch(chr); - object_unref(OBJECT(s->sioc)); - s->sioc = NULL; - object_unref(OBJECT(s->ioc)); - s->ioc = NULL; - g_free(chr->filename); chr->filename = SocketAddress_to_str("disconnected:", s->addr, s->is_listen, s->is_telnet); qemu_chr_be_event(chr, CHR_EVENT_CLOSED); @@ -3177,17 +3201,14 @@ int qemu_chr_wait_connected(CharDriverState *chr, Error **errp) static void tcp_chr_close(CharDriverState *chr) { TCPCharDriver *s = chr->opaque; - int i; + + tcp_chr_free_connection(chr); if (s->reconnect_timer) { g_source_remove(s->reconnect_timer); s->reconnect_timer = 0; } qapi_free_SocketAddress(s->addr); - remove_fd_in_watch(chr); - if (s->ioc) { - object_unref(OBJECT(s->ioc)); - } if (s->listen_tag) { g_source_remove(s->listen_tag); s->listen_tag = 0; @@ -3195,18 +3216,9 @@ static void tcp_chr_close(CharDriverState *chr) if (s->listen_ioc) { object_unref(OBJECT(s->listen_ioc)); } - if (s->read_msgfds_num) { - for (i = 0; i < s->read_msgfds_num; i++) { - close(s->read_msgfds[i]); - } - g_free(s->read_msgfds); - } if (s->tls_creds) { object_unref(OBJECT(s->tls_creds)); } - if (s->write_msgfds_num) { - g_free(s->write_msgfds); - } g_free(s); qemu_chr_be_event(chr, CHR_EVENT_CLOSED); } From 1371a3693650c4cf5f12bc103a8c0dae83e6de15 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Lureau?= Date: Thu, 14 Jul 2016 04:14:13 +0200 Subject: [PATCH 3/9] char: free MuxDriver when closing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Similarly to other chr_close callbacks, free char type specific data. Signed-off-by: Marc-André Lureau Reviewed-by: Eric Blake --- qemu-char.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/qemu-char.c b/qemu-char.c index 7c529b2b48..8a0ab05a7b 100644 --- a/qemu-char.c +++ b/qemu-char.c @@ -786,6 +786,13 @@ static GSource *mux_chr_add_watch(CharDriverState *s, GIOCondition cond) return d->drv->chr_add_watch(d->drv, cond); } +static void mux_chr_close(struct CharDriverState *chr) +{ + MuxDriver *d = chr->opaque; + + g_free(d); +} + static CharDriverState *qemu_chr_open_mux(const char *id, ChardevBackend *backend, ChardevReturn *ret, Error **errp) @@ -810,6 +817,7 @@ static CharDriverState *qemu_chr_open_mux(const char *id, chr->opaque = d; d->drv = drv; d->focus = -1; + chr->chr_close = mux_chr_close; chr->chr_write = mux_chr_write; chr->chr_update_read_handler = mux_chr_update_read_handler; chr->chr_accept_input = mux_chr_accept_input; From 9d324b0e67c2b570df389c1361f591b95a4e4278 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Lureau?= Date: Thu, 14 Jul 2016 18:03:40 +0200 Subject: [PATCH 4/9] ahci: free irqs array MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Each irq is referenced by the IDEBus in ide_init2(), thus we can free the no longer used array. Signed-off-by: Marc-André Lureau Reviewed-by: John Snow Acked-by: John Snow --- hw/ide/ahci.c | 1 + 1 file changed, 1 insertion(+) diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c index bcb9ff9e1b..6defeed81c 100644 --- a/hw/ide/ahci.c +++ b/hw/ide/ahci.c @@ -1478,6 +1478,7 @@ void ahci_realize(AHCIState *s, DeviceState *qdev, AddressSpace *as, int ports) ad->port.dma->ops = &ahci_dma_ops; ide_register_restart_cb(&ad->port); } + g_free(irqs); } void ahci_uninit(AHCIState *s) From df37dd6ffe49dc0da919c5034fb89e1f85e3c98f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Lureau?= Date: Fri, 15 Jul 2016 10:41:03 +0200 Subject: [PATCH 5/9] qjson: free str MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Release the qstring allocated in qjson_new(). Signed-off-by: Marc-André Lureau Reviewed-by: Eric Blake --- migration/qjson.c | 1 + 1 file changed, 1 insertion(+) diff --git a/migration/qjson.c b/migration/qjson.c index 5cae55af07..f345904919 100644 --- a/migration/qjson.c +++ b/migration/qjson.c @@ -109,5 +109,6 @@ void qjson_finish(QJSON *json) void qjson_destroy(QJSON *json) { + QDECREF(json->str); g_free(json); } From 0137a557aac07480ff8447ef372f0581af5ee65a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Lureau?= Date: Fri, 15 Jul 2016 10:55:12 +0200 Subject: [PATCH 6/9] virtio-input: free config list MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Clear the list when finalizing. The list is created during realize with virtio_input_idstr_config() and later by further calls to virtio_input_init_config() and virtio_input_add_config(). This leak can be reproduced with device-introspect-test -p /x86_64/device/introspect/concrete. Signed-off-by: Marc-André Lureau Reviewed-by: Gerd Hoffmann --- hw/input/virtio-input.c | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/hw/input/virtio-input.c b/hw/input/virtio-input.c index a87fd6862e..ccdf7308a5 100644 --- a/hw/input/virtio-input.c +++ b/hw/input/virtio-input.c @@ -270,6 +270,16 @@ static void virtio_input_device_realize(DeviceState *dev, Error **errp) vinput->sts = virtio_add_queue(vdev, 64, virtio_input_handle_sts); } +static void virtio_input_finalize(Object *obj) +{ + VirtIOInput *vinput = VIRTIO_INPUT(obj); + VirtIOInputConfig *cfg, *next; + + QTAILQ_FOREACH_SAFE(cfg, &vinput->cfg_list, node, next) { + QTAILQ_REMOVE(&vinput->cfg_list, cfg, node); + g_free(cfg); + } +} static void virtio_input_device_unrealize(DeviceState *dev, Error **errp) { VirtIOInputClass *vic = VIRTIO_INPUT_GET_CLASS(dev); @@ -318,6 +328,7 @@ static const TypeInfo virtio_input_info = { .class_size = sizeof(VirtIOInputClass), .class_init = virtio_input_class_init, .abstract = true, + .instance_finalize = virtio_input_finalize, }; /* ----------------------------------------------------------------- */ From ec507f112361ddf6dac0f0a30e84d9da1b4910b6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Lureau?= Date: Fri, 15 Jul 2016 11:14:07 +0200 Subject: [PATCH 7/9] usb: free USBDevice.strings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The list is created during instance init and further populated with usb_desc_set_string(). Clear it when unrealizing the device. Signed-off-by: Marc-André Lureau Reviewed-by: Gerd Hoffmann --- hw/usb/bus.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/hw/usb/bus.c b/hw/usb/bus.c index c28ccb8a96..25913ad488 100644 --- a/hw/usb/bus.c +++ b/hw/usb/bus.c @@ -279,6 +279,13 @@ static void usb_qdev_realize(DeviceState *qdev, Error **errp) static void usb_qdev_unrealize(DeviceState *qdev, Error **errp) { USBDevice *dev = USB_DEVICE(qdev); + USBDescString *s, *next; + + QLIST_FOREACH_SAFE(s, &dev->strings, next, next) { + QLIST_REMOVE(s, next); + g_free(s->str); + g_free(s); + } if (dev->attached) { usb_device_detach(dev); From 9ef617246b629109e2779835b9a3a8400029484d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Lureau?= Date: Fri, 15 Jul 2016 11:48:13 +0200 Subject: [PATCH 8/9] usb: free leaking path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit qdev_get_dev_path() returns an allocated string, free it when no longer needed. Signed-off-by: Marc-André Lureau Reviewed-by: Gerd Hoffmann --- hw/usb/desc.c | 1 + 1 file changed, 1 insertion(+) diff --git a/hw/usb/desc.c b/hw/usb/desc.c index adb026e43b..5e0e1d157e 100644 --- a/hw/usb/desc.c +++ b/hw/usb/desc.c @@ -574,6 +574,7 @@ void usb_desc_create_serial(USBDevice *dev) } dst += snprintf(serial+dst, sizeof(serial)-dst, "-%s", dev->port->path); usb_desc_set_string(dev, index, serial); + g_free(path); } const char *usb_desc_get_string(USBDevice *dev, uint8_t index) From 5839df7b71540a2af2580bb53ad1e2005bb175e6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Lureau?= Date: Tue, 19 Jul 2016 10:47:46 +0400 Subject: [PATCH 9/9] ahci: fix sglist leak on retry MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ahci-test /x86_64/ahci/io/dma/lba28/retry triggers the following leak: Direct leak of 16 byte(s) in 1 object(s) allocated from: #0 0x7fc4b2a25e20 in malloc (/lib64/libasan.so.3+0xc6e20) #1 0x7fc4993bce58 in g_malloc (/lib64/libglib-2.0.so.0+0x4ee58) #2 0x556a187d4b34 in ahci_populate_sglist hw/ide/ahci.c:896 #3 0x556a187d8237 in ahci_dma_prepare_buf hw/ide/ahci.c:1367 #4 0x556a187b5a1a in ide_dma_cb hw/ide/core.c:844 #5 0x556a187d7eec in ahci_start_dma hw/ide/ahci.c:1333 #6 0x556a187b650b in ide_start_dma hw/ide/core.c:921 #7 0x556a187b61e6 in ide_sector_start_dma hw/ide/core.c:911 #8 0x556a187b9e26 in cmd_write_dma hw/ide/core.c:1486 #9 0x556a187bd519 in ide_exec_cmd hw/ide/core.c:2027 #10 0x556a187d71c5 in handle_reg_h2d_fis hw/ide/ahci.c:1204 #11 0x556a187d7681 in handle_cmd hw/ide/ahci.c:1254 #12 0x556a187d168a in check_cmd hw/ide/ahci.c:510 #13 0x556a187d0afc in ahci_port_write hw/ide/ahci.c:314 #14 0x556a187d105d in ahci_mem_write hw/ide/ahci.c:435 #15 0x556a1831d959 in memory_region_write_accessor /home/elmarco/src/qemu/memory.c:525 #16 0x556a1831dc35 in access_with_adjusted_size /home/elmarco/src/qemu/memory.c:591 #17 0x556a18323ce3 in memory_region_dispatch_write /home/elmarco/src/qemu/memory.c:1262 #18 0x556a1828cf67 in address_space_write_continue /home/elmarco/src/qemu/exec.c:2578 #19 0x556a1828d20b in address_space_write /home/elmarco/src/qemu/exec.c:2635 #20 0x556a1828d92b in address_space_rw /home/elmarco/src/qemu/exec.c:2737 #21 0x556a1828daf7 in cpu_physical_memory_rw /home/elmarco/src/qemu/exec.c:2746 #22 0x556a183068d3 in cpu_physical_memory_write /home/elmarco/src/qemu/include/exec/cpu-common.h:72 #23 0x556a18308194 in qtest_process_command /home/elmarco/src/qemu/qtest.c:382 #24 0x556a18309999 in qtest_process_inbuf /home/elmarco/src/qemu/qtest.c:573 #25 0x556a18309a4a in qtest_read /home/elmarco/src/qemu/qtest.c:585 #26 0x556a18598b85 in qemu_chr_be_write_impl /home/elmarco/src/qemu/qemu-char.c:387 #27 0x556a18598c52 in qemu_chr_be_write /home/elmarco/src/qemu/qemu-char.c:399 #28 0x556a185a2afa in tcp_chr_read /home/elmarco/src/qemu/qemu-char.c:2902 #29 0x556a18cbaf52 in qio_channel_fd_source_dispatch io/channel-watch.c:84 Follow John Snow recommendation: Everywhere else ncq_err is used, it is accompanied by a list cleanup except for ncq_cb, which is the case you are fixing here. Move the sglist destruction inside of ncq_err and then delete it from the other two locations to keep it tidy. Call dma_buf_commit in ide_dma_cb after the early return. Though, this is also a little wonky because this routine does more than clear the list, but it is at the moment the centralized "we're done with the sglist" function and none of the other side effects that occur in dma_buf_commit will interfere with the reset that occurs from ide_restart_bh, I think Signed-off-by: Marc-André Lureau Reviewed-by: John Snow --- hw/ide/ahci.c | 3 +-- hw/ide/core.c | 1 + 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c index 6defeed81c..f3438ad78a 100644 --- a/hw/ide/ahci.c +++ b/hw/ide/ahci.c @@ -919,6 +919,7 @@ static void ncq_err(NCQTransferState *ncq_tfs) ide_state->error = ABRT_ERR; ide_state->status = READY_STAT | ERR_STAT; ncq_tfs->drive->port_regs.scr_err |= (1 << ncq_tfs->tag); + qemu_sglist_destroy(&ncq_tfs->sglist); ncq_tfs->used = 0; } @@ -1025,7 +1026,6 @@ static void execute_ncq_command(NCQTransferState *ncq_tfs) default: DPRINTF(port, "error: unsupported NCQ command (0x%02x) received\n", ncq_tfs->cmd); - qemu_sglist_destroy(&ncq_tfs->sglist); ncq_err(ncq_tfs); } } @@ -1092,7 +1092,6 @@ static void process_ncq_command(AHCIState *s, int port, uint8_t *cmd_fis, error_report("ahci: PRDT length for NCQ command (0x%zx) " "is smaller than the requested size (0x%zx)", ncq_tfs->sglist.size, size); - qemu_sglist_destroy(&ncq_tfs->sglist); ncq_err(ncq_tfs); ahci_trigger_irq(ad->hba, ad, PORT_IRQ_OVERFLOW); return; diff --git a/hw/ide/core.c b/hw/ide/core.c index d117b7c202..45b6df132c 100644 --- a/hw/ide/core.c +++ b/hw/ide/core.c @@ -824,6 +824,7 @@ static void ide_dma_cb(void *opaque, int ret) if (ret < 0) { if (ide_handle_rw_error(s, -ret, ide_dma_cmd_to_retry(s->dma_cmd))) { s->bus->dma->aiocb = NULL; + dma_buf_commit(s, 0); return; } }