[PATCH 0/2] qemu: fix crash in qemuDomainGetGuestInfo
See patch 2/2 Peter Krempa (2): qemuAgentFSInfoFormatParams: Remove pointless returned value qemuDomainGetGuestInfo: Don't try to free a negative number of entries src/qemu/qemu_agent.c | 2 +- src/qemu/qemu_driver.c | 43 ++++++++++++++++++++---------------------- 2 files changed, 21 insertions(+), 24 deletions(-) -- 2.24.1
The only caller doesn't check the value and also there are no real errors to report anyways. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 31 ++++++++++++------------------- 1 file changed, 12 insertions(+), 19 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index cd761f87b5..02ea582767 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -22709,24 +22709,20 @@ qemuDomainGetGuestInfoCheckSupport(unsigned int *types) *types = *types & supportedGuestInfoTypes; } -/* Returns: 0 on success - * -1 otherwise - */ -static int +static void qemuAgentFSInfoFormatParams(qemuAgentFSInfoPtr *fsinfo, int nfs, virDomainDefPtr vmdef, virTypedParameterPtr *params, int *nparams, int *maxparams) { - int ret = -1; size_t i, j; /* FIXME: get disk target */ if (virTypedParamsAddUInt(params, nparams, maxparams, "fs.count", nfs) < 0) - goto cleanup; + return; for (i = 0; i < nfs; i++) { char param_name[VIR_TYPED_PARAM_FIELD_LENGTH]; @@ -22734,17 +22730,17 @@ qemuAgentFSInfoFormatParams(qemuAgentFSInfoPtr *fsinfo, "fs.%zu.name", i); if (virTypedParamsAddString(params, nparams, maxparams, param_name, fsinfo[i]->name) < 0) - goto cleanup; + return; g_snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH, "fs.%zu.mountpoint", i); if (virTypedParamsAddString(params, nparams, maxparams, param_name, fsinfo[i]->mountpoint) < 0) - goto cleanup; + return; g_snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH, "fs.%zu.fstype", i); if (virTypedParamsAddString(params, nparams, maxparams, param_name, fsinfo[i]->fstype) < 0) - goto cleanup; + return; /* disk usage values are not returned by older guest agents, so * only add the params if the value is set */ @@ -22753,20 +22749,20 @@ qemuAgentFSInfoFormatParams(qemuAgentFSInfoPtr *fsinfo, if (fsinfo[i]->total_bytes != -1 && virTypedParamsAddULLong(params, nparams, maxparams, param_name, fsinfo[i]->total_bytes) < 0) - goto cleanup; + return; g_snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH, "fs.%zu.used-bytes", i); if (fsinfo[i]->used_bytes != -1 && virTypedParamsAddULLong(params, nparams, maxparams, param_name, fsinfo[i]->used_bytes) < 0) - goto cleanup; + return; g_snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH, "fs.%zu.disk.count", i); if (virTypedParamsAddUInt(params, nparams, maxparams, param_name, fsinfo[i]->ndisks) < 0) - goto cleanup; + return; for (j = 0; j < fsinfo[i]->ndisks; j++) { virDomainDiskDefPtr diskdef = NULL; qemuAgentDiskInfoPtr d = fsinfo[i]->disks[j]; @@ -22782,7 +22778,7 @@ qemuAgentFSInfoFormatParams(qemuAgentFSInfoPtr *fsinfo, if (diskdef->dst && virTypedParamsAddString(params, nparams, maxparams, param_name, diskdef->dst) < 0) - goto cleanup; + return; } g_snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH, @@ -22790,22 +22786,19 @@ qemuAgentFSInfoFormatParams(qemuAgentFSInfoPtr *fsinfo, if (d->serial && virTypedParamsAddString(params, nparams, maxparams, param_name, d->serial) < 0) - goto cleanup; + return; g_snprintf(param_name, VIR_TYPED_PARAM_FIELD_LENGTH, "fs.%zu.disk.%zu.device", i, j); if (d->devnode && virTypedParamsAddString(params, nparams, maxparams, param_name, d->devnode) < 0) - goto cleanup; + return; } } - ret = nfs; - - cleanup: - return ret; } + static int qemuDomainGetGuestInfo(virDomainPtr dom, unsigned int types, -- 2.24.1
On a Thursday in 2020, Peter Krempa wrote:
The only caller doesn't check the value and also there are no real errors to report anyways.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_driver.c | 31 ++++++++++++------------------- 1 file changed, 12 insertions(+), 19 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
'nfs' variable was set to -1 or -2 on agent failure. Cleanup then tried to free 'nfs' elements of the array which resulted into a crash. Make 'nfs' size_t and assign it only on successful agent call. https://bugzilla.redhat.com/show_bug.cgi?id=1812965 Broken by commit 599ae372d8cf092 Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_agent.c | 2 +- src/qemu/qemu_driver.c | 12 ++++++++---- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c index 9f3fb9732f..dff327e8d5 100644 --- a/src/qemu/qemu_agent.c +++ b/src/qemu/qemu_agent.c @@ -1914,7 +1914,7 @@ qemuAgentGetFSInfoFillDisks(virJSONValuePtr jsondisks, return 0; } -/* Returns: 0 on success +/* Returns: number of entries in '@info' on success * -2 when agent command is not supported by the agent * -1 otherwise */ diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 02ea582767..e285e9373c 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -22814,7 +22814,7 @@ qemuDomainGetGuestInfo(virDomainPtr dom, g_autofree char *hostname = NULL; unsigned int supportedTypes = types; int rc; - int nfs = 0; + size_t nfs = 0; qemuAgentFSInfoPtr *agentfsinfo = NULL; size_t i; @@ -22867,9 +22867,13 @@ qemuDomainGetGuestInfo(virDomainPtr dom, } } if (supportedTypes & VIR_DOMAIN_GUEST_INFO_FILESYSTEM) { - rc = nfs = qemuAgentGetFSInfo(agent, &agentfsinfo); - if (rc < 0 && !(rc == -2 && types == 0)) - goto exitagent; + rc = qemuAgentGetFSInfo(agent, &agentfsinfo); + if (rc < 0) { + if (!(rc == -2 && types == 0)) + goto exitagent; + } else { + nfs = rc; + } } ret = 0; -- 2.24.1
On a Thursday in 2020, Peter Krempa wrote:
'nfs' variable was set to -1 or -2 on agent failure. Cleanup then tried to free 'nfs' elements of the array which resulted into a crash.
Make 'nfs' size_t and assign it only on successful agent call.
https://bugzilla.redhat.com/show_bug.cgi?id=1812965
Broken by commit 599ae372d8cf092
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_agent.c | 2 +- src/qemu/qemu_driver.c | 12 ++++++++---- 2 files changed, 9 insertions(+), 5 deletions(-)
diff --git a/src/qemu/qemu_agent.c b/src/qemu/qemu_agent.c index 9f3fb9732f..dff327e8d5 100644 --- a/src/qemu/qemu_agent.c +++ b/src/qemu/qemu_agent.c @@ -1914,7 +1914,7 @@ qemuAgentGetFSInfoFillDisks(virJSONValuePtr jsondisks, return 0; }
-/* Returns: 0 on success +/* Returns: number of entries in '@info' on success * -2 when agent command is not supported by the agent * -1 otherwise */ diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 02ea582767..e285e9373c 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -22814,7 +22814,7 @@ qemuDomainGetGuestInfo(virDomainPtr dom, g_autofree char *hostname = NULL; unsigned int supportedTypes = types; int rc; - int nfs = 0; + size_t nfs = 0; qemuAgentFSInfoPtr *agentfsinfo = NULL; size_t i;
@@ -22867,9 +22867,13 @@ qemuDomainGetGuestInfo(virDomainPtr dom,
Some separate issues: The hostname call above also shares the same code path on unsupported command and success, assigning NULL to the TypedParameter Also, I'm confused about the 'types' semantics - info types unsupported by libvirt (none so far, unless the caller passed in nonsensical values) are quietly filtered out. But if a type was requested and the agent does not support it, we error out without actually setting an error.
} } if (supportedTypes & VIR_DOMAIN_GUEST_INFO_FILESYSTEM) { - rc = nfs = qemuAgentGetFSInfo(agent, &agentfsinfo); - if (rc < 0 && !(rc == -2 && types == 0)) - goto exitagent; + rc = qemuAgentGetFSInfo(agent, &agentfsinfo); + if (rc < 0) { + if (!(rc == -2 && types == 0)) + goto exitagent;
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
On Fri, Mar 13, 2020 at 07:39:24 +0100, Ján Tomko wrote:
On a Thursday in 2020, Peter Krempa wrote:
'nfs' variable was set to -1 or -2 on agent failure. Cleanup then tried to free 'nfs' elements of the array which resulted into a crash.
Make 'nfs' size_t and assign it only on successful agent call.
https://bugzilla.redhat.com/show_bug.cgi?id=1812965
Broken by commit 599ae372d8cf092
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/qemu/qemu_agent.c | 2 +- src/qemu/qemu_driver.c | 12 ++++++++---- 2 files changed, 9 insertions(+), 5 deletions(-)
[...]
@@ -22867,9 +22867,13 @@ qemuDomainGetGuestInfo(virDomainPtr dom,
Some separate issues:
The hostname call above also shares the same code path on unsupported command and success, assigning NULL to the TypedParameter
I'll fix this.
Also, I'm confused about the 'types' semantics - info types unsupported by libvirt (none so far, unless the caller passed in nonsensical values)
Yes that's also weird. I have a patch.
are quietly filtered out. But if a type was requested and the agent does not support it, we error out without actually setting an error.
Actually qemuAgentCommand calls qemuAgentCheckError all the time even if we don't want to report the error. I'm not sure if we care.
On 3/12/20 1:01 PM, Peter Krempa wrote:
See patch 2/2
Peter Krempa (2): qemuAgentFSInfoFormatParams: Remove pointless returned value qemuDomainGetGuestInfo: Don't try to free a negative number of entries
src/qemu/qemu_agent.c | 2 +- src/qemu/qemu_driver.c | 43 ++++++++++++++++++++---------------------- 2 files changed, 21 insertions(+), 24 deletions(-)
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
participants (3)
-
Daniel Henrique Barboza -
Ján Tomko -
Peter Krempa