Re: [Qemu-devel] [PATCH RFC v7 1/9] Fix segmentation fault when qemu_signal_init fails

[Fei Li <fli@suse.com>]

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