Re: [PATCH v2 25/25] tests: Pass in MigrateStart** into test_migrate_start()

[Dr. David Alan Gilbert]

* Peter Xu (peterx@redhat.com) wrote:
> test_migrate_start() will release the MigrateStart structure that passed
> in, however that's not super clear to the caller because after the call
> returned the pointer can still be referenced by the callers.  It can easily
> be a source of use-after-free.
> 
> Let's pass in a double pointer of that, then we can safely clear the
> pointer for the caller after the struct is released.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  tests/qtest/migration-test.c | 27 +++++++++++++++------------
>  1 file changed, 15 insertions(+), 12 deletions(-)
> 
> diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
> index 09a9ce4401..67f0601988 100644
> --- a/tests/qtest/migration-test.c
> +++ b/tests/qtest/migration-test.c
> @@ -496,7 +496,7 @@ static void migrate_start_destroy(MigrateStart *args)
>  }
>  
>  static int test_migrate_start(QTestState **from, QTestState **to,
> -                              const char *uri, MigrateStart *args)
> +                              const char *uri, MigrateStart **pargs)
>  {
>      g_autofree gchar *arch_source = NULL;
>      g_autofree gchar *arch_target = NULL;
> @@ -508,6 +508,7 @@ static int test_migrate_start(QTestState **from, 
> QTestState **to,
>      g_autofree char *shmem_path = NULL;
>      const char *arch = qtest_get_arch();
>      const char *machine_opts = NULL;
> +    MigrateStart *args = *pargs;
>      const char *memory_size;
>      int ret = 0;
>  
> @@ -622,6 +623,8 @@ static int test_migrate_start(QTestState **from, 
> QTestState **to,
>  
>  out:
>      migrate_start_destroy(args);
> +    /* This tells the caller that this structure is gone */
> +    *pargs = NULL;
>      return ret;
>  }
>  
> @@ -668,7 +671,7 @@ static int migrate_postcopy_prepare(QTestState **from_ptr,
>      bool postcopy_preempt = args->postcopy_preempt;
>      QTestState *from, *to;
>  
> -    if (test_migrate_start(&from, &to, uri, args)) {
> +    if (test_migrate_start(&from, &to, uri, &args)) {
>          return -1;
>      }
>  
> @@ -822,7 +825,7 @@ static void test_baddest(void)
>  
>      args->hide_stderr = true;
>  
> -    if (test_migrate_start(&from, &to, "tcp:127.0.0.1:0", args)) {
> +    if (test_migrate_start(&from, &to, "tcp:127.0.0.1:0", &args)) {
>          return;
>      }
>      migrate_qmp(from, "tcp:127.0.0.1:0", "{}");
> @@ -838,7 +841,7 @@ static void test_precopy_unix_common(bool dirty_ring)
>  
>      args->use_dirty_ring = dirty_ring;
>  
> -    if (test_migrate_start(&from, &to, uri, args)) {
> +    if (test_migrate_start(&from, &to, uri, &args)) {
>          return;
>      }
>  
> @@ -926,7 +929,7 @@ static void test_xbzrle(const char *uri)
>      MigrateStart *args = migrate_start_new();
>      QTestState *from, *to;
>  
> -    if (test_migrate_start(&from, &to, uri, args)) {
> +    if (test_migrate_start(&from, &to, uri, &args)) {
>          return;
>      }
>  
> @@ -980,7 +983,7 @@ static void test_precopy_tcp(void)
>      g_autofree char *uri = NULL;
>      QTestState *from, *to;
>  
> -    if (test_migrate_start(&from, &to, "tcp:127.0.0.1:0", args)) {
> +    if (test_migrate_start(&from, &to, "tcp:127.0.0.1:0", &args)) {
>          return;
>      }
>  
> @@ -1025,7 +1028,7 @@ static void test_migrate_fd_proto(void)
>      QDict *rsp;
>      const char *error_desc;
>  
> -    if (test_migrate_start(&from, &to, "defer", args)) {
> +    if (test_migrate_start(&from, &to, "defer", &args)) {
>          return;
>      }
>  
> @@ -1105,7 +1108,7 @@ static void do_test_validate_uuid(MigrateStart *args, 
> bool should_fail)
>      g_autofree char *uri = g_strdup_printf("unix:%s/migsocket", tmpfs);
>      QTestState *from, *to;
>  
> -    if (test_migrate_start(&from, &to, uri, args)) {
> +    if (test_migrate_start(&from, &to, uri, &args)) {
>          return;
>      }
>  
> @@ -1197,7 +1200,7 @@ static void test_migrate_auto_converge(void)
>       */
>      const int64_t expected_threshold = max_bandwidth * downtime_limit / 1000;
>  
> -    if (test_migrate_start(&from, &to, uri, args)) {
> +    if (test_migrate_start(&from, &to, uri, &args)) {
>          return;
>      }
>  
> @@ -1266,7 +1269,7 @@ static void test_multifd_tcp(const char *method)
>      QDict *rsp;
>      g_autofree char *uri = NULL;
>  
> -    if (test_migrate_start(&from, &to, "defer", args)) {
> +    if (test_migrate_start(&from, &to, "defer", &args)) {
>          return;
>      }
>  
> @@ -1352,7 +1355,7 @@ static void test_multifd_tcp_cancel(void)
>  
>      args->hide_stderr = true;
>  
> -    if (test_migrate_start(&from, &to, "defer", args)) {
> +    if (test_migrate_start(&from, &to, "defer", &args)) {
>          return;
>      }
>  
> @@ -1391,7 +1394,7 @@ static void test_multifd_tcp_cancel(void)
>      args = migrate_start_new();
>      args->only_target = true;
>  
> -    if (test_migrate_start(&from, &to2, "defer", args)) {
> +    if (test_migrate_start(&from, &to2, "defer", &args)) {
>          return;
>      }
>  
> -- 
> 2.32.0
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK