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

[Laurent Vivier]

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...
> 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>