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

[Martin Kletzander <mkletzan@xxxxxxxxxx>]

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

Attachment: signature.asc
Description: PGP signature