Re: [PATCH v2 20/25] x86: Fix x86_cpu_new() error API violations

[Markus Armbruster]

Igor Mammedov <imammedo@redhat.com> writes:

> On Wed, 24 Jun 2020 10:37:32 +0200
> Markus Armbruster <armbru@redhat.com> wrote:
>
>> The Error ** argument must be NULL, &error_abort, &error_fatal, or a
>> pointer to a variable containing NULL.  Passing an argument of the
>> latter kind twice without clearing it in between is wrong: if the
>> first call sets an error, it no longer points to NULL for the second
>> call.
>> 
>> x86_cpu_new() is wrong that way: it passes &local_err to
>> object_property_set_uint() without checking it, and then to
>> qdev_realize().  Harmless, because the former can't actually fail
>> here.
>> 
>> Fix by checking for failure right away.  While there, replace
>> qdev_realize(); object_unref() by qdev_realize_and_unref().
>> 
>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>> Cc: Richard Henderson <rth@twiddle.net>
>> Cc: Eduardo Habkost <ehabkost@redhat.com>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>>  hw/i386/x86.c | 12 +++---------
>>  1 file changed, 3 insertions(+), 9 deletions(-)
>> 
>> diff --git a/hw/i386/x86.c b/hw/i386/x86.c
>> index 34229b45c7..3a7029e6db 100644
>> --- a/hw/i386/x86.c
>> +++ b/hw/i386/x86.c
>> @@ -118,16 +118,10 @@ uint32_t x86_cpu_apic_id_from_index(X86MachineState 
>> *x86ms,
>>  
>>  void x86_cpu_new(X86MachineState *x86ms, int64_t apic_id, Error **errp)
>>  {
>> -    Object *cpu = NULL;
>> -    Error *local_err = NULL;
>> +    Object *cpu = object_new(MACHINE(x86ms)->cpu_type);
>>  
>> -    cpu = object_new(MACHINE(x86ms)->cpu_type);
>> -
>> -    object_property_set_uint(cpu, apic_id, "apic-id", &local_err);
>> -    qdev_realize(DEVICE(cpu), NULL, &local_err);
>> -
>> -    object_unref(cpu);
>> -    error_propagate(errp, local_err);
>> +    object_property_set_uint(cpu, apic_id, "apic-id", &error_abort);
> it may fail here if user specified wrong cpu flags, but there is nothing we 
> can do to fix it.

Really?

object_property_set_uint() fails when property "apic-id" doesn't exist,
has no ->set() method, or its ->set() fails.

Property "apic-id" is defined in x86_cpu_properties[] as

    DEFINE_PROP_UINT32("apic-id", X86CPU, apic_id, UNASSIGNED_APIC_ID),

This means "apic-id" exists, and its ->set() is set_uint32().  That
leaves only set_uint32() as possible source of failure.

It fails when

* the device is already realized: programming error

* the value to be stored is not an integer: object_property_set_uint()
  makes it one, can't fail

* the value is not representable as uint32_t: @api_id is declared as
  int64_t, but:

  - pc_hot_add_cpu() passes x86_cpu_apic_id_from_index(), which is
    uint32_t, converted to int64_t.  Can't fail.

  - x86_cpus_init() passes possible_cpus->cpus[i].arch_id, which is
    uint64_t.  Is this the "if user specified wrong cpu flags" case?

  Aside: should the integer types be cleaned up?

To assess the bug's impact, we need to know when the other call in this
error pileup fails.  If we can make both fail, we have a crash bug.
Else, we have a harmless API violation.

Any ideas on how to make the qdev_realize() fail?

[...]