There are several places where postcopy_start() fails without setting
errp. This can cause a null pointer de-reference, as in case of error,
the caller of postcopy_start() copies/prints the error set in errp.

Fix it by setting errp in all of postcopy_start() error paths.

Fixes: 908927db28ea ("migration: Update error description whenever migration fails")
Signed-off-by: Avihai Horon <avihaih@nvidia.com>
---
 migration/migration.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/migration/migration.c b/migration/migration.c
index b73ae3a72c4..86bf76e9258 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2510,6 +2510,8 @@ static int postcopy_start(MigrationState *ms, Error **errp)
         migration_wait_main_channel(ms);
         if (postcopy_preempt_establish_channel(ms)) {
             migrate_set_state(&ms->state, ms->state, MIGRATION_STATUS_FAILED);
+            error_setg(errp, "%s: Failed to establish preempt channel",
+                       __func__);
             return -1;
         }
     }
@@ -2525,17 +2527,22 @@ static int postcopy_start(MigrationState *ms, Error **errp)
 
     ret = migration_stop_vm(ms, RUN_STATE_FINISH_MIGRATE);
     if (ret < 0) {
+        error_setg_errno(errp, -ret, "%s: Failed to stop the VM", __func__);
         goto fail;
     }
 
     ret = migration_maybe_pause(ms, &cur_state,
                                 MIGRATION_STATUS_POSTCOPY_ACTIVE);
     if (ret < 0) {
+        error_setg_errno(errp, -ret, "%s: Failed in migration_maybe_pause()",
+                         __func__);
         goto fail;
     }
 
     ret = bdrv_inactivate_all();
     if (ret < 0) {
+        error_setg_errno(errp, -ret, "%s: Failed in bdrv_inactivate_all()",
+                         __func__);
         goto fail;
     }
     restart_block = true;
@@ -2612,6 +2619,7 @@ static int postcopy_start(MigrationState *ms, Error **errp)
 
     /* Now send that blob */
     if (qemu_savevm_send_packaged(ms->to_dst_file, bioc->data, bioc->usage)) {
+        error_setg(errp, "%s: Failed to send packaged data", __func__);
         goto fail_closefb;
     }
     qemu_fclose(fb);
-- 
2.26.3

On Thu, Mar 28, 2024 at 04:02:52PM +0200, Avihai Horon wrote:
> There are several places where postcopy_start() fails without setting
> errp. This can cause a null pointer de-reference, as in case of error,
> the caller of postcopy_start() copies/prints the error set in errp.
> 
> Fix it by setting errp in all of postcopy_start() error paths.
> 
> Fixes: 908927db28ea ("migration: Update error description whenever migration fails")

I think this should need:

Cc: qemu-stable <qemu-stable@nongnu.org>

> Signed-off-by: Avihai Horon <avihaih@nvidia.com>

Reviewed-by: Peter Xu <peterx@redhat.com>

-- 
Peter Xu

On 3/28/24 15:02, Avihai Horon wrote:
> There are several places where postcopy_start() fails without setting
> errp. This can cause a null pointer de-reference, as in case of error,
> the caller of postcopy_start() copies/prints the error set in errp.
> 
> Fix it by setting errp in all of postcopy_start() error paths.
> 
> Fixes: 908927db28ea ("migration: Update error description whenever migration fails")
> Signed-off-by: Avihai Horon <avihaih@nvidia.com>


Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.


> ---
>   migration/migration.c | 8 ++++++++
>   1 file changed, 8 insertions(+)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index b73ae3a72c4..86bf76e9258 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -2510,6 +2510,8 @@ static int postcopy_start(MigrationState *ms, Error **errp)
>           migration_wait_main_channel(ms);
>           if (postcopy_preempt_establish_channel(ms)) {
>               migrate_set_state(&ms->state, ms->state, MIGRATION_STATUS_FAILED);
> +            error_setg(errp, "%s: Failed to establish preempt channel",
> +                       __func__);
>               return -1;
>           }
>       }
> @@ -2525,17 +2527,22 @@ static int postcopy_start(MigrationState *ms, Error **errp)
>   
>       ret = migration_stop_vm(ms, RUN_STATE_FINISH_MIGRATE);
>       if (ret < 0) {
> +        error_setg_errno(errp, -ret, "%s: Failed to stop the VM", __func__);
>           goto fail;
>       }
>   
>       ret = migration_maybe_pause(ms, &cur_state,
>                                   MIGRATION_STATUS_POSTCOPY_ACTIVE);
>       if (ret < 0) {
> +        error_setg_errno(errp, -ret, "%s: Failed in migration_maybe_pause()",
> +                         __func__);
>           goto fail;
>       }
>   
>       ret = bdrv_inactivate_all();
>       if (ret < 0) {
> +        error_setg_errno(errp, -ret, "%s: Failed in bdrv_inactivate_all()",
> +                         __func__);
>           goto fail;
>       }
>       restart_block = true;
> @@ -2612,6 +2619,7 @@ static int postcopy_start(MigrationState *ms, Error **errp)
>   
>       /* Now send that blob */
>       if (qemu_savevm_send_packaged(ms->to_dst_file, bioc->data, bioc->usage)) {
> +        error_setg(errp, "%s: Failed to send packaged data", __func__);
>           goto fail_closefb;
>       }
>       qemu_fclose(fb);