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

[Het Gala]

On 10/4/2023 8:25 PM, Fabiano Rosas wrote:

> 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(-)
> >
> > -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.
>     
    Yes, will increase the length of argv in exec_start_outgoing_migration() by one, that would solve the issue right. 
> > +    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.
>

Yes you are rigght. I assumed the user will itself have to end the list of argument with a 'NULL', but that's not correct. Thanks for pointing it out. Will make the length of argv from length -> length+1.

> >  
> > -    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.
>     
    Ack, will increase length of argv by one while initalization.

> >       
>     
    Regards,
    Het Gala