Re: [PATCH v2 4/6] migration: Avoid multiple parsing of uri in migration code flow

["Daniel P. Berrangé" <berrange@redhat.com>]

On Thu, Feb 09, 2023 at 07:24:48PM +0530, Het Gala wrote:
> 
> On 09/02/23 5:39 pm, Daniel P. Berrangé wrote:
> > On Wed, Feb 08, 2023 at 09:35:58AM +0000, Het Gala wrote:
> > > In existing senario, 'migrate' QAPI argument - string uri, is encoded
> > > twice to extract migration parameters for stream connection. This is
> > > not a good representation of migration wire protocol as it is a data
> > > encoding scheme within a data encoding scheme. Qemu should be able to
> > > directly work with results from QAPI without having to do a second
> > > level parsing.
> > > Modified 'migrate' QAPI design supports well defined MigrateChannel
> > > struct which plays important role in avoiding double encoding
> > > of uri strings.
> > > 
> > > qemu_uri_parsing() parses uri string (kept for backward
> > > compatibility) and populate the MigrateChannel struct parameters.
> > > Migration code flow for all required migration transport types -
> > > socket, exec and rdma is modified.
> > > 
> > > Suggested-by: Daniel P. Berrange<berrange@redhat.com>
> > > Suggested-by: Manish Mishra<manish.mishra@nutanix.com>
> > > Suggested-by: Aravind Retnakaran<aravind.retnakaran@nutanix.com>
> > > Signed-off-by: Het Gala<het.gala@nutanix.com>
> > > ---
> > >   migration/exec.c      | 31 ++++++++++++++++--
> > >   migration/exec.h      |  4 ++-
> > >   migration/migration.c | 75 +++++++++++++++++++++++++++++++++++--------
> > >   migration/rdma.c      | 30 +++++------------
> > >   migration/rdma.h      |  3 +-
> > >   migration/socket.c    | 21 ++++--------
> > >   migration/socket.h    |  3 +-
> > >   7 files changed, 110 insertions(+), 57 deletions(-)
> > > 
> > > diff --git a/migration/exec.c b/migration/exec.c
> > > index 375d2e1b54..4fa9819792 100644
> > > --- a/migration/exec.c
> > > +++ b/migration/exec.c
> > > @@ -23,14 +23,39 @@
> > >   #include "migration.h"
> > >   #include "io/channel-command.h"
> > >   #include "trace.h"
> > > +#include "qapi/error.h"
> > > -void exec_start_outgoing_migration(MigrationState *s, const char *command, Error **errp)
> > > +void init_exec_array(strList *command, const char *argv[], Error **errp)
> > > +{
> > > +    int i = 0;
> > > +    strList *lst;
> > > +
> > > +    for (lst = command; lst ; lst = lst->next) {
> > > +        argv[i++] = lst->value;
> > > +    }
> > > +
> > > +    /*
> > > +     * Considering exec command always has 3 arguments to execute
> > > +     * a command directly from the bash itself.
> > > +     */
> > > +    if (i > 3) {
> > > +        error_setg(errp, "exec accepts maximum of 3 arguments in the list");
> > > +        return;
> > > +    }
> > By the time this check fires, the for() loop above has already
> > done out of bounds writes on argv[].
> Ack. check should be before for loop.
> > > +
> > > +    argv[i] = NULL;
> > > +    return;
> > > +}
> > > +
> > > +void exec_start_outgoing_migration(MigrationState *s, strList *command,
> > > +                                   Error **errp)
> > >   {
> > >       QIOChannel *ioc;
> > > -    const char *argv[] = { "/bin/sh", "-c", command, NULL };
> > > +    const char *argv[4];
> > > +    init_exec_array(command, argv, errp);
> > If someone invokes 'migrate' with the old URI style, the
> > strList will be 3 elements, and thus argv[4] is safe.
> > 
> > If someone invokes 'migrate' with thue new MigrateChannel style,
> > the strList can be arbitrarily long and thus argv[4] will be
> > risk of overflow.
> 
> Okay, Can you give me an example where strList can be very long in the new
> MigrateChannel ? because in that case,

The new MigrateAddress struct allows the user to have arbitrary
command args, so for example I would expect to be able to do


 { "execute": "migrate",
     "arguments": {
         "channel": { "channeltype": "main",
                      "addr": { "transport": "exec",
                                "exec": ["/bin/ssh",
				         "-p", "6000",
					 "-l", "root",
					 "-o", "CheckHostIP=no",
					 "-o", "ConnectTimeout=15",
                                         "somehost" ] } } } }



> 
> trace_migration_exec_outgoing(argv[2]);
> 
> will also be not correct right. Will have to come up with something that is
> dynamic ?

Yes, that will need addressing too.

We already need to convert from strList to char ** in order
to call qio_channel_command_new_spawn.

Given that, you can use g_strjoinv(" ", argv) to generate a
combined string that can be given to the trace func.


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|