exec_start_outgoing_migration() and exec_start_incoming_migration()
leak argv because it uses g_steal_pointer() is used to pass argv
qio_channel_command_new_spawn() while it does not free argv either.

Removing g_steal_pointer() is not sufficient though because argv is
typed g_auto(GStrv), which means the array of strings *and strings* will
be freed. The strings are only borrowed from the caller of
exec_start_outgoing_migration() and exec_start_incoming_migration() so
freeing them result in double-free.

Instead, type argv as g_autofree char **. This ensures only the array
of strings will be freed and the strings won't be freed. Also, remove
unnecessary casts according to the new type.

Fixes: cbab4face57b ("migration: convert exec backend to accept MigrateAddress.")
Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
 migration/exec.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/migration/exec.c b/migration/exec.c
index 47d2f3b8fb02..205675265ea1 100644
--- a/migration/exec.c
+++ b/migration/exec.c
@@ -73,15 +73,15 @@ void exec_start_outgoing_migration(MigrationState *s, strList *command,
     QIOChannel *ioc;
 
     int length = str_list_length(command);
-    g_auto(GStrv) argv = (char **) g_new0(const char *, length + 1);
+    g_autofree char **argv = g_new0(char *, length + 1);
 
     init_exec_array(command, argv, errp);
-    g_autofree char *new_command = g_strjoinv(" ", (char **)argv);
+    g_autofree char *new_command = g_strjoinv(" ", argv);
 
     trace_migration_exec_outgoing(new_command);
     ioc = QIO_CHANNEL(
         qio_channel_command_new_spawn(
-                            (const char * const *) g_steal_pointer(&argv),
+                            (const char * const *) argv,
                             O_RDWR,
                             errp));
     if (!ioc) {
@@ -107,15 +107,15 @@ void exec_start_incoming_migration(strList *command, Error **errp)
     QIOChannel *ioc;
 
     int length = str_list_length(command);
-    g_auto(GStrv) argv = (char **) g_new0(const char *, length + 1);
+    g_autofree char **argv = g_new0(char *, length + 1);
 
     init_exec_array(command, argv, errp);
-    g_autofree char *new_command = g_strjoinv(" ", (char **)argv);
+    g_autofree char *new_command = g_strjoinv(" ", argv);
 
     trace_migration_exec_incoming(new_command);
     ioc = QIO_CHANNEL(
         qio_channel_command_new_spawn(
-                            (const char * const *) g_steal_pointer(&argv),
+                            (const char * const *) argv,
                             O_RDWR,
                             errp));
     if (!ioc) {

---
base-commit: 5005aed8a7e728d028efb40e243ecfc2b4f3df3a
change-id: 20240219-argv-518065e20e87

Best regards,
-- 
Akihiko Odaki <akihiko.odaki@daynix.com>

On Sun, Feb 25, 2024 at 02:54:01PM +0900, Akihiko Odaki wrote:
> exec_start_outgoing_migration() and exec_start_incoming_migration()
> leak argv because it uses g_steal_pointer() is used to pass argv
> qio_channel_command_new_spawn() while it does not free argv either.
> 
> Removing g_steal_pointer() is not sufficient though because argv is
> typed g_auto(GStrv), which means the array of strings *and strings* will
> be freed. The strings are only borrowed from the caller of
> exec_start_outgoing_migration() and exec_start_incoming_migration() so
> freeing them result in double-free.
> 
> Instead, type argv as g_autofree char **. This ensures only the array
> of strings will be freed and the strings won't be freed. Also, remove
> unnecessary casts according to the new type.
> 
> Fixes: cbab4face57b ("migration: convert exec backend to accept MigrateAddress.")
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>

Cc: qemu-stable <qemu-stable@nongnu.org>
Reviewed-by: Peter Xu <peterx@redhat.com>

This should conflict with Steve's other series:

https://lore.kernel.org/r/1708638470-114846-1-git-send-email-steven.sistare@oracle.com

Considering this can be stable material, should be easier if we have the
other series rebased on top of this, even if that was sent first..

Steve, do you still plan to repost your series?  Maybe you can review it &
pick this up into your series?  Then whoever pick up your series will pick
up both (Markus will?)?

Thanks,

-- 
Peter Xu

On 2024/02/26 12:49, Peter Xu wrote:
> On Sun, Feb 25, 2024 at 02:54:01PM +0900, Akihiko Odaki wrote:
>> exec_start_outgoing_migration() and exec_start_incoming_migration()
>> leak argv because it uses g_steal_pointer() is used to pass argv
>> qio_channel_command_new_spawn() while it does not free argv either.
>>
>> Removing g_steal_pointer() is not sufficient though because argv is
>> typed g_auto(GStrv), which means the array of strings *and strings* will
>> be freed. The strings are only borrowed from the caller of
>> exec_start_outgoing_migration() and exec_start_incoming_migration() so
>> freeing them result in double-free.
>>
>> Instead, type argv as g_autofree char **. This ensures only the array
>> of strings will be freed and the strings won't be freed. Also, remove
>> unnecessary casts according to the new type.
>>
>> Fixes: cbab4face57b ("migration: convert exec backend to accept MigrateAddress.")
>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> 
> Cc: qemu-stable <qemu-stable@nongnu.org>
> Reviewed-by: Peter Xu <peterx@redhat.com>
> 
> This should conflict with Steve's other series:
> 
> https://lore.kernel.org/r/1708638470-114846-1-git-send-email-steven.sistare@oracle.com
> 
> Considering this can be stable material, should be easier if we have the
> other series rebased on top of this, even if that was sent first..
> 
> Steve, do you still plan to repost your series?  Maybe you can review it &
> pick this up into your series?  Then whoever pick up your series will pick
> up both (Markus will?)?

Patch "migration: simplify exec migration functions" included in the 
series fixes the identical problem:
https://lore.kernel.org/all/1708638470-114846-6-git-send-email-steven.sistare@oracle.com/

I withdraw my patch as duplicate.

Regards,
Akihiko Odaki