[libvirt PATCH 0/3] fix a memory leak on qemuProcessReconnect
The first two patches have no functional impact on current code. Ján Tomko (3): qemu: rename qemuDomainObjFreeJob -> qemuDomainObjClearJob qemu: qemuDomainObjClearJob: use g_clear_pointer qemuProcessReconnect: clear 'oldjob' src/qemu/qemu_domain.c | 2 +- src/qemu/qemu_domainjob.c | 7 +++++-- src/qemu/qemu_domainjob.h | 3 ++- src/qemu/qemu_process.c | 2 +- 4 files changed, 9 insertions(+), 5 deletions(-) -- 2.26.2
This function does not free the job. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/qemu/qemu_domain.c | 2 +- src/qemu/qemu_domainjob.c | 2 +- src/qemu/qemu_domainjob.h | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 785cee6f18..03917cf000 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -1859,7 +1859,7 @@ qemuDomainObjPrivateFree(void *data) qemuDomainObjPrivateDataClear(priv); virObjectUnref(priv->monConfig); - qemuDomainObjFreeJob(&priv->job); + qemuDomainObjClearJob(&priv->job); VIR_FREE(priv->lockState); VIR_FREE(priv->origname); diff --git a/src/qemu/qemu_domainjob.c b/src/qemu/qemu_domainjob.c index eeaa089959..9f76f1ef04 100644 --- a/src/qemu/qemu_domainjob.c +++ b/src/qemu/qemu_domainjob.c @@ -246,7 +246,7 @@ qemuDomainObjRestoreJob(virDomainObjPtr obj, } void -qemuDomainObjFreeJob(qemuDomainJobObjPtr job) +qemuDomainObjClearJob(qemuDomainJobObjPtr job) { qemuDomainObjResetJob(job); qemuDomainObjResetAsyncJob(job); diff --git a/src/qemu/qemu_domainjob.h b/src/qemu/qemu_domainjob.h index fcbe3fe112..eedd84c503 100644 --- a/src/qemu/qemu_domainjob.h +++ b/src/qemu/qemu_domainjob.h @@ -274,7 +274,7 @@ int qemuDomainJobInfoToParams(qemuDomainJobInfoPtr jobInfo, bool qemuDomainTrackJob(qemuDomainJob job); -void qemuDomainObjFreeJob(qemuDomainJobObjPtr job); +void qemuDomainObjClearJob(qemuDomainJobObjPtr job); int qemuDomainObjInitJob(qemuDomainJobObjPtr job, -- 2.26.2
The function used g_clear_pointer for all but one pointer. Signed-off-by: Ján Tomko <jtomko@redhat.com> --- src/qemu/qemu_domainjob.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/qemu/qemu_domainjob.c b/src/qemu/qemu_domainjob.c index 9f76f1ef04..e5910a11a1 100644 --- a/src/qemu/qemu_domainjob.c +++ b/src/qemu/qemu_domainjob.c @@ -250,7 +250,7 @@ qemuDomainObjClearJob(qemuDomainJobObjPtr job) { qemuDomainObjResetJob(job); qemuDomainObjResetAsyncJob(job); - job->cb->freeJobPrivate(job->privateData); + g_clear_pointer(&job->privateData, job->cb->freeJobPrivate); g_clear_pointer(&job->current, qemuDomainJobInfoFree); g_clear_pointer(&job->completed, qemuDomainJobInfoFree); virCondDestroy(&job->cond); -- 2.26.2
After we started copying the privateData pointer in qemuDomainObjRestoreJob, we should also free them once we're done with them. Register the clear function and use g_auto. Also add a check for job->cb to qemuDomainObjClearJob, to prevent freeing an uninitialized job. https://bugzilla.redhat.com/show_bug.cgi?id=1878450 Signed-off-by: Ján Tomko <jtomko@redhat.com> Fixes: aca37c3fb2e8d733c2788ca4b796c153ea7ce391 --- src/qemu/qemu_domainjob.c | 3 +++ src/qemu/qemu_domainjob.h | 1 + src/qemu/qemu_process.c | 2 +- 3 files changed, 5 insertions(+), 1 deletion(-) diff --git a/src/qemu/qemu_domainjob.c b/src/qemu/qemu_domainjob.c index e5910a11a1..3c2c6b9179 100644 --- a/src/qemu/qemu_domainjob.c +++ b/src/qemu/qemu_domainjob.c @@ -248,6 +248,9 @@ qemuDomainObjRestoreJob(virDomainObjPtr obj, void qemuDomainObjClearJob(qemuDomainJobObjPtr job) { + if (!job->cb) + return; + qemuDomainObjResetJob(job); qemuDomainObjResetAsyncJob(job); g_clear_pointer(&job->privateData, job->cb->freeJobPrivate); diff --git a/src/qemu/qemu_domainjob.h b/src/qemu/qemu_domainjob.h index eedd84c503..79f0127252 100644 --- a/src/qemu/qemu_domainjob.h +++ b/src/qemu/qemu_domainjob.h @@ -275,6 +275,7 @@ int qemuDomainJobInfoToParams(qemuDomainJobInfoPtr jobInfo, bool qemuDomainTrackJob(qemuDomainJob job); void qemuDomainObjClearJob(qemuDomainJobObjPtr job); +G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(qemuDomainJobObj, qemuDomainObjClearJob); int qemuDomainObjInitJob(qemuDomainJobObjPtr job, diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index b1af35b933..a345edd7fb 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -8081,7 +8081,7 @@ qemuProcessReconnect(void *opaque) virQEMUDriverPtr driver = data->driver; virDomainObjPtr obj = data->obj; qemuDomainObjPrivatePtr priv; - qemuDomainJobObj oldjob; + g_auto(qemuDomainJobObj) oldjob = { 0 }; int state; int reason; g_autoptr(virQEMUDriverConfig) cfg = NULL; -- 2.26.2
On a Monday in 2020, Ján Tomko wrote:
After we started copying the privateData pointer in qemuDomainObjRestoreJob, we should also free them once we're done with them.
Register the clear function and use g_auto. Also add a check for job->cb to qemuDomainObjClearJob, to prevent freeing an uninitialized job.
https://bugzilla.redhat.com/show_bug.cgi?id=1878450
Signed-off-by: Ján Tomko <jtomko@redhat.com> Fixes: aca37c3fb2e8d733c2788ca4b796c153ea7ce391 --- src/qemu/qemu_domainjob.c | 3 +++ src/qemu/qemu_domainjob.h | 1 + src/qemu/qemu_process.c | 2 +- 3 files changed, 5 insertions(+), 1 deletion(-)
diff --git a/src/qemu/qemu_domainjob.c b/src/qemu/qemu_domainjob.c index e5910a11a1..3c2c6b9179 100644 --- a/src/qemu/qemu_domainjob.c +++ b/src/qemu/qemu_domainjob.c @@ -248,6 +248,9 @@ qemuDomainObjRestoreJob(virDomainObjPtr obj, void qemuDomainObjClearJob(qemuDomainJobObjPtr job) { + if (!job->cb) + return; + qemuDomainObjResetJob(job); qemuDomainObjResetAsyncJob(job); g_clear_pointer(&job->privateData, job->cb->freeJobPrivate); diff --git a/src/qemu/qemu_domainjob.h b/src/qemu/qemu_domainjob.h index eedd84c503..79f0127252 100644 --- a/src/qemu/qemu_domainjob.h +++ b/src/qemu/qemu_domainjob.h @@ -275,6 +275,7 @@ int qemuDomainJobInfoToParams(qemuDomainJobInfoPtr jobInfo, bool qemuDomainTrackJob(qemuDomainJob job);
void qemuDomainObjClearJob(qemuDomainJobObjPtr job); +G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(qemuDomainJobObj, qemuDomainObjClearJob);
int qemuDomainObjInitJob(qemuDomainJobObjPtr job, diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index b1af35b933..a345edd7fb 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -8081,7 +8081,7 @@ qemuProcessReconnect(void *opaque) virQEMUDriverPtr driver = data->driver; virDomainObjPtr obj = data->obj; qemuDomainObjPrivatePtr priv; - qemuDomainJobObj oldjob; + g_auto(qemuDomainJobObj) oldjob = { 0 };
This fails on older compilers: ../src/qemu/qemu_process.c: In function ‘qemuProcessReconnect’: ../src/qemu/qemu_process.c:8084:5: error: missing braces around initializer [-Werror=missing-braces] g_auto(qemuDomainJobObj) oldjob = { 0 }; ^ ../src/qemu/qemu_process.c:8084:5: error: (near initialization for ‘oldjob.cond’) [-Werror=missing-braces] ../src/qemu/qemu_process.c:8084:5: error: missing initializer for field ‘active’ of ‘qemuDomainJobObj’ [-Werror=missing-field-initializers] In file included from ../src/qemu/qemu_domain.h:34:0, from ../src/qemu/qemu_process.h:25, from ../src/qemu/qemu_process.c:41: ../src/qemu/qemu_domainjob.h:184:19: note: ‘active’ declared here qemuDomainJob active; /* Currently running job */ ^ cc1: all warnings being treated as errors conisder this squashed in: diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index a345edd7fb..073b2d96e0 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -8081,7 +8081,9 @@ qemuProcessReconnect(void *opaque) virQEMUDriverPtr driver = data->driver; virDomainObjPtr obj = data->obj; qemuDomainObjPrivatePtr priv; - g_auto(qemuDomainJobObj) oldjob = { 0 }; + g_auto(qemuDomainJobObj) oldjob = { + .cb = NULL, + }; int state; int reason; g_autoptr(virQEMUDriverConfig) cfg = NULL;
On 9/14/20 1:49 PM, Ján Tomko wrote:
The first two patches have no functional impact on current code.
Ján Tomko (3): qemu: rename qemuDomainObjFreeJob -> qemuDomainObjClearJob qemu: qemuDomainObjClearJob: use g_clear_pointer qemuProcessReconnect: clear 'oldjob'
src/qemu/qemu_domain.c | 2 +- src/qemu/qemu_domainjob.c | 7 +++++-- src/qemu/qemu_domainjob.h | 3 ++- src/qemu/qemu_process.c | 2 +- 4 files changed, 9 insertions(+), 5 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
participants (2)
-
Ján Tomko -
Michal Privoznik