The remote_*_args methods will generally borrow pointers
passed in the caller, so should not be freed.

On failure of the virTypedParamsSerialize method, however,
xdr_free was being called. This is presumably because it
was thought that the params may have been partially
serialized and need cleaning up. This is incorrect, as
virTypedParamsSerialize takes care to cleanup partially
serialized data. This xdr_free call would lead to free'ing
the borrowed cookie pointers, which would be a double free.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 src/remote/remote_driver.c | 12 ------------
 1 file changed, 12 deletions(-)

diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
index b0dba9057b..bb44d0004f 100644
--- a/src/remote/remote_driver.c
+++ b/src/remote/remote_driver.c
@@ -6919,8 +6919,6 @@ remoteDomainMigrateBegin3Params(virDomainPtr domain,
                                 (struct _virTypedParameterRemote **) &args.params.params_val,
                                 &args.params.params_len,
                                 VIR_TYPED_PARAM_STRING_OKAY) < 0) {
-        xdr_free((xdrproc_t) xdr_remote_domain_migrate_begin3_params_args,
-                 (char *) &args);
         goto cleanup;
     }
 
@@ -6981,8 +6979,6 @@ remoteDomainMigratePrepare3Params(virConnectPtr dconn,
                                 (struct _virTypedParameterRemote **) &args.params.params_val,
                                 &args.params.params_len,
                                 VIR_TYPED_PARAM_STRING_OKAY) < 0) {
-        xdr_free((xdrproc_t) xdr_remote_domain_migrate_prepare3_params_args,
-                 (char *) &args);
         goto cleanup;
     }
 
@@ -7063,8 +7059,6 @@ remoteDomainMigratePrepareTunnel3Params(virConnectPtr dconn,
                                 (struct _virTypedParameterRemote **) &args.params.params_val,
                                 &args.params.params_len,
                                 VIR_TYPED_PARAM_STRING_OKAY) < 0) {
-        xdr_free((xdrproc_t) xdr_remote_domain_migrate_prepare_tunnel3_params_args,
-                 (char *) &args);
         goto cleanup;
     }
 
@@ -7149,8 +7143,6 @@ remoteDomainMigratePerform3Params(virDomainPtr dom,
                                 (struct _virTypedParameterRemote **) &args.params.params_val,
                                 &args.params.params_len,
                                 VIR_TYPED_PARAM_STRING_OKAY) < 0) {
-        xdr_free((xdrproc_t) xdr_remote_domain_migrate_perform3_params_args,
-                 (char *) &args);
         goto cleanup;
     }
 
@@ -7216,8 +7208,6 @@ remoteDomainMigrateFinish3Params(virConnectPtr dconn,
                                 (struct _virTypedParameterRemote **) &args.params.params_val,
                                 &args.params.params_len,
                                 VIR_TYPED_PARAM_STRING_OKAY) < 0) {
-        xdr_free((xdrproc_t) xdr_remote_domain_migrate_finish3_params_args,
-                 (char *) &args);
         goto cleanup;
     }
 
@@ -7284,8 +7274,6 @@ remoteDomainMigrateConfirm3Params(virDomainPtr domain,
                                 (struct _virTypedParameterRemote **) &args.params.params_val,
                                 &args.params.params_len,
                                 VIR_TYPED_PARAM_STRING_OKAY) < 0) {
-        xdr_free((xdrproc_t) xdr_remote_domain_migrate_confirm3_params_args,
-                 (char *) &args);
         goto cleanup;
     }
 
-- 
2.38.1

On Tue, Jan 10, 2023 at 05:42:24AM -0500, Daniel P. Berrangé wrote:
>The remote_*_args methods will generally borrow pointers
>passed in the caller, so should not be freed.
>
>On failure of the virTypedParamsSerialize method, however,
>xdr_free was being called. This is presumably because it
>was thought that the params may have been partially
>serialized and need cleaning up. This is incorrect, as
>virTypedParamsSerialize takes care to cleanup partially
>serialized data. This xdr_free call would lead to free'ing
>the borrowed cookie pointers, which would be a double free.
>

Which are marked g_autofree in the caller, yes.  Some other places even
mention that caller free()s those.

>Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>

Reviewed-by: Martin Kletzander <mkletzan@redhat.com>

and SFF