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

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.