[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 01/16] io: store reference to thread informat
From: |
Daniel P . Berrangé |
Subject: |
Re: [Qemu-devel] [PATCH v2 01/16] io: store reference to thread information in the QIOTask struct |
Date: |
Mon, 4 Feb 2019 13:56:08 +0000 |
User-agent: |
Mutt/1.10.1 (2018-07-13) |
On Mon, Feb 04, 2019 at 11:40:55AM +0100, Marc-André Lureau wrote:
> Hi
> On Mon, Feb 4, 2019 at 11:38 AM Marc-André Lureau
> <address@hidden> wrote:
> >
> > On Wed, Jan 23, 2019 at 6:27 PM Daniel P. Berrangé <address@hidden> wrote:
> > >
> > > Currently the struct QIOTaskThreadData is only needed by the worker
> > > thread, but a subsequent patch will need to access it from another
> > > context.
> > >
> > > Signed-off-by: Daniel P. Berrangé <address@hidden>
> >
> > Reviewed-by: Marc-André Lureau <address@hidden>
>
> nack:
>
>
> $ tests/test-char
> /char/null: OK
> /char/invalid: OK
> /char/ringbuf: OK
> /char/mux: OK
> /char/stdio: OK
> /char/pipe: OK
> /char/file: OK
> /char/file-fifo: OK
> /char/udp: OK
> /char/serial: OK
> /char/hotswap: OK
> /char/websocket: OK
> /char/socket/basic: OK
> /char/socket/reconnect:
> =================================================================
> ==22150==ERROR: AddressSanitizer: heap-use-after-free on address
> 0x606000004198 at pc 0x5618a22be8d3 bp 0x7fffa43d8a80 sp
> 0x7fffa43d8a70
> READ of size 8 at 0x606000004198 thread T0
> #0 0x5618a22be8d2 in qio_task_thread_result
> /home/elmarco/src/qemu/io/task.c:91
> #1 0x7f111f08397a (/lib64/libglib-2.0.so.0+0x4b97a)
> #2 0x7f111f08706c in g_main_context_dispatch
> (/lib64/libglib-2.0.so.0+0x4f06c)
> #3 0x5618a23969b7 in glib_pollfds_poll
> /home/elmarco/src/qemu/util/main-loop.c:215
> #4 0x5618a23969b7 in os_host_main_loop_wait
> /home/elmarco/src/qemu/util/main-loop.c:238
> #5 0x5618a23969b7 in main_loop_wait
> /home/elmarco/src/qemu/util/main-loop.c:497
> #6 0x5618a2299786 in main_loop /home/elmarco/src/qemu/tests/test-char.c:27
> #7 0x5618a229b651 in char_socket_test_common
> /home/elmarco/src/qemu/tests/test-char.c:355
> #8 0x7f111f0aefc9 (/lib64/libglib-2.0.so.0+0x76fc9)
> #9 0x7f111f0aee83 (/lib64/libglib-2.0.so.0+0x76e83)
> #10 0x7f111f0aee83 (/lib64/libglib-2.0.so.0+0x76e83)
> #11 0x7f111f0af281 in g_test_run_suite (/lib64/libglib-2.0.so.0+0x77281)
> #12 0x7f111f0af2a4 in g_test_run (/lib64/libglib-2.0.so.0+0x772a4)
> #13 0x5618a2293859 in main /home/elmarco/src/qemu/tests/test-char.c:971
> #14 0x7f111de11412 in __libc_start_main (/lib64/libc.so.6+0x24412)
> #15 0x5618a2295c1d in _start
> (/home/elmarco/src/qemu/build/tests/test-char+0x23ac1d)
>
> 0x606000004198 is located 56 bytes inside of 64-byte region
> [0x606000004160,0x6060000041a0)
> freed by thread T0 here:
> #0 0x7f111f2f0480 in free (/lib64/libasan.so.5+0xef480)
> #1 0x7f111f08ced1 in g_free (/lib64/libglib-2.0.so.0+0x54ed1)
> #2 0x5618a243759f (/home/elmarco/src/qemu/build/tests/test-char+0x3dc59f)
>
> previously allocated by thread T0 here:
> #0 0x7f111f2f0a50 in __interceptor_calloc (/lib64/libasan.so.5+0xefa50)
> #1 0x7f111f08ce1d in g_malloc0 (/lib64/libglib-2.0.so.0+0x54e1d)
>
> SUMMARY: AddressSanitizer: heap-use-after-free
> /home/elmarco/src/qemu/io/task.c:91 in qio_task_thread_result
> Shadow bytes around the buggy address:
FWIW, valgrind reports the same problem
Needs this change squashed in to fix it
diff --git a/io/task.c b/io/task.c
index d100a754d3..396866b10f 100644
--- a/io/task.c
+++ b/io/task.c
@@ -66,6 +66,18 @@ QIOTask *qio_task_new(Object *source,
static void qio_task_free(QIOTask *task)
{
+ if (task->thread) {
+ if (task->thread->destroy) {
+ task->thread->destroy(task->thread->opaque);
+ }
+
+ if (task->thread->context) {
+ g_main_context_unref(task->thread->context);
+ }
+
+ g_free(task->thread);
+ }
+
if (task->destroy) {
task->destroy(task->opaque);
}
@@ -88,17 +100,6 @@ static gboolean qio_task_thread_result(gpointer opaque)
trace_qio_task_thread_result(task);
qio_task_complete(task);
- if (task->thread->destroy) {
- task->thread->destroy(task->thread->opaque);
- }
-
- if (task->thread->context) {
- g_main_context_unref(task->thread->context);
- }
-
- g_free(task->thread);
- task->thread = NULL;
-
return FALSE;
}
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|