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<berra...@redhat.com>
> > > Suggested-by: Manish Mishra<manish.mis...@nutanix.com>
> > > Suggested-by: Aravind Retnakaran<aravind.retnaka...@nutanix.com>
> > > Signed-off-by: Het Gala<het.g...@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 :|
