| Message ID | 20181225140449.15786-13-fli@suse.com |
|---|---|
| State | New |
| Headers | show |
| Series | [for-4.0,v9,01/16] Fix segmentation fault when qemu_signal_init fails | expand |
Fei Li <fli@suse.com> writes: > For iothread_complete: utilize the existed errp to propagate the > error and do the corresponding cleanup to replace the temporary > &error_abort. > > For qemu_signalfd_compat: add a local_err to hold the error, and > return the corresponding error code to replace the temporary > &error_abort. I'd split the patch. > > Cc: Markus Armbruster <armbru@redhat.com> > Cc: Eric Blake <eblake@redhat.com> > Signed-off-by: Fei Li <fli@suse.com> > --- > iothread.c | 17 +++++++++++------ > util/compatfd.c | 11 ++++++++--- > 2 files changed, 19 insertions(+), 9 deletions(-) > > diff --git a/iothread.c b/iothread.c > index 8e8aa01999..7335dacf0b 100644 > --- a/iothread.c > +++ b/iothread.c > @@ -164,9 +164,7 @@ static void iothread_complete(UserCreatable *obj, Error **errp) > &local_error); > if (local_error) { > error_propagate(errp, local_error); > - aio_context_unref(iothread->ctx); > - iothread->ctx = NULL; > - return; > + goto fail; > } > > qemu_mutex_init(&iothread->init_done_lock); > @@ -178,9 +176,12 @@ static void iothread_complete(UserCreatable *obj, Error **errp) > */ > name = object_get_canonical_path_component(OBJECT(obj)); > thread_name = g_strdup_printf("IO %s", name); > - /* TODO: let the further caller handle the error instead of abort() here */ > - qemu_thread_create(&iothread->thread, thread_name, iothread_run, > - iothread, QEMU_THREAD_JOINABLE, &error_abort); > + if (!qemu_thread_create(&iothread->thread, thread_name, iothread_run, > + iothread, QEMU_THREAD_JOINABLE, errp)) { > + g_free(thread_name); > + g_free(name); I suspect you're missing cleanup here: qemu_cond_destroy(&iothread->init_done_cond); qemu_mutex_destroy(&iothread->init_done_lock); But I'm not 100% sure, to be honest. Stefan, can you help? > + goto fail; > + } > g_free(thread_name); > g_free(name); > I'd avoid the code duplication like this: thread_ok = qemu_thread_create(&iothread->thread, thread_name, iothread_run, iothread, QEMU_THREAD_JOINABLE, errp); g_free(thread_name); g_free(name); if (!thread_ok) { qemu_cond_destroy(&iothread->init_done_cond); qemu_mutex_destroy(&iothread->init_done_lock); goto fail; } Matter of taste. Hmm, iothread.c has no maintainer. Stefan, you created it, would you be willing to serve as maintainer? > @@ -191,6 +192,10 @@ static void iothread_complete(UserCreatable *obj, Error **errp) > &iothread->init_done_lock); > } > qemu_mutex_unlock(&iothread->init_done_lock); > + return; > +fail: > + aio_context_unref(iothread->ctx); > + iothread->ctx = NULL; > } > > typedef struct { > diff --git a/util/compatfd.c b/util/compatfd.c > index c3d8448264..9cb13381e4 100644 > --- a/util/compatfd.c > +++ b/util/compatfd.c > @@ -71,6 +71,7 @@ static int qemu_signalfd_compat(const sigset_t *mask) > struct sigfd_compat_info *info; > QemuThread thread; > int fds[2]; > + Error *local_err = NULL; > > info = malloc(sizeof(*info)); > if (info == NULL) { > @@ -89,9 +90,13 @@ static int qemu_signalfd_compat(const sigset_t *mask) > memcpy(&info->mask, mask, sizeof(*mask)); > info->fd = fds[1]; > > - /* TODO: let the further caller handle the error instead of abort() here */ > - qemu_thread_create(&thread, "signalfd_compat", sigwait_compat, > - info, QEMU_THREAD_DETACHED, &error_abort); > + if (!qemu_thread_create(&thread, "signalfd_compat", sigwait_compat, > + info, QEMU_THREAD_DETACHED, &local_err)) { > + close(fds[0]); > + close(fds[1]); > + free(info); > + return -1; Leaks @local_err. Pass NULL instead. > + } > > return fds[0]; > }
> 在 2019年1月8日，01:50，Markus Armbruster <armbru@redhat.com> 写道： > > Fei Li <fli@suse.com> writes: > >> For iothread_complete: utilize the existed errp to propagate the >> error and do the corresponding cleanup to replace the temporary >> &error_abort. >> >> For qemu_signalfd_compat: add a local_err to hold the error, and >> return the corresponding error code to replace the temporary >> &error_abort. > > I'd split the patch. Ok. > >> >> Cc: Markus Armbruster <armbru@redhat.com> >> Cc: Eric Blake <eblake@redhat.com> >> Signed-off-by: Fei Li <fli@suse.com> >> --- >> iothread.c | 17 +++++++++++------ >> util/compatfd.c | 11 ++++++++--- >> 2 files changed, 19 insertions(+), 9 deletions(-) >> >> diff --git a/iothread.c b/iothread.c >> index 8e8aa01999..7335dacf0b 100644 >> --- a/iothread.c >> +++ b/iothread.c >> @@ -164,9 +164,7 @@ static void iothread_complete(UserCreatable *obj, Error **errp) >> &local_error); >> if (local_error) { >> error_propagate(errp, local_error); >> - aio_context_unref(iothread->ctx); >> - iothread->ctx = NULL; >> - return; >> + goto fail; >> } >> >> qemu_mutex_init(&iothread->init_done_lock); >> @@ -178,9 +176,12 @@ static void iothread_complete(UserCreatable *obj, Error **errp) >> */ >> name = object_get_canonical_path_component(OBJECT(obj)); >> thread_name = g_strdup_printf("IO %s", name); >> - /* TODO: let the further caller handle the error instead of abort() here */ >> - qemu_thread_create(&iothread->thread, thread_name, iothread_run, >> - iothread, QEMU_THREAD_JOINABLE, &error_abort); >> + if (!qemu_thread_create(&iothread->thread, thread_name, iothread_run, >> + iothread, QEMU_THREAD_JOINABLE, errp)) { >> + g_free(thread_name); >> + g_free(name); > > I suspect you're missing cleanup here: > > qemu_cond_destroy(&iothread->init_done_cond); > qemu_mutex_destroy(&iothread->init_done_lock); I remember I checked the code, when ucc->complete() fails, there’s a finalize() function to do the destroy. But did not test all the callers, so let’s wait for Stefan’s feedback. :) > > But I'm not 100% sure, to be honest. Stefan, can you help? > > >> + goto fail; >> + } >> g_free(thread_name); >> g_free(name); >> > > I'd avoid the code duplication like this: > > thread_ok = qemu_thread_create(&iothread->thread, thread_name, > iothread_run, iothread, > QEMU_THREAD_JOINABLE, errp); > g_free(thread_name); > g_free(name); > if (!thread_ok) { > qemu_cond_destroy(&iothread->init_done_cond); > qemu_mutex_destroy(&iothread->init_done_lock); > goto fail; > } > > Matter of taste. > > Hmm, iothread.c has no maintainer. Stefan, you created it, would you be > willing to serve as maintainer? > >> @@ -191,6 +192,10 @@ static void iothread_complete(UserCreatable *obj, Error **errp) >> &iothread->init_done_lock); >> } >> qemu_mutex_unlock(&iothread->init_done_lock); >> + return; >> +fail: >> + aio_context_unref(iothread->ctx); >> + iothread->ctx = NULL; >> } >> >> typedef struct { >> diff --git a/util/compatfd.c b/util/compatfd.c >> index c3d8448264..9cb13381e4 100644 >> --- a/util/compatfd.c >> +++ b/util/compatfd.c >> @@ -71,6 +71,7 @@ static int qemu_signalfd_compat(const sigset_t *mask) >> struct sigfd_compat_info *info; >> QemuThread thread; >> int fds[2]; >> + Error *local_err = NULL; >> >> info = malloc(sizeof(*info)); >> if (info == NULL) { >> @@ -89,9 +90,13 @@ static int qemu_signalfd_compat(const sigset_t *mask) >> memcpy(&info->mask, mask, sizeof(*mask)); >> info->fd = fds[1]; >> >> - /* TODO: let the further caller handle the error instead of abort() here */ >> - qemu_thread_create(&thread, "signalfd_compat", sigwait_compat, >> - info, QEMU_THREAD_DETACHED, &error_abort); >> + if (!qemu_thread_create(&thread, "signalfd_compat", sigwait_compat, >> + info, QEMU_THREAD_DETACHED, &local_err)) { >> + close(fds[0]); >> + close(fds[1]); >> + free(info); >> + return -1; > > Leaks @local_err. Pass NULL instead. Ok, thanks! Have a nice day Fei > >> + } >> >> return fds[0]; >> }
在 2019/1/9 上午12:18, fei 写道: > >> 在 2019年1月8日，01:50，Markus Armbruster <armbru@redhat.com> 写道： >> >> Fei Li <fli@suse.com> writes: >> >>> For iothread_complete: utilize the existed errp to propagate the >>> error and do the corresponding cleanup to replace the temporary >>> &error_abort. >>> >>> For qemu_signalfd_compat: add a local_err to hold the error, and >>> return the corresponding error code to replace the temporary >>> &error_abort. >> I'd split the patch. > Ok. >>> Cc: Markus Armbruster <armbru@redhat.com> >>> Cc: Eric Blake <eblake@redhat.com> >>> Signed-off-by: Fei Li <fli@suse.com> >>> --- >>> iothread.c | 17 +++++++++++------ >>> util/compatfd.c | 11 ++++++++--- >>> 2 files changed, 19 insertions(+), 9 deletions(-) >>> >>> diff --git a/iothread.c b/iothread.c >>> index 8e8aa01999..7335dacf0b 100644 >>> --- a/iothread.c >>> +++ b/iothread.c >>> @@ -164,9 +164,7 @@ static void iothread_complete(UserCreatable *obj, Error **errp) >>> &local_error); >>> if (local_error) { >>> error_propagate(errp, local_error); >>> - aio_context_unref(iothread->ctx); >>> - iothread->ctx = NULL; >>> - return; >>> + goto fail; >>> } >>> >>> qemu_mutex_init(&iothread->init_done_lock); >>> @@ -178,9 +176,12 @@ static void iothread_complete(UserCreatable *obj, Error **errp) >>> */ >>> name = object_get_canonical_path_component(OBJECT(obj)); >>> thread_name = g_strdup_printf("IO %s", name); >>> - /* TODO: let the further caller handle the error instead of abort() here */ >>> - qemu_thread_create(&iothread->thread, thread_name, iothread_run, >>> - iothread, QEMU_THREAD_JOINABLE, &error_abort); >>> + if (!qemu_thread_create(&iothread->thread, thread_name, iothread_run, >>> + iothread, QEMU_THREAD_JOINABLE, errp)) { >>> + g_free(thread_name); >>> + g_free(name); >> I suspect you're missing cleanup here: >> >> qemu_cond_destroy(&iothread->init_done_cond); >> qemu_mutex_destroy(&iothread->init_done_lock); > I remember I checked the code, when ucc->complete() fails, there’s a finalize() function to do the destroy. To be specific, the qemu_xxx_destroy() is called by object_unref() => object_finalize() => object_deinit() => type->instance_finalize(obj); (that is, iothread_instance_finalize). For the iothread_complete(), it is only called in user_creatable_complete() as ucc->complete(). I checked the code, when callers of user_creatable_complete() fails, all of them will call object_unref() to call the qemu_xxx_destroy(), except one &error_abort case (e.i. desugar_shm()). > But did not test all the callers, so let’s wait for Stefan’s feedback. :) But again, I did not do all the test. Correct me if I am wrong. :) >> But I'm not 100% sure, to be honest. Stefan, can you help? >> >> >>> + goto fail; >>> + } >>> g_free(thread_name); >>> g_free(name); >>> >> I'd avoid the code duplication like this: >> >> thread_ok = qemu_thread_create(&iothread->thread, thread_name, >> iothread_run, iothread, >> QEMU_THREAD_JOINABLE, errp); >> g_free(thread_name); >> g_free(name); >> if (!thread_ok) { >> qemu_cond_destroy(&iothread->init_done_cond); >> qemu_mutex_destroy(&iothread->init_done_lock); >> goto fail; >> } Ok, thanks. Have a nice day Fei >> Matter of taste. >> >> Hmm, iothread.c has no maintainer. Stefan, you created it, would you be >> willing to serve as maintainer? >>
Fei Li <lifei1214@126.com> writes: > 在 2019/1/9 上午12:18, fei 写道: >> >>> 在 2019年1月8日，01:50，Markus Armbruster <armbru@redhat.com> 写道： >>> >>> Fei Li <fli@suse.com> writes: >>> >>>> For iothread_complete: utilize the existed errp to propagate the >>>> error and do the corresponding cleanup to replace the temporary >>>> &error_abort. >>>> >>>> For qemu_signalfd_compat: add a local_err to hold the error, and >>>> return the corresponding error code to replace the temporary >>>> &error_abort. >>> I'd split the patch. >> Ok. >>>> Cc: Markus Armbruster <armbru@redhat.com> >>>> Cc: Eric Blake <eblake@redhat.com> >>>> Signed-off-by: Fei Li <fli@suse.com> >>>> --- >>>> iothread.c | 17 +++++++++++------ >>>> util/compatfd.c | 11 ++++++++--- >>>> 2 files changed, 19 insertions(+), 9 deletions(-) >>>> >>>> diff --git a/iothread.c b/iothread.c >>>> index 8e8aa01999..7335dacf0b 100644 >>>> --- a/iothread.c >>>> +++ b/iothread.c >>>> @@ -164,9 +164,7 @@ static void iothread_complete(UserCreatable *obj, Error **errp) >>>> &local_error); >>>> if (local_error) { >>>> error_propagate(errp, local_error); >>>> - aio_context_unref(iothread->ctx); >>>> - iothread->ctx = NULL; >>>> - return; >>>> + goto fail; >>>> } >>>> >>>> qemu_mutex_init(&iothread->init_done_lock); >>>> @@ -178,9 +176,12 @@ static void iothread_complete(UserCreatable *obj, Error **errp) >>>> */ >>>> name = object_get_canonical_path_component(OBJECT(obj)); >>>> thread_name = g_strdup_printf("IO %s", name); >>>> - /* TODO: let the further caller handle the error instead of abort() here */ >>>> - qemu_thread_create(&iothread->thread, thread_name, iothread_run, >>>> - iothread, QEMU_THREAD_JOINABLE, &error_abort); >>>> + if (!qemu_thread_create(&iothread->thread, thread_name, iothread_run, >>>> + iothread, QEMU_THREAD_JOINABLE, errp)) { >>>> + g_free(thread_name); >>>> + g_free(name); >>> I suspect you're missing cleanup here: >>> >>> qemu_cond_destroy(&iothread->init_done_cond); >>> qemu_mutex_destroy(&iothread->init_done_lock); >> I remember I checked the code, when ucc->complete() fails, there’s a finalize() function to do the destroy. > > To be specific, the qemu_xxx_destroy() is called by > > object_unref() => object_finalize() => object_deinit() => > type->instance_finalize(obj); (that is, iothread_instance_finalize). > > For the iothread_complete(), it is only called in > user_creatable_complete() as ucc->complete(). > I checked the code, when callers of user_creatable_complete() fails, > all of them will call > object_unref() to call the qemu_xxx_destroy(), except one &error_abort > case (e.i. desugar_shm()). I'm not familiar with iothread.c. But like anyone capable of reading C, I can find out stuff. iothread_instance_finalize() guards its cleanups. In particular, it cleans up ->init_done_cond and init_done_lock only when ->thread_id != -1. iothread_instance_init() initializes ->thread_id = -1. iothread_run() sets it to the actual thread ID. When iothread_instance_complete() succeeds, it has waited for ->thread_id to become != -1, in the /* Wait for initialization to complete */ loop. When it fails, ->thread_id is still -1. Therefore, you cannot rely on iothread_instance_finalize() for cleaning up ->init_done_lock and ->init_done_cond on iothread_instance_complete() failure. I'm pretty sure you could've figured this out yourself instead of relying on me. [...]
在 2019/1/14 下午8:53, Markus Armbruster 写道: > Fei Li <lifei1214@126.com> writes: > >> 在 2019/1/9 上午12:18, fei 写道: >>>> 在 2019年1月8日，01:50，Markus Armbruster <armbru@redhat.com> 写道： >>>> >>>> Fei Li <fli@suse.com> writes: >>>> >>>>> For iothread_complete: utilize the existed errp to propagate the >>>>> error and do the corresponding cleanup to replace the temporary >>>>> &error_abort. >>>>> >>>>> For qemu_signalfd_compat: add a local_err to hold the error, and >>>>> return the corresponding error code to replace the temporary >>>>> &error_abort. >>>> I'd split the patch. >>> Ok. >>>>> Cc: Markus Armbruster <armbru@redhat.com> >>>>> Cc: Eric Blake <eblake@redhat.com> >>>>> Signed-off-by: Fei Li <fli@suse.com> >>>>> --- >>>>> iothread.c | 17 +++++++++++------ >>>>> util/compatfd.c | 11 ++++++++--- >>>>> 2 files changed, 19 insertions(+), 9 deletions(-) >>>>> >>>>> diff --git a/iothread.c b/iothread.c >>>>> index 8e8aa01999..7335dacf0b 100644 >>>>> --- a/iothread.c >>>>> +++ b/iothread.c >>>>> @@ -164,9 +164,7 @@ static void iothread_complete(UserCreatable *obj, Error **errp) >>>>> &local_error); >>>>> if (local_error) { >>>>> error_propagate(errp, local_error); >>>>> - aio_context_unref(iothread->ctx); >>>>> - iothread->ctx = NULL; >>>>> - return; >>>>> + goto fail; >>>>> } >>>>> >>>>> qemu_mutex_init(&iothread->init_done_lock); >>>>> @@ -178,9 +176,12 @@ static void iothread_complete(UserCreatable *obj, Error **errp) >>>>> */ >>>>> name = object_get_canonical_path_component(OBJECT(obj)); >>>>> thread_name = g_strdup_printf("IO %s", name); >>>>> - /* TODO: let the further caller handle the error instead of abort() here */ >>>>> - qemu_thread_create(&iothread->thread, thread_name, iothread_run, >>>>> - iothread, QEMU_THREAD_JOINABLE, &error_abort); >>>>> + if (!qemu_thread_create(&iothread->thread, thread_name, iothread_run, >>>>> + iothread, QEMU_THREAD_JOINABLE, errp)) { >>>>> + g_free(thread_name); >>>>> + g_free(name); >>>> I suspect you're missing cleanup here: >>>> >>>> qemu_cond_destroy(&iothread->init_done_cond); >>>> qemu_mutex_destroy(&iothread->init_done_lock); >>> I remember I checked the code, when ucc->complete() fails, there’s a finalize() function to do the destroy. >> To be specific, the qemu_xxx_destroy() is called by >> >> object_unref() => object_finalize() => object_deinit() => >> type->instance_finalize(obj); (that is, iothread_instance_finalize). >> >> For the iothread_complete(), it is only called in >> user_creatable_complete() as ucc->complete(). >> I checked the code, when callers of user_creatable_complete() fails, >> all of them will call >> object_unref() to call the qemu_xxx_destroy(), except one &error_abort >> case (e.i. desugar_shm()). > I'm not familiar with iothread.c. But like anyone capable of reading C, > I can find out stuff. > > iothread_instance_finalize() guards its cleanups. In particular, it > cleans up ->init_done_cond and init_done_lock only when ->thread_id != > -1. Ah, sorry that I overlooked the "if (iothread->thread_id != -1)". So embarrassed, and sorry for the trouble.. You are right, and I will add the qemu_xxx_destroy() in the next version. ;) Have a nice day, thanks so much! Fei > > iothread_instance_init() initializes ->thread_id = -1. > > iothread_run() sets it to the actual thread ID. > > When iothread_instance_complete() succeeds, it has waited for > ->thread_id to become != -1, in the /* Wait for initialization to > complete */ loop. > > When it fails, ->thread_id is still -1. > > Therefore, you cannot rely on iothread_instance_finalize() for cleaning > up ->init_done_lock and ->init_done_cond on iothread_instance_complete() > failure. > > I'm pretty sure you could've figured this out yourself instead of > relying on me. > > [...]
diff --git a/iothread.c b/iothread.c index 8e8aa01999..7335dacf0b 100644 --- a/iothread.c +++ b/iothread.c @@ -164,9 +164,7 @@ static void iothread_complete(UserCreatable *obj, Error **errp) &local_error); if (local_error) { error_propagate(errp, local_error); - aio_context_unref(iothread->ctx); - iothread->ctx = NULL; - return; + goto fail; } qemu_mutex_init(&iothread->init_done_lock); @@ -178,9 +176,12 @@ static void iothread_complete(UserCreatable *obj, Error **errp) */ name = object_get_canonical_path_component(OBJECT(obj)); thread_name = g_strdup_printf("IO %s", name); - /* TODO: let the further caller handle the error instead of abort() here */ - qemu_thread_create(&iothread->thread, thread_name, iothread_run, - iothread, QEMU_THREAD_JOINABLE, &error_abort); + if (!qemu_thread_create(&iothread->thread, thread_name, iothread_run, + iothread, QEMU_THREAD_JOINABLE, errp)) { + g_free(thread_name); + g_free(name); + goto fail; + } g_free(thread_name); g_free(name); @@ -191,6 +192,10 @@ static void iothread_complete(UserCreatable *obj, Error **errp) &iothread->init_done_lock); } qemu_mutex_unlock(&iothread->init_done_lock); + return; +fail: + aio_context_unref(iothread->ctx); + iothread->ctx = NULL; } typedef struct { diff --git a/util/compatfd.c b/util/compatfd.c index c3d8448264..9cb13381e4 100644 --- a/util/compatfd.c +++ b/util/compatfd.c @@ -71,6 +71,7 @@ static int qemu_signalfd_compat(const sigset_t *mask) struct sigfd_compat_info *info; QemuThread thread; int fds[2]; + Error *local_err = NULL; info = malloc(sizeof(*info)); if (info == NULL) { @@ -89,9 +90,13 @@ static int qemu_signalfd_compat(const sigset_t *mask) memcpy(&info->mask, mask, sizeof(*mask)); info->fd = fds[1]; - /* TODO: let the further caller handle the error instead of abort() here */ - qemu_thread_create(&thread, "signalfd_compat", sigwait_compat, - info, QEMU_THREAD_DETACHED, &error_abort); + if (!qemu_thread_create(&thread, "signalfd_compat", sigwait_compat, + info, QEMU_THREAD_DETACHED, &local_err)) { + close(fds[0]); + close(fds[1]); + free(info); + return -1; + } return fds[0]; }
For iothread_complete: utilize the existed errp to propagate the error and do the corresponding cleanup to replace the temporary &error_abort. For qemu_signalfd_compat: add a local_err to hold the error, and return the corresponding error code to replace the temporary &error_abort. Cc: Markus Armbruster <armbru@redhat.com> Cc: Eric Blake <eblake@redhat.com> Signed-off-by: Fei Li <fli@suse.com> --- iothread.c | 17 +++++++++++------ util/compatfd.c | 11 ++++++++--- 2 files changed, 19 insertions(+), 9 deletions(-)