[libvirt] [PATCH v2 python 0/2] fix crash in libvirt_virDomainPin*
this small patch set: * move common logic of all libvirt_virDomainPin* functions to new helper in util module. * add check for pycpumap length. Changes since v1: - add new helper in util module. Konstantin Neumoin (2): minor clean-up for libvirt_virDomainPin* add check for pycpumap length libvirt-override.c | 131 +++++------------------------------------------------ libvirt-utils.c | 63 ++++++++++++++++++++++++++ libvirt-utils.h | 5 ++ 3 files changed, 79 insertions(+), 120 deletions(-) -- 2.5.5
All libvirt_virDomainPin* functions do the same thing for convert pycpumap to cpumap, so this patch moves all common logic to new helper - virPyCpuMapToChar. Signed-off-by: Konstantin Neumoin <kneumoin@virtuozzo.com> --- libvirt-override.c | 131 +++++------------------------------------------------ libvirt-utils.c | 60 ++++++++++++++++++++++++ libvirt-utils.h | 5 ++ 3 files changed, 76 insertions(+), 120 deletions(-) diff --git a/libvirt-override.c b/libvirt-override.c index fa3e2ca..ba0d87c 100644 --- a/libvirt-override.c +++ b/libvirt-override.c @@ -1302,8 +1302,7 @@ libvirt_virDomainPinVcpu(PyObject *self ATTRIBUTE_UNUSED, PyObject *pyobj_domain, *pycpumap; PyObject *ret = NULL; unsigned char *cpumap; - int cpumaplen, vcpu, tuple_size, cpunum; - size_t i; + int cpumaplen, vcpu, cpunum; int i_retval; if (!PyArg_ParseTuple(args, (char *)"OiO:virDomainPinVcpu", @@ -1314,34 +1313,8 @@ libvirt_virDomainPinVcpu(PyObject *self ATTRIBUTE_UNUSED, if ((cpunum = getPyNodeCPUCount(virDomainGetConnect(domain))) < 0) return VIR_PY_INT_FAIL; - if (PyTuple_Check(pycpumap)) { - tuple_size = PyTuple_Size(pycpumap); - if (tuple_size == -1) - return ret; - } else { - PyErr_SetString(PyExc_TypeError, "Unexpected type, tuple is required"); - return ret; - } - - cpumaplen = VIR_CPU_MAPLEN(cpunum); - if (VIR_ALLOC_N(cpumap, cpumaplen) < 0) - return PyErr_NoMemory(); - - for (i = 0; i < tuple_size; i++) { - PyObject *flag = PyTuple_GetItem(pycpumap, i); - bool b; - - if (!flag || libvirt_boolUnwrap(flag, &b) < 0) - goto cleanup; - - if (b) - VIR_USE_CPU(cpumap, i); - else - VIR_UNUSE_CPU(cpumap, i); - } - - for (; i < cpunum; i++) - VIR_UNUSE_CPU(cpumap, i); + if (virPyCpuMapToChar(cpunum, pycpumap, &cpumap, &cpumaplen) < 0) + return NULL; LIBVIRT_BEGIN_ALLOW_THREADS; i_retval = virDomainPinVcpu(domain, vcpu, cpumap, cpumaplen); @@ -1366,8 +1339,7 @@ libvirt_virDomainPinVcpuFlags(PyObject *self ATTRIBUTE_UNUSED, PyObject *pyobj_domain, *pycpumap; PyObject *ret = NULL; unsigned char *cpumap; - int cpumaplen, vcpu, tuple_size, cpunum; - size_t i; + int cpumaplen, vcpu, cpunum; unsigned int flags; int i_retval; @@ -1379,34 +1351,8 @@ libvirt_virDomainPinVcpuFlags(PyObject *self ATTRIBUTE_UNUSED, if ((cpunum = getPyNodeCPUCount(virDomainGetConnect(domain))) < 0) return VIR_PY_INT_FAIL; - if (PyTuple_Check(pycpumap)) { - tuple_size = PyTuple_Size(pycpumap); - if (tuple_size == -1) - return ret; - } else { - PyErr_SetString(PyExc_TypeError, "Unexpected type, tuple is required"); - return ret; - } - - cpumaplen = VIR_CPU_MAPLEN(cpunum); - if (VIR_ALLOC_N(cpumap, cpumaplen) < 0) - return PyErr_NoMemory(); - - for (i = 0; i < tuple_size; i++) { - PyObject *flag = PyTuple_GetItem(pycpumap, i); - bool b; - - if (!flag || libvirt_boolUnwrap(flag, &b) < 0) - goto cleanup; - - if (b) - VIR_USE_CPU(cpumap, i); - else - VIR_UNUSE_CPU(cpumap, i); - } - - for (; i < cpunum; i++) - VIR_UNUSE_CPU(cpumap, i); + if (virPyCpuMapToChar(cpunum, pycpumap, &cpumap, &cpumaplen) < 0) + return NULL; LIBVIRT_BEGIN_ALLOW_THREADS; i_retval = virDomainPinVcpuFlags(domain, vcpu, cpumap, cpumaplen, flags); @@ -1505,8 +1451,7 @@ libvirt_virDomainPinEmulator(PyObject *self ATTRIBUTE_UNUSED, virDomainPtr domain; PyObject *pyobj_domain, *pycpumap; unsigned char *cpumap = NULL; - int cpumaplen, tuple_size, cpunum; - size_t i; + int cpumaplen, cpunum; int i_retval; unsigned int flags; @@ -1519,37 +1464,9 @@ libvirt_virDomainPinEmulator(PyObject *self ATTRIBUTE_UNUSED, if ((cpunum = getPyNodeCPUCount(virDomainGetConnect(domain))) < 0) return VIR_PY_INT_FAIL; - cpumaplen = VIR_CPU_MAPLEN(cpunum); - - if (!PyTuple_Check(pycpumap)) { - PyErr_SetString(PyExc_TypeError, "Unexpected type, tuple is required"); - return NULL; - } - - if ((tuple_size = PyTuple_Size(pycpumap)) == -1) + if (virPyCpuMapToChar(cpunum, pycpumap, &cpumap, &cpumaplen) < 0) return NULL; - if (VIR_ALLOC_N(cpumap, cpumaplen) < 0) - return PyErr_NoMemory(); - - for (i = 0; i < tuple_size; i++) { - PyObject *flag = PyTuple_GetItem(pycpumap, i); - bool b; - - if (!flag || libvirt_boolUnwrap(flag, &b) < 0) { - VIR_FREE(cpumap); - return NULL; - } - - if (b) - VIR_USE_CPU(cpumap, i); - else - VIR_UNUSE_CPU(cpumap, i); - } - - for (; i < cpunum; i++) - VIR_UNUSE_CPU(cpumap, i); - LIBVIRT_BEGIN_ALLOW_THREADS; i_retval = virDomainPinEmulator(domain, cpumap, cpumaplen, flags); LIBVIRT_END_ALLOW_THREADS; @@ -1713,8 +1630,7 @@ libvirt_virDomainPinIOThread(PyObject *self ATTRIBUTE_UNUSED, PyObject *pyobj_domain, *pycpumap; PyObject *ret = NULL; unsigned char *cpumap; - int cpumaplen, iothread_val, tuple_size, cpunum; - size_t i; + int cpumaplen, iothread_val, cpunum; unsigned int flags; int i_retval; @@ -1726,33 +1642,8 @@ libvirt_virDomainPinIOThread(PyObject *self ATTRIBUTE_UNUSED, if ((cpunum = getPyNodeCPUCount(virDomainGetConnect(domain))) < 0) return VIR_PY_INT_FAIL; - if (PyTuple_Check(pycpumap)) { - if ((tuple_size = PyTuple_Size(pycpumap)) == -1) - return ret; - } else { - PyErr_SetString(PyExc_TypeError, "Unexpected type, tuple is required"); - return ret; - } - - cpumaplen = VIR_CPU_MAPLEN(cpunum); - if (VIR_ALLOC_N(cpumap, cpumaplen) < 0) - return PyErr_NoMemory(); - - for (i = 0; i < tuple_size; i++) { - PyObject *flag = PyTuple_GetItem(pycpumap, i); - bool b; - - if (!flag || libvirt_boolUnwrap(flag, &b) < 0) - goto cleanup; - - if (b) - VIR_USE_CPU(cpumap, i); - else - VIR_UNUSE_CPU(cpumap, i); - } - - for (; i < cpunum; i++) - VIR_UNUSE_CPU(cpumap, i); + if (virPyCpuMapToChar(cpunum, pycpumap, &cpumap, &cpumaplen) < 0) + return NULL; LIBVIRT_BEGIN_ALLOW_THREADS; i_retval = virDomainPinIOThread(domain, iothread_val, diff --git a/libvirt-utils.c b/libvirt-utils.c index 2bf7519..aaf4bea 100644 --- a/libvirt-utils.c +++ b/libvirt-utils.c @@ -586,3 +586,63 @@ virPyDictToTypedParams(PyObject *dict, return ret; } #endif /* LIBVIR_CHECK_VERSION(1, 1, 0) */ + + +/* virPyCpuMapToChar + * @cpunum: the number of cpus + * @pycpumap: source Py cpu map + * @cpumapptr: destination cpu map + * @cpumaplen: destination cpu map length + * + * Helper function to convert a pycpumap to char* + * + * return -1 on error. + */ +int +virPyCpuMapToChar(int cpunum, + PyObject *pycpumap, + unsigned char **cpumapptr, + int *cpumaplen) +{ + int tuple_size; + size_t i; + int i_retval = -1; + *cpumapptr = NULL; + + if (!PyTuple_Check(pycpumap)) { + PyErr_SetString(PyExc_TypeError, "Unexpected type, tuple is required"); + goto exit; + } + + *cpumaplen = VIR_CPU_MAPLEN(cpunum); + + if ((tuple_size = PyTuple_Size(pycpumap)) == -1) + goto exit; + + if (VIR_ALLOC_N(*cpumapptr, *cpumaplen) < 0) { + PyErr_NoMemory(); + goto exit; + } + + for (i = 0; i < tuple_size; i++) { + PyObject *flag = PyTuple_GetItem(pycpumap, i); + bool b; + + if (!flag || libvirt_boolUnwrap(flag, &b) < 0) { + VIR_FREE(*cpumapptr); + goto exit; + } + + if (b) + VIR_USE_CPU(*cpumapptr, i); + else + VIR_UNUSE_CPU(*cpumapptr, i); + } + + for (; i < cpunum; i++) + VIR_UNUSE_CPU(*cpumapptr, i); + + i_retval = 0; +exit: + return i_retval; +} diff --git a/libvirt-utils.h b/libvirt-utils.h index f74654c..34d40b7 100644 --- a/libvirt-utils.h +++ b/libvirt-utils.h @@ -349,4 +349,9 @@ int virPyDictToTypedParams(PyObject *dict, ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); # endif /* LIBVIR_CHECK_VERSION(1, 1, 0) */ +int virPyCpuMapToChar(int cpunum, + PyObject *pycpumap, + unsigned char **cpumapptr, + int *cpumaplen); + #endif /* __LIBVIRT_UTILS_H__ */ -- 2.5.5
On Fri, Oct 28, 2016 at 13:41:09 +0300, Konstantin Neumoin wrote:
All libvirt_virDomainPin* functions do the same thing for convert pycpumap to cpumap, so this patch moves all common logic to new helper - virPyCpuMapToChar.
Signed-off-by: Konstantin Neumoin <kneumoin@virtuozzo.com> --- libvirt-override.c | 131 +++++------------------------------------------------ libvirt-utils.c | 60 ++++++++++++++++++++++++ libvirt-utils.h | 5 ++ 3 files changed, 76 insertions(+), 120 deletions(-)
Nice cleanup.
diff --git a/libvirt-override.c b/libvirt-override.c index fa3e2ca..ba0d87c 100644 --- a/libvirt-override.c +++ b/libvirt-override.c
The usage here looks good to me.
diff --git a/libvirt-utils.c b/libvirt-utils.c index 2bf7519..aaf4bea 100644 --- a/libvirt-utils.c +++ b/libvirt-utils.c @@ -586,3 +586,63 @@ virPyDictToTypedParams(PyObject *dict, return ret; } #endif /* LIBVIR_CHECK_VERSION(1, 1, 0) */ + + +/* virPyCpuMapToChar
virPyCpumapConvert perhaps?
+ * @cpunum: the number of cpus
This is the number of physical cpus of the host we are talking to. It should be noted.
+ * @pycpumap: source Py cpu map
You should note that this should be a python tuple of bools.
+ * @cpumapptr: destination cpu map + * @cpumaplen: destination cpu map length + * + * Helper function to convert a pycpumap to char* + * + * return -1 on error.
Success case is not documented. Also the fact that the proper python error is set in cases when it makes sense is not documented.
+ */ +int +virPyCpuMapToChar(int cpunum, + PyObject *pycpumap, + unsigned char **cpumapptr, + int *cpumaplen) +{ + int tuple_size; + size_t i; + int i_retval = -1;
We usually call this "ret".
+ *cpumapptr = NULL; + + if (!PyTuple_Check(pycpumap)) { + PyErr_SetString(PyExc_TypeError, "Unexpected type, tuple is required"); + goto exit; + } + + *cpumaplen = VIR_CPU_MAPLEN(cpunum); + + if ((tuple_size = PyTuple_Size(pycpumap)) == -1) + goto exit; + + if (VIR_ALLOC_N(*cpumapptr, *cpumaplen) < 0) { + PyErr_NoMemory(); + goto exit; + } + + for (i = 0; i < tuple_size; i++) { + PyObject *flag = PyTuple_GetItem(pycpumap, i); + bool b; + + if (!flag || libvirt_boolUnwrap(flag, &b) < 0) { + VIR_FREE(*cpumapptr); + goto exit; + } + + if (b) + VIR_USE_CPU(*cpumapptr, i); + else + VIR_UNUSE_CPU(*cpumapptr, i); + } + + for (; i < cpunum; i++) + VIR_UNUSE_CPU(*cpumapptr, i); + + i_retval = 0; +exit:
We indent labels by one space in libvirt so that git grep -p works well. Also we have a naming convention for labels passed both on success and error and call them "cleanup". Finally the label is not necessary in this case as you are cleaning up the pointer in the code and thus the label can be avoided completely and you can return -1 directly. This will also avoid the "ret" or "i_retval" variable.
+ return i_retval; +}
Peter
In subject: "Move cpumap conversion code to a common helper" or something more descriptive than "minor clean-up" On Fri, Oct 28, 2016 at 13:41:09 +0300, Konstantin Neumoin wrote:
All libvirt_virDomainPin* functions do the same thing for convert pycpumap to cpumap, so this patch moves all common logic to new helper - virPyCpuMapToChar.
Signed-off-by: Konstantin Neumoin <kneumoin@virtuozzo.com> ---
Peter
If we pass large(more than cpunum) cpu mask to any libvirt_virDomainPin* function, it could leads to crash. So we have to check tuple size in virPyCpuMapToChar and ignore extra tuple members. Signed-off-by: Konstantin Neumoin <kneumoin@virtuozzo.com> --- libvirt-utils.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/libvirt-utils.c b/libvirt-utils.c index aaf4bea..3fc0fdd 100644 --- a/libvirt-utils.c +++ b/libvirt-utils.c @@ -589,7 +589,8 @@ virPyDictToTypedParams(PyObject *dict, /* virPyCpuMapToChar - * @cpunum: the number of cpus + * @cpunum: the number of cpus, only this first elements make sense, + * so others will be ignored(filled by zeros). * @pycpumap: source Py cpu map * @cpumapptr: destination cpu map * @cpumaplen: destination cpu map length @@ -604,7 +605,7 @@ virPyCpuMapToChar(int cpunum, unsigned char **cpumapptr, int *cpumaplen) { - int tuple_size; + int tuple_size, rel_cpumaplen; size_t i; int i_retval = -1; *cpumapptr = NULL; @@ -624,7 +625,9 @@ virPyCpuMapToChar(int cpunum, goto exit; } - for (i = 0; i < tuple_size; i++) { + rel_cpumaplen = MIN(cpunum, tuple_size); + + for (i = 0; i < rel_cpumaplen; i++) { PyObject *flag = PyTuple_GetItem(pycpumap, i); bool b; -- 2.5.5
In subject: "Don't overrun buffer when converting cpumap" perhaps? That would IMHO explain the patch a bit more when looking at shortlog. On Fri, Oct 28, 2016 at 13:41:10 +0300, Konstantin Neumoin wrote:
If we pass large(more than cpunum) cpu mask to any libvirt_virDomainPin* function, it could leads to crash. So we have to check tuple size in virPyCpuMapToChar and ignore extra tuple members.
Signed-off-by: Konstantin Neumoin <kneumoin@virtuozzo.com> --- libvirt-utils.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/libvirt-utils.c b/libvirt-utils.c index aaf4bea..3fc0fdd 100644 --- a/libvirt-utils.c +++ b/libvirt-utils.c @@ -589,7 +589,8 @@ virPyDictToTypedParams(PyObject *dict,
/* virPyCpuMapToChar - * @cpunum: the number of cpus + * @cpunum: the number of cpus, only this first elements make sense, + * so others will be ignored(filled by zeros).
So this sentence belongs to the previous patch and I'd put it below into the text explaining how this variable is treated.
* @pycpumap: source Py cpu map * @cpumapptr: destination cpu map * @cpumaplen: destination cpu map length @@ -604,7 +605,7 @@ virPyCpuMapToChar(int cpunum, unsigned char **cpumapptr, int *cpumaplen) { - int tuple_size; + int tuple_size, rel_cpumaplen; size_t i; int i_retval = -1; *cpumapptr = NULL; @@ -624,7 +625,9 @@ virPyCpuMapToChar(int cpunum, goto exit; }
- for (i = 0; i < tuple_size; i++) { + rel_cpumaplen = MIN(cpunum, tuple_size); + + for (i = 0; i < rel_cpumaplen; i++) {
You can avoid the temporary variable by checking both tuple_size and cpumaplen in the condition.
PyObject *flag = PyTuple_GetItem(pycpumap, i); bool b;
Not visible in the context is the second for loop that clears the rest of the bits from the tuple which exceed "cpumap". This is not necessary any more since you now fill only the first elements. Peter
participants (2)
-
Konstantin Neumoin -
Peter Krempa