Re: [PATCH v11 05/10] migration: convert exec backend to accept MigrateAddress.

[Fabiano Rosas <farosas@suse.de>]

Het Gala <het.gala@nutanix.com> writes:

> Exec transport backend for 'migrate'/'migrate-incoming' QAPIs accept
> new wire protocol of MigrateAddress struct.
>
> It is achived by parsing 'uri' string and storing migration parameters
> required for exec connection into strList struct.
>
> Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com>
> Signed-off-by: Het Gala <het.gala@nutanix.com>
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  migration/exec.c      | 71 +++++++++++++++++++++++++++++++------------
>  migration/exec.h      |  4 +--
>  migration/migration.c | 10 +++---
>  3 files changed, 57 insertions(+), 28 deletions(-)
>
> diff --git a/migration/exec.c b/migration/exec.c
> index 32f5143dfd..8bc321c66b 100644
> --- a/migration/exec.c
> +++ b/migration/exec.c
> @@ -39,20 +39,50 @@ const char *exec_get_cmd_path(void)
>  }
>  #endif
>  
> -void exec_start_outgoing_migration(MigrationState *s, const char *command, Error **errp)
> +/* provides the length of strList */
> +static int
> +str_list_length(strList *list)
> +{
> +    int len = 0;
> +    strList *elem;
> +
> +    for (elem = list; elem != NULL; elem = elem->next) {
> +        len++;
> +    }
> +
> +    return len;
> +}
> +
> +static void
> +init_exec_array(strList *command, char **argv, Error **errp)
> +{
> +    int i = 0;
> +    strList *lst;
> +
> +    for (lst = command; lst; lst = lst->next) {
> +        argv[i++] = lst->value;
> +    }
> +
> +    argv[i] = NULL;

This will write out of bounds. 

> +    return;
> +}
> +
> +void exec_start_outgoing_migration(MigrationState *s, strList *command,
> +                                   Error **errp)
>  {
>      QIOChannel *ioc;
>  
> -#ifdef WIN32
> -    const char *argv[] = { exec_get_cmd_path(), "/c", command, NULL };
> -#else
> -    const char *argv[] = { "/bin/sh", "-c", command, NULL };
> -#endif
> +    int length = str_list_length(command);
> +    g_auto(GStrv) argv = (char **) g_new0(const char *, length);

This allocation does not leave space for the NULL byte.

>  
> -    trace_migration_exec_outgoing(command);
> -    ioc = QIO_CHANNEL(qio_channel_command_new_spawn(argv,
> -                                                    O_RDWR,
> -                                                    errp));
> +    init_exec_array(command, argv, errp);
> +    g_autofree char *new_command = g_strjoinv(" ", (char **)argv);
> +
> +    trace_migration_exec_outgoing(new_command);
> +    ioc = QIO_CHANNEL(
> +        qio_channel_command_new_spawn((const char * const *) argv,
> +                                      O_RDWR,
> +                                      errp));
>      if (!ioc) {
>          return;
>      }
> @@ -71,20 +101,21 @@ static gboolean exec_accept_incoming_migration(QIOChannel *ioc,
>      return G_SOURCE_REMOVE;
>  }
>  
> -void exec_start_incoming_migration(const char *command, Error **errp)
> +void exec_start_incoming_migration(strList *command, Error **errp)
>  {
>      QIOChannel *ioc;
>  
> -#ifdef WIN32
> -    const char *argv[] = { exec_get_cmd_path(), "/c", command, NULL };
> -#else
> -    const char *argv[] = { "/bin/sh", "-c", command, NULL };
> -#endif
> +    int length = str_list_length(command);
> +    g_auto(GStrv) argv = (char **) g_new0(const char *, length);

Here as well.

> +
> +    init_exec_array(command, argv, errp);
> +    g_autofree char *new_command = g_strjoinv(" ", (char **)argv);
>  
> -    trace_migration_exec_incoming(command);
> -    ioc = QIO_CHANNEL(qio_channel_command_new_spawn(argv,
> -                                                    O_RDWR,
> -                                                    errp));
> +    trace_migration_exec_incoming(new_command);
> +    ioc = QIO_CHANNEL(
> +        qio_channel_command_new_spawn((const char * const *) argv,
> +                                      O_RDWR,
> +                                      errp));
>      if (!ioc) {
>          return;
>      }
> diff --git a/migration/exec.h b/migration/exec.h
> index 736cd71028..3107f205e3 100644
> --- a/migration/exec.h
> +++ b/migration/exec.h
> @@ -23,8 +23,8 @@
>  #ifdef WIN32
>  const char *exec_get_cmd_path(void);
>  #endif
> -void exec_start_incoming_migration(const char *host_port, Error **errp);
> +void exec_start_incoming_migration(strList *host_port, Error **errp);
>  
> -void exec_start_outgoing_migration(MigrationState *s, const char *host_port,
> +void exec_start_outgoing_migration(MigrationState *s, strList *host_port,
>                                     Error **errp);
>  #endif
> diff --git a/migration/migration.c b/migration/migration.c
> index b41fda6f80..ebe14b9c38 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -474,7 +474,6 @@ static bool migrate_uri_parse(const char *uri,
>  
>  static void qemu_start_incoming_migration(const char *uri, Error **errp)
>  {
> -    const char *p = NULL;
>      g_autoptr(MigrationAddress) channel = g_new0(MigrationAddress, 1);
>  
>      /* URI is not suitable for migration? */
> @@ -500,8 +499,8 @@ static void qemu_start_incoming_migration(const char *uri, Error **errp)
>      } else if (channel->transport == MIGRATION_ADDRESS_TYPE_RDMA) {
>          rdma_start_incoming_migration(&channel->u.rdma, errp);
>  #endif
> -    } else if (strstart(uri, "exec:", &p)) {
> -        exec_start_incoming_migration(p, errp);
> +    } else if (channel->transport == MIGRATION_ADDRESS_TYPE_EXEC) {
> +        exec_start_incoming_migration(channel->u.exec.args, errp);
>      } else {
>          error_setg(errp, "unknown migration protocol: %s", uri);
>      }
> @@ -1723,7 +1722,6 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
>      bool resume_requested;
>      Error *local_err = NULL;
>      MigrationState *s = migrate_get_current();
> -    const char *p = NULL;
>      g_autoptr(MigrationAddress) channel = g_new0(MigrationAddress, 1);
>  
>      /* URI is not suitable for migration? */
> @@ -1761,8 +1759,8 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
>      } else if (channel->transport == MIGRATION_ADDRESS_TYPE_RDMA) {
>          rdma_start_outgoing_migration(s, &channel->u.rdma, &local_err);
>  #endif
> -    } else if (strstart(uri, "exec:", &p)) {
> -        exec_start_outgoing_migration(s, p, &local_err);
> +    } else if (channel->transport == MIGRATION_ADDRESS_TYPE_EXEC) {
> +        exec_start_outgoing_migration(s, channel->u.exec.args, &local_err);
>      } else {
>          error_setg(&local_err, QERR_INVALID_PARAMETER_VALUE, "uri",
>                     "a valid migration protocol");