[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 20/25] x86: Fix x86_cpu_new() error API violations
From: |
Markus Armbruster |
Subject: |
Re: [PATCH v2 20/25] x86: Fix x86_cpu_new() error API violations |
Date: |
Fri, 26 Jun 2020 14:54:38 +0200 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/26.3 (gnu/linux) |
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?
[...]
- [PATCH v2 04/25] tests: Use &error_abort where appropriate, (continued)
- [PATCH v2 04/25] tests: Use &error_abort where appropriate, Markus Armbruster, 2020/06/24
- [PATCH v2 13/25] qdev: Drop qbus_set_bus_hotplug_handler() parameter @errp, Markus Armbruster, 2020/06/24
- [PATCH v2 12/25] aspeed: Clean up roundabout error propagation, Markus Armbruster, 2020/06/24
- [PATCH v2 09/25] sd/milkymist-memcard: Plug minor memory leak in realize, Markus Armbruster, 2020/06/24
- [PATCH v2 08/25] qga: Plug unlikely memory leak in guest-set-memory-blocks, Markus Armbruster, 2020/06/24
- [PATCH v2 20/25] x86: Fix x86_cpu_new() error API violations, Markus Armbruster, 2020/06/24
- [PATCH v2 06/25] usb/dev-mtp: Fix Error double free after inotify failure, Markus Armbruster, 2020/06/24
- [PATCH v2 17/25] riscv/sifive_u: Fix sifive_u_soc_realize() error API violations, Markus Armbruster, 2020/06/24
- [PATCH v2 19/25] mips/cps: Fix mips_cps_realize() error API violations, Markus Armbruster, 2020/06/24
- [PATCH v2 02/25] pci: Delete useless error_propagate(), Markus Armbruster, 2020/06/24
- [PATCH v2 01/25] net/virtio: Fix failover_replug_primary() return value regression, Markus Armbruster, 2020/06/24
- [PATCH v2 18/25] riscv_hart: Fix riscv_harts_realize() error API violations, Markus Armbruster, 2020/06/24
- [PATCH v2 11/25] vnc: Plug minor memory leak in vnc_display_open(), Markus Armbruster, 2020/06/24
- [PATCH v2 23/25] aspeed: Fix realize error API violation, Markus Armbruster, 2020/06/24
- [PATCH v2 25/25] arm/{bcm2835, fsl-imx25, fsl-imx6}: Fix realize error API violations, Markus Armbruster, 2020/06/24
- [PATCH v2 22/25] arm/stm32f205 arm/stm32f405: Fix realize error API violation, Markus Armbruster, 2020/06/24