[PATCH v1] migration: Use QAPI_CLONE_MEMBERS in migrate_params_test_apply

[PATCH v1] migration: Use QAPI_CLONE_MEMBERS in migrate_params_test_apply

[Fabiano Rosas]

Use QAPI_CLONE_MEMBERS instead of making an assignment. The QAPI
method makes the handling of the TLS strings more intuitive because it
clones them as well.

This also fixes a segfault when a NULL TLS option is accessed as part
of a validation check for another option (e.g. in the zero-copy +
multifd compression case). Details follow:

Currently, after copying s->parameters to the temporary
MigrationParameters object before migrate_params_check(), the
references in temporary object to the TLS options are dropped, either
because:

a) the user set a new option, in which case that's fine as
   s->parameters still holds the reference to the old memory or,

b) the user did not set a new option, in which case keeping the
   references in the temporary object would later cause them to be
   freed along with it, leading to double-free when s->parameters is
   also freed later on.

In this second case, it was overlooked that the TLS options can be
accessed already during migrate_params_check() as part of validation
of another option. Those pointers should not have been cleared.

Using QAPI_CLONE_MEMBERS fixes the issue because the temporary object
is not stealing a reference from s->parameters anymore.

Fixes: aed97f0563 ("migration: Normalize tls arguments")
Reported-by: Maciej S. Szmigiero <mail@maciej.szmigiero.name>
Link: https://lore.kernel.org/r/a65a1049-9f19-460a-8e27-a62bb30d2727@maciej.szmigiero.name
Reviewed-by: Peter Xu <peterx@redhat.com>
Signed-off-by: Fabiano Rosas <farosas@suse.de>
---
NOTE#1: For the release we could have a simpler fix just checking for
NULL, but that would allow two unsupported configurations to be
accepted: zero-copy with either multifd compression or TLS.

NOTE#2: CI is red due to pre-existing failure in functional tests with
"exec:socat" migration.
---
 migration/options.c | 26 ++++++++++++--------------
 1 file changed, 12 insertions(+), 14 deletions(-)

