[PATCH v2] virdomainjob: virDomainObjInitJob: Avoid borrowing memory from 'virDomainXMLOption'

[Martin Kletzander]

On Tue, Sep 20, 2022 at 11:04:39AM +0200, Peter Krempa wrote:
>The 'cb' and 'jobDataPrivateCb' pointers are stored in the job object
>but made point to the memory owned by the virDomainXMLOption struct in
>the callers.
>
>Since the 'virdomainjob' module isn't in control the lifetime of the
>virDomainXMLOption, which in some cases is freed before the domain job
>data, freed memory would be dereferenced in some cases.
>
>Copy the structs from virDomainXMLOption to ensure the lifetime. This is
>possible since the callback functions are immutable.
>

I thought all of xmlopt will have a larger lifetime than the jobs, but
that's not true in all drivers.  Even though it could be handled
elsewhere, or in a different fashion (reference counting xmlopt, etc.)
I think this is the cleanest and safest anyway.  Maybe copying the whole
virDomainJobObjConfig would make a bit more sense...  Anyway,

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

>Fixes: 84e9fd068ccad6e19e037cd6680df437617e2de5
>Signed-off-by: Peter Krempa <pkrempa at redhat.com>
>---
>
>v2:
>    - copy both pointers
>    - don't bother with creating copy functions, use g_memdup
>
> src/conf/virdomainjob.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
>diff --git a/src/conf/virdomainjob.c b/src/conf/virdomainjob.c
>index 7915faa125..aca801af38 100644
>--- a/src/conf/virdomainjob.c
>+++ b/src/conf/virdomainjob.c
>@@ -128,8 +128,8 @@ virDomainObjInitJob(virDomainJobObj *job,
>                     virDomainJobDataPrivateDataCallbacks *jobDataPrivateCb)
> {
>     memset(job, 0, sizeof(*job));
>-    job->cb = cb;
>-    job->jobDataPrivateCb = jobDataPrivateCb;
>+    job->cb = g_memdup(cb, sizeof(*cb));
>+    job->jobDataPrivateCb = g_memdup(jobDataPrivateCb, sizeof(*jobDataPrivateCb));
>
>     if (virCondInit(&job->cond) < 0)
>         return -1;
>@@ -229,6 +229,9 @@ virDomainObjClearJob(virDomainJobObj *job)
>
>     if (job->cb && job->cb->freeJobPrivate)
>         g_clear_pointer(&job->privateData, job->cb->freeJobPrivate);
>+
>+    g_clear_pointer(&job->cb, g_free);
>+    g_clear_pointer(&job->jobDataPrivateCb, g_free);
> }
>
> void
>-- 
>2.37.1
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20220920/12a4ba82/attachment.sig>