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 <f...@suse.com> Reviewed-by: Fam Zheng <f...@redhat.com> --- 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; }