diff --git a/migration/options.c b/migration/options.c
index 7556fbc06b..68441f0276 100644
--- a/migration/options.c
+++ b/migration/options.c
@@ -1279,9 +1279,9 @@ bool migrate_params_check(MigrationParameters *params, Error **errp)
 static void migrate_params_test_apply(MigrationParameters *params,
                                       MigrationParameters *dest)
 {
-    *dest = migrate_get_current()->parameters;
+    MigrationState *s = migrate_get_current();
 
-    /* TODO use QAPI_CLONE() instead of duplicating it inline */
+    QAPI_CLONE_MEMBERS(MigrationParameters, dest, &s->parameters);
 
     if (params->has_throttle_trigger_threshold) {
         dest->throttle_trigger_threshold = params->throttle_trigger_threshold;
@@ -1300,24 +1300,18 @@ static void migrate_params_test_apply(MigrationParameters *params,
     }
 
     if (params->tls_creds) {
+        qapi_free_StrOrNull(dest->tls_creds);
         dest->tls_creds = QAPI_CLONE(StrOrNull, params->tls_creds);
-    } else {
-        /* clear the reference, it's owned by s->parameters */
-        dest->tls_creds = NULL;
     }
 
     if (params->tls_hostname) {
+        qapi_free_StrOrNull(dest->tls_hostname);
         dest->tls_hostname = QAPI_CLONE(StrOrNull, params->tls_hostname);
-    } else {
-        /* clear the reference, it's owned by s->parameters */
-        dest->tls_hostname = NULL;
     }
 
     if (params->tls_authz) {
+        qapi_free_StrOrNull(dest->tls_authz);
         dest->tls_authz = QAPI_CLONE(StrOrNull, params->tls_authz);
-    } else {
-        /* clear the reference, it's owned by s->parameters */
-        dest->tls_authz = NULL;
     }
 
     if (params->has_max_bandwidth) {
@@ -1374,8 +1368,9 @@ static void migrate_params_test_apply(MigrationParameters *params,
     }
 
     if (params->has_block_bitmap_mapping) {
-        dest->has_block_bitmap_mapping = true;
-        dest->block_bitmap_mapping = params->block_bitmap_mapping;
+        qapi_free_BitmapMigrationNodeAliasList(dest->block_bitmap_mapping);
+        dest->block_bitmap_mapping = QAPI_CLONE(BitmapMigrationNodeAliasList,
+                                                params->block_bitmap_mapping);
     }
 
     if (params->has_x_vcpu_dirty_limit_period) {
@@ -1399,7 +1394,8 @@ static void migrate_params_test_apply(MigrationParameters *params,
     }
 
     if (params->has_cpr_exec_command) {
-        dest->cpr_exec_command = params->cpr_exec_command;
+        qapi_free_strList(dest->cpr_exec_command);
+        dest->cpr_exec_command = QAPI_CLONE(strList, params->cpr_exec_command);
     }
 }
 
@@ -1555,4 +1551,6 @@ void qmp_migrate_set_parameters(MigrationParameters *params, Error **errp)
     }
 
     migrate_tls_opts_free(&tmp);
+    qapi_free_BitmapMigrationNodeAliasList(tmp.block_bitmap_mapping);
+    qapi_free_strList(tmp.cpr_exec_command);
 }
-- 
2.51.0

Re: [PATCH v1] migration: Use QAPI_CLONE_MEMBERS in migrate_params_test_apply

[Maciej S. Szmigiero]

On 15.04.2026 00:37, Fabiano Rosas wrote:
> Use QAPI_CLONE_MEMBERS instead of making an assignment. The QAPI
> method makes the handling of the TLS strings more intuitive because it
> clones them as well.
> 
> This also fixes a segfault when a NULL TLS option is accessed as part
> of a validation check for another option (e.g. in the zero-copy +
> multifd compression case). Details follow:
> 
> Currently, after copying s->parameters to the temporary
> MigrationParameters object before migrate_params_check(), the
> references in temporary object to the TLS options are dropped, either
> because:
> 
> a) the user set a new option, in which case that's fine as
>     s->parameters still holds the reference to the old memory or,
> 
> b) the user did not set a new option, in which case keeping the
>     references in the temporary object would later cause them to be
>     freed along with it, leading to double-free when s->parameters is
>     also freed later on.
> 
> In this second case, it was overlooked that the TLS options can be
> accessed already during migrate_params_check() as part of validation
> of another option. Those pointers should not have been cleared.
> 
> Using QAPI_CLONE_MEMBERS fixes the issue because the temporary object
> is not stealing a reference from s->parameters anymore.
> 
> Fixes: aed97f0563 ("migration: Normalize tls arguments")
> Reported-by: Maciej S. Szmigiero <mail@maciej.szmigiero.name>
> Link: https://lore.kernel.org/r/a65a1049-9f19-460a-8e27-a62bb30d2727@maciej.szmigiero.name
> Reviewed-by: Peter Xu <peterx@redhat.com>
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
> NOTE#1: For the release we could have a simpler fix just checking for
> NULL, but that would allow two unsupported configurations to be
> accepted: zero-copy with either multifd compression or TLS.
> 
> NOTE#2: CI is red due to pre-existing failure in functional tests with
> "exec:socat" migration.
> ---

Thanks for the quick fix Fabiano.

The patch seems to fix the issue, so:
Tested-by: Maciej S. Szmigiero <maciej.szmigiero@oracle.com>

Thanks,
Maciej

Re: [PATCH v1] migration: Use QAPI_CLONE_MEMBERS in migrate_params_test_apply

[Fabiano Rosas]

Fabiano Rosas <farosas@suse.de> writes:

> Use QAPI_CLONE_MEMBERS instead of making an assignment. The QAPI
> method makes the handling of the TLS strings more intuitive because it
> clones them as well.
>
> This also fixes a segfault when a NULL TLS option is accessed as part
> of a validation check for another option (e.g. in the zero-copy +
> multifd compression case). Details follow:
>
> Currently, after copying s->parameters to the temporary
> MigrationParameters object before migrate_params_check(), the
> references in temporary object to the TLS options are dropped, either
> because:
>
> a) the user set a new option, in which case that's fine as
>    s->parameters still holds the reference to the old memory or,
>
> b) the user did not set a new option, in which case keeping the
>    references in the temporary object would later cause them to be
>    freed along with it, leading to double-free when s->parameters is
>    also freed later on.
>
> In this second case, it was overlooked that the TLS options can be
> accessed already during migrate_params_check() as part of validation
> of another option. Those pointers should not have been cleared.
>
> Using QAPI_CLONE_MEMBERS fixes the issue because the temporary object
> is not stealing a reference from s->parameters anymore.
>
> Fixes: aed97f0563 ("migration: Normalize tls arguments")
> Reported-by: Maciej S. Szmigiero <mail@maciej.szmigiero.name>
> Link: https://lore.kernel.org/r/a65a1049-9f19-460a-8e27-a62bb30d2727@maciej.szmigiero.name
> Reviewed-by: Peter Xu <peterx@redhat.com>
> Signed-off-by: Fabiano Rosas <farosas@suse.de>
> ---
> NOTE#1: For the release we could have a simpler fix just checking for
> NULL, but that would allow two unsupported configurations to be
> accepted: zero-copy with either multifd compression or TLS.
>

