Re: [PATCH] migration: Don't free the reason after calling migrate_add_blocker

[Markus Armbruster <armbru@redhat.com>]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Fri, Oct 24, 2025 at 01:15:40PM +0200, Markus Armbruster wrote:
>> Bin Guo <guobin@linux.alibaba.com> writes:
>> 
>> > Signed-off-by: Bin Guo <guobin@linux.alibaba.com>
>> > ---
>> >  hw/intc/arm_gicv3_kvm.c | 1 -
>> >  target/i386/sev.c       | 1 -
>> >  2 files changed, 2 deletions(-)
>> >
>> > diff --git a/hw/intc/arm_gicv3_kvm.c b/hw/intc/arm_gicv3_kvm.c
>> > index 66b0dddfd4..6f311e37ef 100644
>> > --- a/hw/intc/arm_gicv3_kvm.c
>> > +++ b/hw/intc/arm_gicv3_kvm.c
>> > @@ -841,7 +841,6 @@ static void kvm_arm_gicv3_realize(DeviceState *dev, Error **errp)
>> >          error_setg(&kvm_nv_migration_blocker,
>> >                     "Live migration disabled because KVM nested virt is enabled");
>> >          if (migrate_add_blocker(&kvm_nv_migration_blocker, errp)) {
>> > -            error_free(kvm_nv_migration_blocker);
>> >              return;
>> >          }
>> >  
>> > diff --git a/target/i386/sev.c b/target/i386/sev.c
>> > index 1057b8ab2c..fd2dada013 100644
>> > --- a/target/i386/sev.c
>> > +++ b/target/i386/sev.c
>> > @@ -1661,7 +1661,6 @@ sev_snp_launch_finish(SevCommonState *sev_common)
>> >      ret = migrate_add_blocker(&sev_mig_blocker, &local_err);
>> >      if (local_err) {
>> >          error_report_err(local_err);
>> > -        error_free(sev_mig_blocker);
>> >          exit(1);
>> >      }
>> >  }
>> 
>> Does this fix use-after-free bugs?
>
> I don't think so, because when migrate_add_blocker() returns error,
> the Error for the blocker will have been propagated into the errp
> parameter, and then set to NULL. So these two error_free calls
> should be a no-op.
>
> But wow, the migrate_add_blocker API design is unpleasant with its
> pair of "Error **" parameters - it is practically designed to
> maximise confusion & surprise.

It's quite a sight, isn't it?

I'll give it a quick Friday afternoon try.