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

[Daniel P. Berrangé]

On Tue, Sep 20, 2022 at 12:33:15PM +0200, Martin Kletzander wrote:
> 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,

No objection to this patch for the immediate fix, but overall it
feels dirty to be stealing pointers from the xmlopt object.

virDomainXMLOption is a virObject instance, so IMHO  anywhere that
needs the callbacks, should just keep a reference on the whole
virDomainXMLOption object, instead of grabbing callback pointers
from inside it and then passing them around.

> 
> 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
> > 



With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|