Peter Xu just pointed out on IRC that it's actually only the zero-copy +
TLS configuration that will be allowed with the simpler fix. I think
it's best we go that route instead and hold this patch until after the
release.

Re: [PATCH v1] migration: Use QAPI_CLONE_MEMBERS in migrate_params_test_apply

[Peter Maydell]

On Wed, 15 Apr 2026 at 15:39, Fabiano Rosas <farosas@suse.de> wrote:
>
> Fabiano Rosas <farosas@suse.de> writes:
>
> > Use QAPI_CLONE_MEMBERS instead of making an assignment. The QAPI
> > method makes the handling of the TLS strings more intuitive because it
> > clones them as well.
> >
> > This also fixes a segfault when a NULL TLS option is accessed as part
> > of a validation check for another option (e.g. in the zero-copy +
> > multifd compression case). Details follow:
> >
> > Currently, after copying s->parameters to the temporary
> > MigrationParameters object before migrate_params_check(), the
> > references in temporary object to the TLS options are dropped, either
> > because:
> >
> > a) the user set a new option, in which case that's fine as
> >    s->parameters still holds the reference to the old memory or,
> >
> > b) the user did not set a new option, in which case keeping the
> >    references in the temporary object would later cause them to be
> >    freed along with it, leading to double-free when s->parameters is
> >    also freed later on.
> >
> > In this second case, it was overlooked that the TLS options can be
> > accessed already during migrate_params_check() as part of validation
> > of another option. Those pointers should not have been cleared.
> >
> > Using QAPI_CLONE_MEMBERS fixes the issue because the temporary object
> > is not stealing a reference from s->parameters anymore.
> >
> > Fixes: aed97f0563 ("migration: Normalize tls arguments")
> > Reported-by: Maciej S. Szmigiero <mail@maciej.szmigiero.name>
> > Link: https://lore.kernel.org/r/a65a1049-9f19-460a-8e27-a62bb30d2727@maciej.szmigiero.name
> > Reviewed-by: Peter Xu <peterx@redhat.com>
> > Signed-off-by: Fabiano Rosas <farosas@suse.de>
> > ---
> > NOTE#1: For the release we could have a simpler fix just checking for
> > NULL, but that would allow two unsupported configurations to be
> > accepted: zero-copy with either multifd compression or TLS.
> >
>
> Peter Xu just pointed out on IRC that it's actually only the zero-copy +
> TLS configuration that will be allowed with the simpler fix. I think
> it's best we go that route instead and hold this patch until after the
> release.

rc4 has already been tagged and I really don't want to put anything
more into 11.0 unless it is a super high priority release-blocker
kind of a bug. The fact that this is a fix for a commit that's
been in git for pretty much the whole of the 11.0 cycle without anybody
noticing earlier suggests it probably isn't that important ?

