Re: [Qemu-devel] [PATCH] hw/core/generic-loader: Fix crash when running without CPU

[Alistair Francis]

On Thu, Jan 26, 2017 at 12:38 AM, Laurent Vivier <address@hidden> wrote:
> Le 26/01/2017 à 06:50, Thomas Huth a écrit :
>> On 26.01.2017 00:26, Alistair Francis wrote:
>>> On Wed, Jan 25, 2017 at 12:52 PM, Laurent Vivier <address@hidden> wrote:
>>>> Le 25/01/2017 à 21:45, Thomas Huth a écrit :
>>>>> When running QEMU with "-M none -device loader,file=kernel.elf", it
>>>>> currently crashes with a segmentation fault, because the "none"-machine
>>>>> does not have any CPU by default and the generic loader code tries
>>>>> to dereference s->cpu. Fix it by adding an appropriate check for a
>>>>> NULL pointer.
>>>>>
>>>>> Reported-by: Laurent Vivier <address@hidden>
>>>>> Signed-off-by: Thomas Huth <address@hidden>
>>>>> ---
>>>>>  hw/core/generic-loader.c | 9 +++++----
>>>>>  1 file changed, 5 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/hw/core/generic-loader.c b/hw/core/generic-loader.c
>>>>> index 58f1f02..4601267 100644
>>>>> --- a/hw/core/generic-loader.c
>>>>> +++ b/hw/core/generic-loader.c
>>>>> @@ -137,20 +137,21 @@ static void generic_loader_realize(DeviceState 
>>>>> *dev, Error **errp)
>>>>>  #endif
>>>>>
>>>>>      if (s->file) {
>>>>> +        AddressSpace *as = s->cpu ? s->cpu->as :  NULL;
>>>>
>>>> Should we just abort if s->cpu is NULL?
>>>
>>> I agree, what is the use case where you are loading images without a CPU?
>>>
>>> If there is a use case (maybe some KVM thing?) then this patch looks fine 
>>> to me.
>>
>> I think there is no real use case yet. But this fix is 1) simpler than
>> doing an error_report() + exit() here, and 2) maybe the vision of
>> constructing machines on the fly with QEMU will eventually come true one
>> day in the distant future, so with that patch here, the code would
>> already be prepared for the case when QEMU gets started without CPUs and
>> the CPUs are then later added via QOM...

Ok, that is a good enough reason for me.

Reviewed-by: Alistair Francis <address@hidden>

Thanks,

Alistair

>> Well, I don't mind ... if you prefer an error message instead, feel free
>> to suggest another patch. I'm fine as long as we do not simply crash
>> with a segmentation fault here.
>
> OK, the use of NULL as "as" seems reasonable (this is what uses
> load_elf()), so:
>
> Reviewed-by: Laurent Vivier <address@hidden>
>
>
>