Re: [PATCH v3 20/26] x86: Fix x86_cpu_new() error handling
On 02/07/20 06:51, Markus Armbruster wrote: > Igor, Paolo, you showed me the error in v2. Could you have a look at > this revision? > > Markus Armbruster writes: > >> The Error ** argument must be NULL, _abort, _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 _err to >> object_property_set_uint() without checking it, and then to >> qdev_realize(). If both fail, we'll trip error_setv()'s assertion. >> To assess the bug's impact, we'd need to figure out how to make both >> calls fail. Too much work for ignorant me, sorry. >> >> Fix by checking for failure right away. >> >> Cc: Igor Mammedov >> Cc: Paolo Bonzini >> Cc: Richard Henderson >> Cc: Eduardo Habkost >> Signed-off-by: Markus Armbruster >> --- >> hw/i386/x86.c | 8 +--- >> 1 file changed, 5 insertions(+), 3 deletions(-) >> >> diff --git a/hw/i386/x86.c b/hw/i386/x86.c >> index 34229b45c7..93f7371a56 100644 >> --- a/hw/i386/x86.c >> +++ b/hw/i386/x86.c >> @@ -118,14 +118,16 @@ 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; >> - >> -cpu = object_new(MACHINE(x86ms)->cpu_type); >> +Object *cpu = object_new(MACHINE(x86ms)->cpu_type); >> >> object_property_set_uint(cpu, apic_id, "apic-id", _err); >> +if (local_err) { >> +goto out; >> +} >> qdev_realize(DEVICE(cpu), NULL, _err); >> >> +out: >> object_unref(cpu); >> error_propagate(errp, local_err); >> } > Yes, this looks good. Thanks! Reviewed-by: Paolo Bonzini Paolo
Re: [PATCH v3 20/26] x86: Fix x86_cpu_new() error handling
Igor, Paolo, you showed me the error in v2. Could you have a look at this revision? Markus Armbruster writes: > The Error ** argument must be NULL, _abort, _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 _err to > object_property_set_uint() without checking it, and then to > qdev_realize(). If both fail, we'll trip error_setv()'s assertion. > To assess the bug's impact, we'd need to figure out how to make both > calls fail. Too much work for ignorant me, sorry. > > Fix by checking for failure right away. > > Cc: Igor Mammedov > Cc: Paolo Bonzini > Cc: Richard Henderson > Cc: Eduardo Habkost > Signed-off-by: Markus Armbruster > --- > hw/i386/x86.c | 8 +--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/hw/i386/x86.c b/hw/i386/x86.c > index 34229b45c7..93f7371a56 100644 > --- a/hw/i386/x86.c > +++ b/hw/i386/x86.c > @@ -118,14 +118,16 @@ 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; > - > -cpu = object_new(MACHINE(x86ms)->cpu_type); > +Object *cpu = object_new(MACHINE(x86ms)->cpu_type); > > object_property_set_uint(cpu, apic_id, "apic-id", _err); > +if (local_err) { > +goto out; > +} > qdev_realize(DEVICE(cpu), NULL, _err); > > +out: > object_unref(cpu); > error_propagate(errp, local_err); > }
[PATCH v3 20/26] x86: Fix x86_cpu_new() error handling
The Error ** argument must be NULL, _abort, _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 _err to object_property_set_uint() without checking it, and then to qdev_realize(). If both fail, we'll trip error_setv()'s assertion. To assess the bug's impact, we'd need to figure out how to make both calls fail. Too much work for ignorant me, sorry. Fix by checking for failure right away. Cc: Igor Mammedov Cc: Paolo Bonzini Cc: Richard Henderson Cc: Eduardo Habkost Signed-off-by: Markus Armbruster --- hw/i386/x86.c | 8 +--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/hw/i386/x86.c b/hw/i386/x86.c index 34229b45c7..93f7371a56 100644 --- a/hw/i386/x86.c +++ b/hw/i386/x86.c @@ -118,14 +118,16 @@ 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; - -cpu = object_new(MACHINE(x86ms)->cpu_type); +Object *cpu = object_new(MACHINE(x86ms)->cpu_type); object_property_set_uint(cpu, apic_id, "apic-id", _err); +if (local_err) { +goto out; +} qdev_realize(DEVICE(cpu), NULL, _err); +out: object_unref(cpu); error_propagate(errp, local_err); } -- 2.26.2