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

[Martin Kletzander]

On Tue, Sep 20, 2022 at 01:10:49PM +0200, Peter Krempa wrote:
>On Tue, Sep 20, 2022 at 11:44:06 +0100, Daniel P. Berrangé wrote:
>> 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.
>
>The only inconvenient thing here is that virDomainXMLOption is not
>available in virdomainjob.h due to the dependancy order of header files.
>
>And that is something I don't really fancy refactoring. Kristina can do
>that as the original author.
>

virDomainJobObjConfig could be AFAIK, and would make more sense.  Again,
not really asking you to do it.

Adding Kristina to Cc for the sake of completeness.
-------------- 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/179cd8e0/attachment.sig>