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

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(a)redhat.com&gt; > Fixes: 84e9fd068ccad6e19e037cd6680df437617e2de5 > Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; > --- > > 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 >