[libvirt] [PATCH 0/3] Fix a few issues in cpuGetModels
Jiri Denemark (3): cpuGetModels: Switch to virArch cpuGetModels: Fix memory leak on error cpuGetModels: Create a NULL-terminated list src/cpu/cpu.c | 19 +++++-------------- src/cpu/cpu.h | 3 +-- src/cpu/cpu_ppc64.c | 11 ++++++++++- src/cpu/cpu_x86.c | 11 ++++++++++- src/driver-hypervisor.h | 2 +- src/qemu/qemu_driver.c | 11 ++++++++++- src/test/test_driver.c | 12 +++++++++++- 7 files changed, 48 insertions(+), 21 deletions(-) -- 2.8.2
Our internal APIs mostly use virArch rather than strings. Switching cpuGetModels to virArch will save us from unnecessary conversions in the future. Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/cpu/cpu.c | 17 ++++------------- src/cpu/cpu.h | 3 +-- src/driver-hypervisor.h | 2 +- src/qemu/qemu_driver.c | 11 ++++++++++- src/test/test_driver.c | 12 +++++++++++- 5 files changed, 27 insertions(+), 18 deletions(-) diff --git a/src/cpu/cpu.c b/src/cpu/cpu.c index 1952b53..69055e2 100644 --- a/src/cpu/cpu.c +++ b/src/cpu/cpu.c @@ -715,7 +715,7 @@ cpuModelIsAllowed(const char *model, /** * cpuGetModels: * - * @archName: CPU architecture string + * @arch: CPU architecture * @models: where to store the list of supported models * * Fetches all CPU models supported by libvirt on @archName. @@ -723,26 +723,17 @@ cpuModelIsAllowed(const char *model, * Returns number of supported CPU models or -1 on error. */ int -cpuGetModels(const char *archName, char ***models) +cpuGetModels(virArch arch, char ***models) { struct cpuArchDriver *driver; - virArch arch; - VIR_DEBUG("arch=%s", archName); - - arch = virArchFromString(archName); - if (arch == VIR_ARCH_NONE) { - virReportError(VIR_ERR_INVALID_ARG, - _("cannot find architecture %s"), - archName); - return -1; - } + VIR_DEBUG("arch=%s", virArchToString(arch)); driver = cpuGetSubDriver(arch); if (driver == NULL) { virReportError(VIR_ERR_INVALID_ARG, _("cannot find a driver for the architecture %s"), - archName); + virArchToString(arch)); return -1; } diff --git a/src/cpu/cpu.h b/src/cpu/cpu.h index f15dc16..5192607 100644 --- a/src/cpu/cpu.h +++ b/src/cpu/cpu.h @@ -199,8 +199,7 @@ cpuModelIsAllowed(const char *model, ATTRIBUTE_NONNULL(1); int -cpuGetModels(const char *arch, char ***models) - ATTRIBUTE_NONNULL(1); +cpuGetModels(virArch arch, char ***models); /* cpuDataFormat and cpuDataParse are implemented for unit tests only and * have no real-life usage diff --git a/src/driver-hypervisor.h b/src/driver-hypervisor.h index d11ff7f..89ff446 100644 --- a/src/driver-hypervisor.h +++ b/src/driver-hypervisor.h @@ -677,7 +677,7 @@ typedef char * typedef int (*virDrvConnectGetCPUModelNames)(virConnectPtr conn, - const char *args, + const char *archName, char ***models, unsigned int flags); diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index c4c4968..171fddb 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -18647,14 +18647,23 @@ qemuNodeSuspendForDuration(virConnectPtr conn, static int qemuConnectGetCPUModelNames(virConnectPtr conn, - const char *arch, + const char *archName, char ***models, unsigned int flags) { + virArch arch; + virCheckFlags(0, -1); if (virConnectGetCPUModelNamesEnsureACL(conn) < 0) return -1; + if (!(arch = virArchFromString(archName))) { + virReportError(VIR_ERR_INVALID_ARG, + _("cannot find architecture %s"), + archName); + return -1; + } + return cpuGetModels(arch, models); } diff --git a/src/test/test_driver.c b/src/test/test_driver.c index a51eb09..aba98ca 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -5696,11 +5696,21 @@ testDomainScreenshot(virDomainPtr dom ATTRIBUTE_UNUSED, static int testConnectGetCPUModelNames(virConnectPtr conn ATTRIBUTE_UNUSED, - const char *arch, + const char *archName, char ***models, unsigned int flags) { + virArch arch; + virCheckFlags(0, -1); + + if (!(arch = virArchFromString(archName))) { + virReportError(VIR_ERR_INVALID_ARG, + _("cannot find architecture %s"), + archName); + return -1; + } + return cpuGetModels(arch, models); } -- 2.8.2
On Fri, May 13, 2016 at 11:23:28PM +0200, Jiri Denemark wrote:
Our internal APIs mostly use virArch rather than strings. Switching cpuGetModels to virArch will save us from unnecessary conversions in the future.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> ---
This patch seems to be unrelated and after off-list discussion this change will be used by another patch series. It would be probably better to resend it with that patch series.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/cpu/cpu_ppc64.c | 4 +++- src/cpu/cpu_x86.c | 4 +++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/src/cpu/cpu_ppc64.c b/src/cpu/cpu_ppc64.c index 364c8ed..1aed984 100644 --- a/src/cpu/cpu_ppc64.c +++ b/src/cpu/cpu_ppc64.c @@ -886,8 +886,10 @@ ppc64DriverGetModels(char ***models) if (VIR_STRDUP(name, model->name) < 0) goto error; - if (VIR_APPEND_ELEMENT(*models, nmodels, name) < 0) + if (VIR_APPEND_ELEMENT(*models, nmodels, name) < 0) { + VIR_FREE(name); goto error; + } } else { nmodels++; } diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c index b7f1690..c6aacc1 100644 --- a/src/cpu/cpu_x86.c +++ b/src/cpu/cpu_x86.c @@ -2219,8 +2219,10 @@ x86GetModels(char ***models) if (VIR_STRDUP(name, model->name) < 0) goto error; - if (VIR_APPEND_ELEMENT(*models, nmodels, name) < 0) + if (VIR_APPEND_ELEMENT(*models, nmodels, name) < 0) { + VIR_FREE(name); goto error; + } } else { nmodels++; } -- 2.8.2
On Fri, May 13, 2016 at 11:23:29PM +0200, Jiri Denemark wrote:
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/cpu/cpu_ppc64.c | 4 +++- src/cpu/cpu_x86.c | 4 +++- 2 files changed, 6 insertions(+), 2 deletions(-)
ACK Pavel
On Mon, May 16, 2016 at 10:47:54 +0200, Pavel Hrdina wrote:
On Fri, May 13, 2016 at 11:23:29PM +0200, Jiri Denemark wrote:
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/cpu/cpu_ppc64.c | 4 +++- src/cpu/cpu_x86.c | 4 +++- 2 files changed, 6 insertions(+), 2 deletions(-)
ACK
Thanks, pushed. Jirka
The list of CPU models is freed using virStringFreeList, which expects the list to by NULL-terminated. This bug could theoretically crash libvirtd in remoteDispatchConnectGetCPUModelNames, but luckily enough we never return more than REMOTE_CONNECT_CPU_MODELS_MAX models in the list. Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/cpu/cpu.c | 2 +- src/cpu/cpu_ppc64.c | 7 +++++++ src/cpu/cpu_x86.c | 7 +++++++ 3 files changed, 15 insertions(+), 1 deletion(-) diff --git a/src/cpu/cpu.c b/src/cpu/cpu.c index 69055e2..2f2b658 100644 --- a/src/cpu/cpu.c +++ b/src/cpu/cpu.c @@ -716,7 +716,7 @@ cpuModelIsAllowed(const char *model, * cpuGetModels: * * @arch: CPU architecture - * @models: where to store the list of supported models + * @models: where to store the NULL-terminated list of supported models * * Fetches all CPU models supported by libvirt on @archName. * diff --git a/src/cpu/cpu_ppc64.c b/src/cpu/cpu_ppc64.c index 1aed984..c784a55 100644 --- a/src/cpu/cpu_ppc64.c +++ b/src/cpu/cpu_ppc64.c @@ -897,6 +897,13 @@ ppc64DriverGetModels(char ***models) model = model->next; } + if (models) { + /* Make sure models is NULL-terminated */ + if (VIR_EXPAND_N(*models, nmodels, 1) < 0) + goto error; + nmodels--; + } + cleanup: ppc64MapFree(map); diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c index c6aacc1..6ee7ff9 100644 --- a/src/cpu/cpu_x86.c +++ b/src/cpu/cpu_x86.c @@ -2230,6 +2230,13 @@ x86GetModels(char ***models) model = model->next; } + if (models) { + /* Make sure models is NULL-terminated */ + if (VIR_EXPAND_N(*models, nmodels, 1) < 0) + goto error; + nmodels--; + } + return nmodels; error: -- 2.8.2
On Fri, May 13, 2016 at 11:23:30PM +0200, Jiri Denemark wrote:
The list of CPU models is freed using virStringFreeList, which expects the list to by NULL-terminated. This bug could theoretically crash libvirtd in remoteDispatchConnectGetCPUModelNames, but luckily enough we never return more than REMOTE_CONNECT_CPU_MODELS_MAX models in the list.
Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/cpu/cpu.c | 2 +- src/cpu/cpu_ppc64.c | 7 +++++++ src/cpu/cpu_x86.c | 7 +++++++ 3 files changed, 15 insertions(+), 1 deletion(-)
diff --git a/src/cpu/cpu.c b/src/cpu/cpu.c index 69055e2..2f2b658 100644 --- a/src/cpu/cpu.c +++ b/src/cpu/cpu.c @@ -716,7 +716,7 @@ cpuModelIsAllowed(const char *model, * cpuGetModels: * * @arch: CPU architecture - * @models: where to store the list of supported models + * @models: where to store the NULL-terminated list of supported models * * Fetches all CPU models supported by libvirt on @archName. * diff --git a/src/cpu/cpu_ppc64.c b/src/cpu/cpu_ppc64.c index 1aed984..c784a55 100644 --- a/src/cpu/cpu_ppc64.c +++ b/src/cpu/cpu_ppc64.c @@ -897,6 +897,13 @@ ppc64DriverGetModels(char ***models) model = model->next; }
+ if (models) { + /* Make sure models is NULL-terminated */ + if (VIR_EXPAND_N(*models, nmodels, 1) < 0) + goto error; + nmodels--; + } +
This fixes the function only in case that there is no failure. If the VIR_EXPAND_N() or VIR_APPEND_ELEMENT() fails we will hit segfault in virStringFreeList(). We need to preallocate the whole array with nmodels + 1. Pavel
cleanup: ppc64MapFree(map);
diff --git a/src/cpu/cpu_x86.c b/src/cpu/cpu_x86.c index c6aacc1..6ee7ff9 100644 --- a/src/cpu/cpu_x86.c +++ b/src/cpu/cpu_x86.c @@ -2230,6 +2230,13 @@ x86GetModels(char ***models) model = model->next; }
+ if (models) { + /* Make sure models is NULL-terminated */ + if (VIR_EXPAND_N(*models, nmodels, 1) < 0) + goto error; + nmodels--; + } + return nmodels;
error: -- 2.8.2
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
participants (2)
-
Jiri Denemark -
Pavel Hrdina