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é <berrange@redhat.com>
---
io/task.c | 53 ++++++++++++++++++++++++++++-------------------------
1 file changed, 28 insertions(+), 25 deletions(-)
diff --git a/io/task.c b/io/task.c
index 2886a2c1bc..d100a754d3 100644
--- a/io/task.c
+++ b/io/task.c
@@ -24,6 +24,14 @@
#include "qemu/thread.h"
#include "trace.h"
+struct QIOTaskThreadData {
+ QIOTaskWorker worker;
+ gpointer opaque;
+ GDestroyNotify destroy;
+ GMainContext *context;
+};
+
+
struct QIOTask {
Object *source;
QIOTaskFunc func;
@@ -32,6 +40,7 @@ struct QIOTask {
Error *err;
gpointer result;
GDestroyNotify destroyResult;
+ struct QIOTaskThreadData *thread;
};
@@ -72,31 +81,23 @@ static void qio_task_free(QIOTask *task)
}
-struct QIOTaskThreadData {
- QIOTask *task;
- QIOTaskWorker worker;
- gpointer opaque;
- GDestroyNotify destroy;
- GMainContext *context;
-};
-
-
static gboolean qio_task_thread_result(gpointer opaque)
{
- struct QIOTaskThreadData *data = opaque;
+ QIOTask *task = opaque;
- trace_qio_task_thread_result(data->task);
- qio_task_complete(data->task);
+ trace_qio_task_thread_result(task);
+ qio_task_complete(task);
- if (data->destroy) {
- data->destroy(data->opaque);
+ if (task->thread->destroy) {
+ task->thread->destroy(task->thread->opaque);
}
- if (data->context) {
- g_main_context_unref(data->context);
+ if (task->thread->context) {
+ g_main_context_unref(task->thread->context);
}
- g_free(data);
+ g_free(task->thread);
+ task->thread = NULL;
return FALSE;
}
@@ -104,22 +105,23 @@ static gboolean qio_task_thread_result(gpointer opaque)
static gpointer qio_task_thread_worker(gpointer opaque)
{
- struct QIOTaskThreadData *data = opaque;
+ QIOTask *task = opaque;
GSource *idle;
- trace_qio_task_thread_run(data->task);
- data->worker(data->task, data->opaque);
+ trace_qio_task_thread_run(task);
+
+ task->thread->worker(task, task->thread->opaque);
/* We're running in the background thread, and must only
* ever report the task results in the main event loop
* thread. So we schedule an idle callback to report
* the worker results
*/
- trace_qio_task_thread_exit(data->task);
+ trace_qio_task_thread_exit(task);
idle = g_idle_source_new();
- g_source_set_callback(idle, qio_task_thread_result, data, NULL);
- g_source_attach(idle, data->context);
+ g_source_set_callback(idle, qio_task_thread_result, task, NULL);
+ g_source_attach(idle, task->thread->context);
return NULL;
}
@@ -138,17 +140,18 @@ void qio_task_run_in_thread(QIOTask *task,
g_main_context_ref(context);
}
- data->task = task;
data->worker = worker;
data->opaque = opaque;
data->destroy = destroy;
data->context = context;
+ task->thread = data;
+
trace_qio_task_thread_start(task, worker, opaque);
qemu_thread_create(&thread,
"io-task-worker",
qio_task_thread_worker,
- data,
+ task,
QEMU_THREAD_DETACHED);
}
--
2.20.1
On Wed, Jan 23, 2019 at 6:27 PM Daniel P. Berrangé <berrange@redhat.com> 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é <berrange@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
> io/task.c | 53 ++++++++++++++++++++++++++++-------------------------
> 1 file changed, 28 insertions(+), 25 deletions(-)
>
> diff --git a/io/task.c b/io/task.c
> index 2886a2c1bc..d100a754d3 100644
> --- a/io/task.c
> +++ b/io/task.c
> @@ -24,6 +24,14 @@
> #include "qemu/thread.h"
> #include "trace.h"
>
> +struct QIOTaskThreadData {
> + QIOTaskWorker worker;
> + gpointer opaque;
> + GDestroyNotify destroy;
> + GMainContext *context;
> +};
> +
> +
> struct QIOTask {
> Object *source;
> QIOTaskFunc func;
> @@ -32,6 +40,7 @@ struct QIOTask {
> Error *err;
> gpointer result;
> GDestroyNotify destroyResult;
> + struct QIOTaskThreadData *thread;
> };
>
>
> @@ -72,31 +81,23 @@ static void qio_task_free(QIOTask *task)
> }
>
>
> -struct QIOTaskThreadData {
> - QIOTask *task;
> - QIOTaskWorker worker;
> - gpointer opaque;
> - GDestroyNotify destroy;
> - GMainContext *context;
> -};
> -
> -
> static gboolean qio_task_thread_result(gpointer opaque)
> {
> - struct QIOTaskThreadData *data = opaque;
> + QIOTask *task = opaque;
>
> - trace_qio_task_thread_result(data->task);
> - qio_task_complete(data->task);
> + trace_qio_task_thread_result(task);
> + qio_task_complete(task);
>
> - if (data->destroy) {
> - data->destroy(data->opaque);
> + if (task->thread->destroy) {
> + task->thread->destroy(task->thread->opaque);
> }
>
> - if (data->context) {
> - g_main_context_unref(data->context);
> + if (task->thread->context) {
> + g_main_context_unref(task->thread->context);
> }
>
> - g_free(data);
> + g_free(task->thread);
> + task->thread = NULL;
>
> return FALSE;
> }
> @@ -104,22 +105,23 @@ static gboolean qio_task_thread_result(gpointer opaque)
>
> static gpointer qio_task_thread_worker(gpointer opaque)
> {
> - struct QIOTaskThreadData *data = opaque;
> + QIOTask *task = opaque;
> GSource *idle;
>
> - trace_qio_task_thread_run(data->task);
> - data->worker(data->task, data->opaque);
> + trace_qio_task_thread_run(task);
> +
> + task->thread->worker(task, task->thread->opaque);
>
> /* We're running in the background thread, and must only
> * ever report the task results in the main event loop
> * thread. So we schedule an idle callback to report
> * the worker results
> */
> - trace_qio_task_thread_exit(data->task);
> + trace_qio_task_thread_exit(task);
>
> idle = g_idle_source_new();
> - g_source_set_callback(idle, qio_task_thread_result, data, NULL);
> - g_source_attach(idle, data->context);
> + g_source_set_callback(idle, qio_task_thread_result, task, NULL);
> + g_source_attach(idle, task->thread->context);
>
> return NULL;
> }
> @@ -138,17 +140,18 @@ void qio_task_run_in_thread(QIOTask *task,
> g_main_context_ref(context);
> }
>
> - data->task = task;
> data->worker = worker;
> data->opaque = opaque;
> data->destroy = destroy;
> data->context = context;
>
> + task->thread = data;
> +
> trace_qio_task_thread_start(task, worker, opaque);
> qemu_thread_create(&thread,
> "io-task-worker",
> qio_task_thread_worker,
> - data,
> + task,
> QEMU_THREAD_DETACHED);
> }
>
> --
> 2.20.1
>
Hi
On Mon, Feb 4, 2019 at 11:38 AM Marc-André Lureau
<marcandre.lureau@redhat.com> wrote:
>
> On Wed, Jan 23, 2019 at 6:27 PM Daniel P. Berrangé <berrange@redhat.com> 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é <berrange@redhat.com>
>
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
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:
> > ---
> > io/task.c | 53 ++++++++++++++++++++++++++++-------------------------
> > 1 file changed, 28 insertions(+), 25 deletions(-)
> >
> > diff --git a/io/task.c b/io/task.c
> > index 2886a2c1bc..d100a754d3 100644
> > --- a/io/task.c
> > +++ b/io/task.c
> > @@ -24,6 +24,14 @@
> > #include "qemu/thread.h"
> > #include "trace.h"
> >
> > +struct QIOTaskThreadData {
> > + QIOTaskWorker worker;
> > + gpointer opaque;
> > + GDestroyNotify destroy;
> > + GMainContext *context;
> > +};
> > +
> > +
> > struct QIOTask {
> > Object *source;
> > QIOTaskFunc func;
> > @@ -32,6 +40,7 @@ struct QIOTask {
> > Error *err;
> > gpointer result;
> > GDestroyNotify destroyResult;
> > + struct QIOTaskThreadData *thread;
> > };
> >
> >
> > @@ -72,31 +81,23 @@ static void qio_task_free(QIOTask *task)
> > }
> >
> >
> > -struct QIOTaskThreadData {
> > - QIOTask *task;
> > - QIOTaskWorker worker;
> > - gpointer opaque;
> > - GDestroyNotify destroy;
> > - GMainContext *context;
> > -};
> > -
> > -
> > static gboolean qio_task_thread_result(gpointer opaque)
> > {
> > - struct QIOTaskThreadData *data = opaque;
> > + QIOTask *task = opaque;
> >
> > - trace_qio_task_thread_result(data->task);
> > - qio_task_complete(data->task);
> > + trace_qio_task_thread_result(task);
> > + qio_task_complete(task);
> >
> > - if (data->destroy) {
> > - data->destroy(data->opaque);
> > + if (task->thread->destroy) {
> > + task->thread->destroy(task->thread->opaque);
> > }
> >
> > - if (data->context) {
> > - g_main_context_unref(data->context);
> > + if (task->thread->context) {
> > + g_main_context_unref(task->thread->context);
> > }
> >
> > - g_free(data);
> > + g_free(task->thread);
> > + task->thread = NULL;
> >
> > return FALSE;
> > }
> > @@ -104,22 +105,23 @@ static gboolean qio_task_thread_result(gpointer opaque)
> >
> > static gpointer qio_task_thread_worker(gpointer opaque)
> > {
> > - struct QIOTaskThreadData *data = opaque;
> > + QIOTask *task = opaque;
> > GSource *idle;
> >
> > - trace_qio_task_thread_run(data->task);
> > - data->worker(data->task, data->opaque);
> > + trace_qio_task_thread_run(task);
> > +
> > + task->thread->worker(task, task->thread->opaque);
> >
> > /* We're running in the background thread, and must only
> > * ever report the task results in the main event loop
> > * thread. So we schedule an idle callback to report
> > * the worker results
> > */
> > - trace_qio_task_thread_exit(data->task);
> > + trace_qio_task_thread_exit(task);
> >
> > idle = g_idle_source_new();
> > - g_source_set_callback(idle, qio_task_thread_result, data, NULL);
> > - g_source_attach(idle, data->context);
> > + g_source_set_callback(idle, qio_task_thread_result, task, NULL);
> > + g_source_attach(idle, task->thread->context);
> >
> > return NULL;
> > }
> > @@ -138,17 +140,18 @@ void qio_task_run_in_thread(QIOTask *task,
> > g_main_context_ref(context);
> > }
> >
> > - data->task = task;
> > data->worker = worker;
> > data->opaque = opaque;
> > data->destroy = destroy;
> > data->context = context;
> >
> > + task->thread = data;
> > +
> > trace_qio_task_thread_start(task, worker, opaque);
> > qemu_thread_create(&thread,
> > "io-task-worker",
> > qio_task_thread_worker,
> > - data,
> > + task,
> > QEMU_THREAD_DETACHED);
> > }
> >
> > --
> > 2.20.1
> >
>
--
Marc-André Lureau
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
> <marcandre.lureau@redhat.com> wrote:
> >
> > On Wed, Jan 23, 2019 at 6:27 PM Daniel P. Berrangé <berrange@redhat.com> 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é <berrange@redhat.com>
> >
> > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> 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 :|
© 2016 - 2025 Red Hat, Inc.