[PATCH v2] virdomainjob: virDomainObjInitJob: Avoid borrowing memory from 'virDomainXMLOption'
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. Fixes: 84e9fd068ccad6e19e037cd6680df437617e2de5 Signed-off-by: Peter Krempa <pkrempa@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
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@redhat.com>
Fixes: 84e9fd068ccad6e19e037cd6680df437617e2de5 Signed-off-by: Peter Krempa <pkrempa@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
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@redhat.com>
Fixes: 84e9fd068ccad6e19e037cd6680df437617e2de5 Signed-off-by: Peter Krempa <pkrempa@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 :|
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.
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.
participants (3)
-
Daniel P. Berrangé -
Martin Kletzander -
Peter Krempa