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.