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@xxxxxxxxxx>
>
> > Fixes: 84e9fd068ccad6e19e037cd6680df437617e2de5
> > Signed-off-by: Peter Krempa <pkrempa@xxxxxxxxxx>
> > ---
> >
> > 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 :|
