[RFC,v7,1/9] Fix segmentation fault when qemu_signal_init fails

When qemu_signal_init() fails in qemu_init_main_loop(), we return
without setting an error.  Its callers crash then when they try to
report the error with error_report_err().

To avoid such segmentation fault, add a new Error parameter to make
the call trace to propagate the err to the final caller.

Cc: Markus Armbruster <armbru@redhat.com>
Cc: Fam Zheng <famz@redhat.com>
Signed-off-by: Fei Li <fli@suse.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
---
 util/main-loop.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Fei Li <fli@suse.com> wrote:
> When qemu_signal_init() fails in qemu_init_main_loop(), we return
> without setting an error.  Its callers crash then when they try to
> report the error with error_report_err().
>
> To avoid such segmentation fault, add a new Error parameter to make
> the call trace to propagate the err to the final caller.

Hi

I agree that there is a bug that exist here.  But I think that the patch
is not 100% correct.  What is the warrantee that when we call
qemu_signal_init() errp is not *already* assigned.

I think that we need to use here the same code that in the call to
aio_context_new() ...

i.e.


intsead of this

>      init_clocks(qemu_timer_notify_cb);
>  
> -    ret = qemu_signal_init();
> +    ret = qemu_signal_init(errp);
>      if (ret) {
>          return ret;
>      }

    init_clocks(qemu_timer_notify_cb);

    ret = qemu_signal_init();
    ret = qemu_signal_init(&local_error);
    if (ret) {
         error_propagate(errp, local_error);
         return ret;
    }

This way it works correctly if errp is NULL, errp is already assigned,
etc, etc,

Or I am missing something?

Later, Juan.

Hi,


On 11/05/2018 09:32 PM, Juan Quintela wrote:
> Fei Li <fli@suse.com> wrote:
>> When qemu_signal_init() fails in qemu_init_main_loop(), we return
>> without setting an error.  Its callers crash then when they try to
>> report the error with error_report_err().
>>
>> To avoid such segmentation fault, add a new Error parameter to make
>> the call trace to propagate the err to the final caller.
> Hi
>
> I agree that there is a bug that exist here.  But I think that the patch
> is not 100% correct.  What is the warrantee that when we call
> qemu_signal_init() errp is not *already* assigned.
>
> I think that we need to use here the same code that in the call to
> aio_context_new() ...
>
> i.e.
>
>
> intsead of this
>
>>       init_clocks(qemu_timer_notify_cb);
>>   
>> -    ret = qemu_signal_init();
>> +    ret = qemu_signal_init(errp);
>>       if (ret) {
>>           return ret;
>>       }
>      init_clocks(qemu_timer_notify_cb);
>
>      ret = qemu_signal_init();
>      ret = qemu_signal_init(&local_error);
>      if (ret) {
>           error_propagate(errp, local_error);
>           return ret;
>      }
>
> This way it works correctly if errp is NULL, errp is already assigned,
> etc, etc,
>
> Or I am missing something?
>
> Later, Juan.
We have discussed this in the first round of this patch series, just as 
Daniel
and Fam said, we only need the local_err & error_propagate() when functions
like object_new_with_propv() returns void, in that way we need the 
&local_err to
check whether that function succeeds.
But in qemu_signal_init, we have the "if (ret) {...}" to judge whether 
it succeeds.
For more details, the following threads can be referred:

09/04/2018 07:26 PM
Re: [Qemu-devel] [PATCH 1/5] Fix segmentation fault when 
qemu_signal_init fails

BTW, if qemu_signalfd() fails, we use an "error_setg_errno()" to handle:
- for NULL errp, we just set the error message to errp;
- for not-NULL errp, besides the error_setv() we have the 
error_handle_fatal(errp, err).
   If the passed errp is &error_fatal/&error_abort, qemu will exit(1) 
right here.

Have a nice day, thanks :)
Fei