diff mbox series

[v3,1/4] migration/qmp: Fix crash on setting tls-authz with null

Message ID 20230905162335.235619-2-peterx@redhat.com
State New
Headers show
Series qapi/migration: Dedup migration parameter objects and fix tls-authz crash | expand

Commit Message

Peter Xu Sept. 5, 2023, 4:23 p.m. UTC
QEMU will crash if anyone tries to set tls-authz (which is a type
StrOrNull) with 'null' value.  Fix it in the easy way by converting it to
qstring just like the other two tls parameters.

Cc: qemu-stable@nongnu.org # v4.0+
Fixes: d2f1d29b95 ("migration: add support for a "tls-authz" migration parameter")
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Reviewed-by: Fabiano Rosas <farosas@suse.de>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/options.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

Comments

Michael Tokarev Sept. 28, 2023, 4:47 a.m. UTC | #1
05.09.2023 19:23, Peter Xu:
> QEMU will crash if anyone tries to set tls-authz (which is a type
> StrOrNull) with 'null' value.  Fix it in the easy way by converting it to
> qstring just like the other two tls parameters.

Ping?  Has this been forgotten?

Thanks,

/mjt

> Cc: qemu-stable@nongnu.org # v4.0+
> Fixes: d2f1d29b95 ("migration: add support for a "tls-authz" migration parameter")
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> Reviewed-by: Fabiano Rosas <farosas@suse.de>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>   migration/options.c | 9 +++++++--
>   1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/migration/options.c b/migration/options.c
> index 1d1e1321b0..6bbfd4853d 100644
> --- a/migration/options.c
> +++ b/migration/options.c
> @@ -1408,20 +1408,25 @@ void qmp_migrate_set_parameters(MigrateSetParameters *params, Error **errp)
>   {
>       MigrationParameters tmp;
>   
> -    /* TODO Rewrite "" to null instead */
> +    /* TODO Rewrite "" to null instead for all three tls_* parameters */
>       if (params->tls_creds
>           && params->tls_creds->type == QTYPE_QNULL) {
>           qobject_unref(params->tls_creds->u.n);
>           params->tls_creds->type = QTYPE_QSTRING;
>           params->tls_creds->u.s = strdup("");
>       }
> -    /* TODO Rewrite "" to null instead */
>       if (params->tls_hostname
>           && params->tls_hostname->type == QTYPE_QNULL) {
>           qobject_unref(params->tls_hostname->u.n);
>           params->tls_hostname->type = QTYPE_QSTRING;
>           params->tls_hostname->u.s = strdup("");
>       }
> +    if (params->tls_authz
> +        && params->tls_authz->type == QTYPE_QNULL) {
> +        qobject_unref(params->tls_authz->u.n);
> +        params->tls_authz->type = QTYPE_QSTRING;
> +        params->tls_authz->u.s = strdup("");
> +    }
>   
>       migrate_params_test_apply(params, &tmp);
>
Markus Armbruster Sept. 28, 2023, 5:36 a.m. UTC | #2
Michael Tokarev <mjt@tls.msk.ru> writes:

> 05.09.2023 19:23, Peter Xu:
>> QEMU will crash if anyone tries to set tls-authz (which is a type
>> StrOrNull) with 'null' value.  Fix it in the easy way by converting it to
>> qstring just like the other two tls parameters.
>
> Ping?  Has this been forgotten?

Obviously not: review of the series is ongoing.  PATCH 1+2 look ready,
though, and could be expedited.
Michael Tokarev Sept. 28, 2023, 6:56 a.m. UTC | #3
28.09.2023 08:36, Markus Armbruster wrote:
> Michael Tokarev <mjt@tls.msk.ru> writes:
> 
>> 05.09.2023 19:23, Peter Xu:
>>> QEMU will crash if anyone tries to set tls-authz (which is a type
>>> StrOrNull) with 'null' value.  Fix it in the easy way by converting it to
>>> qstring just like the other two tls parameters.
>>
>> Ping?  Has this been forgotten?
> 
> Obviously not: review of the series is ongoing.  PATCH 1+2 look ready,
> though, and could be expedited.

Yeah, obviously.  Please excuse my morning slow brain in -ENOCOFFEE
mode :)  I realized it is a part of patch series ([PATCH 1/4] in the
subject) only after sending this ping out - was looking at what's
left in my qemu-stable mailbox, and this single patch was there in
isolation from the rest of the series.  D'oh :)

/mjt
Juan Quintela Oct. 4, 2023, 1:58 p.m. UTC | #4
Peter Xu <peterx@redhat.com> wrote:
> QEMU will crash if anyone tries to set tls-authz (which is a type
> StrOrNull) with 'null' value.  Fix it in the easy way by converting it to
> qstring just like the other two tls parameters.
>
> Cc: qemu-stable@nongnu.org # v4.0+
> Fixes: d2f1d29b95 ("migration: add support for a "tls-authz" migration parameter")
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> Reviewed-by: Fabiano Rosas <farosas@suse.de>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Signed-off-by: Peter Xu <peterx@redhat.com>

Reviewed-by: Juan Quintela <quintela@redhat.com>

queued.
Markus Armbruster Oct. 16, 2023, 6:05 a.m. UTC | #5
Advice for next time...

Peter Xu <peterx@redhat.com> writes:

> QEMU will crash if anyone tries to set tls-authz (which is a type
> StrOrNull) with 'null' value.  Fix it in the easy way by converting it to
> qstring just like the other two tls parameters.

Include a reproducer whenever practical.  Helps backporters.  It would
have been practical here:

  QEMU will crash if anyone tries to set tls-authz (which is a type
  StrOrNull) with 'null' value.  Reproducer:

      {"execute": "migrate-set-parameters", "arguments": {"tls-authz": null}}

  Fix it in the easy way by converting it to qstring just like the other
  two tls parameters.

[...]
diff mbox series

Patch

diff --git a/migration/options.c b/migration/options.c
index 1d1e1321b0..6bbfd4853d 100644
--- a/migration/options.c
+++ b/migration/options.c
@@ -1408,20 +1408,25 @@  void qmp_migrate_set_parameters(MigrateSetParameters *params, Error **errp)
 {
     MigrationParameters tmp;
 
-    /* TODO Rewrite "" to null instead */
+    /* TODO Rewrite "" to null instead for all three tls_* parameters */
     if (params->tls_creds
         && params->tls_creds->type == QTYPE_QNULL) {
         qobject_unref(params->tls_creds->u.n);
         params->tls_creds->type = QTYPE_QSTRING;
         params->tls_creds->u.s = strdup("");
     }
-    /* TODO Rewrite "" to null instead */
     if (params->tls_hostname
         && params->tls_hostname->type == QTYPE_QNULL) {
         qobject_unref(params->tls_hostname->u.n);
         params->tls_hostname->type = QTYPE_QSTRING;
         params->tls_hostname->u.s = strdup("");
     }
+    if (params->tls_authz
+        && params->tls_authz->type == QTYPE_QNULL) {
+        qobject_unref(params->tls_authz->u.n);
+        params->tls_authz->type = QTYPE_QSTRING;
+        params->tls_authz->u.s = strdup("");
+    }
 
     migrate_params_test_apply(params, &tmp);