If we want to put it in then the commit message needs to clearly
state the severity (e.g. "everybody trying to use migration
hits a segfault") that makes it release-critical.

Otherwise we should put this into 11.1 and with the usual cc:stable
so it is backported to the 11.0 stable series.

thanks
-- PMM

Re: [PATCH v1] migration: Use QAPI_CLONE_MEMBERS in migrate_params_test_apply

[Fabiano Rosas]

Peter Maydell <peter.maydell@linaro.org> writes:

> On Wed, 15 Apr 2026 at 15:39, Fabiano Rosas <farosas@suse.de> wrote:
>>
>> Fabiano Rosas <farosas@suse.de> writes:
>>
>> > Use QAPI_CLONE_MEMBERS instead of making an assignment. The QAPI
>> > method makes the handling of the TLS strings more intuitive because it
>> > clones them as well.
>> >
>> > This also fixes a segfault when a NULL TLS option is accessed as part
>> > of a validation check for another option (e.g. in the zero-copy +
>> > multifd compression case). Details follow:
>> >
>> > Currently, after copying s->parameters to the temporary
>> > MigrationParameters object before migrate_params_check(), the
>> > references in temporary object to the TLS options are dropped, either
>> > because:
>> >
>> > a) the user set a new option, in which case that's fine as
>> >    s->parameters still holds the reference to the old memory or,
>> >
>> > b) the user did not set a new option, in which case keeping the
>> >    references in the temporary object would later cause them to be
>> >    freed along with it, leading to double-free when s->parameters is
>> >    also freed later on.
>> >
>> > In this second case, it was overlooked that the TLS options can be
>> > accessed already during migrate_params_check() as part of validation
>> > of another option. Those pointers should not have been cleared.
>> >
>> > Using QAPI_CLONE_MEMBERS fixes the issue because the temporary object
>> > is not stealing a reference from s->parameters anymore.
>> >
>> > Fixes: aed97f0563 ("migration: Normalize tls arguments")
>> > Reported-by: Maciej S. Szmigiero <mail@maciej.szmigiero.name>
>> > Link: https://lore.kernel.org/r/a65a1049-9f19-460a-8e27-a62bb30d2727@maciej.szmigiero.name
>> > Reviewed-by: Peter Xu <peterx@redhat.com>
>> > Signed-off-by: Fabiano Rosas <farosas@suse.de>
>> > ---
>> > NOTE#1: For the release we could have a simpler fix just checking for
>> > NULL, but that would allow two unsupported configurations to be
>> > accepted: zero-copy with either multifd compression or TLS.
>> >
>>
>> Peter Xu just pointed out on IRC that it's actually only the zero-copy +
>> TLS configuration that will be allowed with the simpler fix. I think
>> it's best we go that route instead and hold this patch until after the
>> release.
>
> rc4 has already been tagged and I really don't want to put anything
> more into 11.0 unless it is a super high priority release-blocker
> kind of a bug. The fact that this is a fix for a commit that's
> been in git for pretty much the whole of the 11.0 cycle without anybody
> noticing earlier suggests it probably isn't that important ?
>
> If we want to put it in then the commit message needs to clearly
> state the severity (e.g. "everybody trying to use migration
> hits a segfault") that makes it release-critical.
>
> Otherwise we should put this into 11.1 and with the usual cc:stable
> so it is backported to the 11.0 stable series.
>

Makes sense.

@Peter Xu, do you agree with leaving this to stable? Given that
it needs multifd and zero-copy-send to be both set, I'd say it doesn't
have that large of a user base.

@Maciej, what's the impact for you?

Re: [PATCH v1] migration: Use QAPI_CLONE_MEMBERS in migrate_params_test_apply

[Maciej S. Szmigiero]

On 16.04.2026 14:19, Fabiano Rosas wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
> 
>> On Wed, 15 Apr 2026 at 15:39, Fabiano Rosas <farosas@suse.de> wrote:
>>>
>>> Fabiano Rosas <farosas@suse.de> writes:
>>>
>>>> Use QAPI_CLONE_MEMBERS instead of making an assignment. The QAPI
>>>> method makes the handling of the TLS strings more intuitive because it
>>>> clones them as well.
>>>>
>>>> This also fixes a segfault when a NULL TLS option is accessed as part
>>>> of a validation check for another option (e.g. in the zero-copy +
>>>> multifd compression case). Details follow:
>>>>
>>>> Currently, after copying s->parameters to the temporary
>>>> MigrationParameters object before migrate_params_check(), the
>>>> references in temporary object to the TLS options are dropped, either
>>>> because:
>>>>
>>>> a) the user set a new option, in which case that's fine as
>>>>     s->parameters still holds the reference to the old memory or,
>>>>
>>>> b) the user did not set a new option, in which case keeping the
>>>>     references in the temporary object would later cause them to be
>>>>     freed along with it, leading to double-free when s->parameters is
>>>>     also freed later on.
>>>>
>>>> In this second case, it was overlooked that the TLS options can be
>>>> accessed already during migrate_params_check() as part of validation
>>>> of another option. Those pointers should not have been cleared.
>>>>
>>>> Using QAPI_CLONE_MEMBERS fixes the issue because the temporary object
>>>> is not stealing a reference from s->parameters anymore.
>>>>
>>>> Fixes: aed97f0563 ("migration: Normalize tls arguments")
>>>> Reported-by: Maciej S. Szmigiero <mail@maciej.szmigiero.name>
>>>> Link: https://lore.kernel.org/r/a65a1049-9f19-460a-8e27-a62bb30d2727@maciej.szmigiero.name
>>>> Reviewed-by: Peter Xu <peterx@redhat.com>
>>>> Signed-off-by: Fabiano Rosas <farosas@suse.de>
>>>> ---
>>>> NOTE#1: For the release we could have a simpler fix just checking for
>>>> NULL, but that would allow two unsupported configurations to be
>>>> accepted: zero-copy with either multifd compression or TLS.
>>>>
>>>
>>> Peter Xu just pointed out on IRC that it's actually only the zero-copy +
>>> TLS configuration that will be allowed with the simpler fix. I think
>>> it's best we go that route instead and hold this patch until after the
>>> release.
>>
>> rc4 has already been tagged and I really don't want to put anything
>> more into 11.0 unless it is a super high priority release-blocker
>> kind of a bug. The fact that this is a fix for a commit that's
>> been in git for pretty much the whole of the 11.0 cycle without anybody
>> noticing earlier suggests it probably isn't that important ?
>>
>> If we want to put it in then the commit message needs to clearly
>> state the severity (e.g. "everybody trying to use migration
>> hits a segfault") that makes it release-critical.
>>
>> Otherwise we should put this into 11.1 and with the usual cc:stable
>> so it is backported to the 11.0 stable series.
>>
> 
> Makes sense.
> 
> @Peter Xu, do you agree with leaving this to stable? Given that
> it needs multifd and zero-copy-send to be both set, I'd say it doesn't
> have that large of a user base.
> 
> @Maciej, what's the impact for you?

Minimal - I can just run my tests with patched QEMU.

Thanks,
Maciej

Re: [PATCH v1] migration: Use QAPI_CLONE_MEMBERS in migrate_params_test_apply

[Peter Xu]

On Thu, Apr 16, 2026 at 09:19:51AM -0300, Fabiano Rosas wrote:
> @Peter Xu, do you agree with leaving this to stable? Given that
> it needs multifd and zero-copy-send to be both set, I'd say it doesn't
> have that large of a user base.

Yes.

-- 
Peter Xu

Re: [PATCH v1] migration: Use QAPI_CLONE_MEMBERS in migrate_params_test_apply

[Peter Xu]

On Tue, Apr 14, 2026 at 07:37:18PM -0300, Fabiano Rosas wrote:
> Use QAPI_CLONE_MEMBERS instead of making an assignment. The QAPI
> method makes the handling of the TLS strings more intuitive because it
> clones them as well.
> 
> This also fixes a segfault when a NULL TLS option is accessed as part
> of a validation check for another option (e.g. in the zero-copy +
> multifd compression case). Details follow:
> 
> Currently, after copying s->parameters to the temporary
> MigrationParameters object before migrate_params_check(), the
> references in temporary object to the TLS options are dropped, either
> because:
> 
> a) the user set a new option, in which case that's fine as
>    s->parameters still holds the reference to the old memory or,
> 
> b) the user did not set a new option, in which case keeping the
>    references in the temporary object would later cause them to be
>    freed along with it, leading to double-free when s->parameters is
>    also freed later on.
> 
> In this second case, it was overlooked that the TLS options can be
> accessed already during migrate_params_check() as part of validation
> of another option. Those pointers should not have been cleared.
> 
> Using QAPI_CLONE_MEMBERS fixes the issue because the temporary object
> is not stealing a reference from s->parameters anymore.
> 
> Fixes: aed97f0563 ("migration: Normalize tls arguments")
> Reported-by: Maciej S. Szmigiero <mail@maciej.szmigiero.name>
> Link: https://lore.kernel.org/r/a65a1049-9f19-460a-8e27-a62bb30d2727@maciej.szmigiero.name
> Reviewed-by: Peter Xu <peterx@redhat.com>
> Signed-off-by: Fabiano Rosas <farosas@suse.de>

Queued and added cc:stable.

-- 
Peter Xu