Re: [Qemu-devel] [PATCH RFC 2/5] qemu_thread_join: fix segmentation fault

[Fei Li]

On 11/29/2018 10:32 PM, Philippe Mathieu-Daudé wrote:
> Hi Fei,
>
> On 28/11/18 11:33, Fei Li wrote:
> > To avoid the segmentation fault in qemu_thread_join(), just directly
> > return when the QemuThread *thread failed to be created in either
> > qemu-thread-posix.c or qemu-thread-win32.c.
> >
> > Signed-off-by: Fei Li <address@hidden>
> > Reviewed-by: Fam Zheng <address@hidden>
> > ---
> >   util/qemu-thread-posix.c | 3 +++
> >   util/qemu-thread-win32.c | 2 +-
> >   2 files changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/util/qemu-thread-posix.c b/util/qemu-thread-posix.c
> > index 865e476df5..b9ab5a4711 100644
> > --- a/util/qemu-thread-posix.c
> > +++ b/util/qemu-thread-posix.c
> > @@ -558,6 +558,9 @@ void *qemu_thread_join(QemuThread *thread)
> >       int err;
> >       void *ret;
> >   
> > +    if (!thread->thread) {
> > +        return NULL;
> > +    }
> >       err = pthread_join(thread->thread, &ret);
> >       if (err) {
> >           error_exit(err, __func__);
> > diff --git a/util/qemu-thread-win32.c b/util/qemu-thread-win32.c
> > index 4a363ca675..1a27e1cf6f 100644
> > --- a/util/qemu-thread-win32.c
> > +++ b/util/qemu-thread-win32.c
> > @@ -366,7 +366,7 @@ void *qemu_thread_join(QemuThread *thread)
> >       HANDLE handle;
> >   
> >       data = thread->data;
> > -    if (data->mode == QEMU_THREAD_DETACHED) {
> > +    if (data == NULL || data->mode == QEMU_THREAD_DETACHED) {
> Isn't it a way to hide a problem from the caller?
>
> qemu_thread_create() always initializes thread->data.
>
> If the thread doesn't exist you shouldn't try to join() it.
>
> If you try to fix a bug, I'd use here:
>
>         assert(thread->data);
>
> Regards,
>
> Phil.
Emm, this patch is actually should not be abstracted separately...
It should be along with the "[PATCH RFC v7 9/9] qemu_thread_create:
propagate errors to callers to check."

For the 9/9 patch, when qemu_thread_create() fails, we would like
the callers to handle the error instead of directly abort in order to robust
our qemu code (e.g. do not let qemu crash when fails to hot-plug a cpu).
And only within that patch, we need to check whether the thread has been
created: if not just return NULL, which means no handling for that thread.
This is because for many cleanups, we just use a loop to join() all the
thread_count. e.g. when compress_threads_save_setup() fails while
trying to create the second/third do_data_compress thread. (Supposing
there are more than three compress threads).

I will remove this patch in next version. Thanks for pointing this out. :)

Have a nice day
Fei
> >           return NULL;
> >       }
> >