[libvirt] [PATCH 0/3] Resolve a few coverity issues
Mostly introduced by the "domtop" program. Peter Krempa (3): examples: domtop: Fix uninitialized variable use examples: domtop: Avoid leaking memory util: Check return value from virStrToLong* functions examples/domtop/domtop.c | 10 +++------- src/util/virsexpr.c | 6 +++--- src/util/virstring.h | 30 ++++++++++++++++++++---------- src/vbox/vbox_tmpl.c | 4 ++-- src/xen/xs_internal.c | 2 +- src/xenxs/xen_xm.c | 9 +++++++-- 6 files changed, 36 insertions(+), 25 deletions(-) -- 2.0.0
max_id could be used uninitialized in the cleanup section after the domain wasn't found. Discovered by Coverity. --- examples/domtop/domtop.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/examples/domtop/domtop.c b/examples/domtop/domtop.c index 8118793..7661057 100644 --- a/examples/domtop/domtop.c +++ b/examples/domtop/domtop.c @@ -254,7 +254,7 @@ do_top(virConnectPtr conn, { int ret = -1; virDomainPtr dom; - int max_id; + int max_id = 0; int nparams = 0, then_nparams = 0, now_nparams = 0; virTypedParameterPtr then_params = NULL, now_params = NULL; struct sigaction action_stop; -- 2.0.0
Use the virTypedParamsFree unconditionally as it handles NULL well and has the benefit of freeing a typed parameter array even if it wasn't yet assigned, but only allocated. --- examples/domtop/domtop.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/examples/domtop/domtop.c b/examples/domtop/domtop.c index 7661057..4ac7889 100644 --- a/examples/domtop/domtop.c +++ b/examples/domtop/domtop.c @@ -334,12 +334,8 @@ do_top(virConnectPtr conn, ret = 0; cleanup: - if (max_id > 0) { - if (now_nparams > 0) - virTypedParamsFree(now_params, now_nparams * max_id); - if (then_nparams > 0) - virTypedParamsFree(then_params, then_nparams * max_id); - } + virTypedParamsFree(now_params, now_nparams * max_id); + virTypedParamsFree(then_params, then_nparams * max_id); if (dom) virDomainFree(dom); return ret; -- 2.0.0
We do so in the vast majority of places, so there's no problem of adding the attribute to enforce it by the complier and fix a few leftover places. This was originally pointed out by Coverity as a recent change triggered it's warning that our code checked the vast majority of returns from virStrToLong_ui. --- src/util/virsexpr.c | 6 +++--- src/util/virstring.h | 30 ++++++++++++++++++++---------- src/vbox/vbox_tmpl.c | 4 ++-- src/xen/xs_internal.c | 2 +- src/xenxs/xen_xm.c | 9 +++++++-- 5 files changed, 33 insertions(+), 18 deletions(-) diff --git a/src/util/virsexpr.c b/src/util/virsexpr.c index f5eb364..f8a0ccd 100644 --- a/src/util/virsexpr.c +++ b/src/util/virsexpr.c @@ -567,7 +567,7 @@ sexpr_int(const struct sexpr *sexpr, const char *name) if (value) { int val = 0; - virStrToLong_i(value, NULL, 0, &val); + ignore_value(virStrToLong_i(value, NULL, 0, &val)); return val; } return 0; @@ -590,7 +590,7 @@ sexpr_float(const struct sexpr *sexpr, const char *name) if (value) { double val = 0; - virStrToDouble(value, NULL, &val); + ignore_value(virStrToDouble(value, NULL, &val)); return val; } return 0; @@ -613,7 +613,7 @@ sexpr_u64(const struct sexpr *sexpr, const char *name) if (value) { unsigned long long val = 0; - virStrToLong_ull(value, NULL, 0, &val); + ignore_value(virStrToLong_ull(value, NULL, 0, &val)); return val; } return 0; diff --git a/src/util/virstring.h b/src/util/virstring.h index df25441..267fbd0 100644 --- a/src/util/virstring.h +++ b/src/util/virstring.h @@ -51,43 +51,53 @@ char *virArgvToString(const char *const *argv); int virStrToLong_i(char const *s, char **end_ptr, int base, - int *result); + int *result) + ATTRIBUTE_RETURN_CHECK; int virStrToLong_ui(char const *s, char **end_ptr, int base, - unsigned int *result); + unsigned int *result) + ATTRIBUTE_RETURN_CHECK; int virStrToLong_uip(char const *s, char **end_ptr, int base, - unsigned int *result); + unsigned int *result) + ATTRIBUTE_RETURN_CHECK; int virStrToLong_l(char const *s, char **end_ptr, int base, - long *result); + long *result) + ATTRIBUTE_RETURN_CHECK; int virStrToLong_ul(char const *s, char **end_ptr, int base, - unsigned long *result); + unsigned long *result) + ATTRIBUTE_RETURN_CHECK; int virStrToLong_ulp(char const *s, char **end_ptr, int base, - unsigned long *result); + unsigned long *result) + ATTRIBUTE_RETURN_CHECK; int virStrToLong_ll(char const *s, char **end_ptr, int base, - long long *result); + long long *result) + ATTRIBUTE_RETURN_CHECK; int virStrToLong_ull(char const *s, char **end_ptr, int base, - unsigned long long *result); + unsigned long long *result) + ATTRIBUTE_RETURN_CHECK; int virStrToLong_ullp(char const *s, char **end_ptr, int base, - unsigned long long *result); + unsigned long long *result) + ATTRIBUTE_RETURN_CHECK; int virStrToDouble(char const *s, char **end_ptr, - double *result); + double *result) + ATTRIBUTE_RETURN_CHECK; void virSkipSpaces(const char **str) ATTRIBUTE_NONNULL(1); void virSkipSpacesAndBackslash(const char **str) ATTRIBUTE_NONNULL(1); diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index e60a672..a18d7ab 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -2344,8 +2344,8 @@ static void vboxHostDeviceGetXMLDesc(vboxGlobalData *data, virDomainDefPtr def, VBOX_UTF16_TO_UTF8(vendorIdUtf16, &vendorIdUtf8); VBOX_UTF16_TO_UTF8(productIdUtf16, &productIdUtf8); - virStrToLong_ui(vendorIdUtf8, &endptr, 16, &vendorId); - virStrToLong_ui(productIdUtf8, &endptr, 16, &productId); + ignore_value(virStrToLong_ui(vendorIdUtf8, &endptr, 16, &vendorId)); + ignore_value(virStrToLong_ui(productIdUtf8, &endptr, 16, &productId)); def->hostdevs[USBFilterCount]->source.subsys.u.usb.vendor = vendorId; def->hostdevs[USBFilterCount]->source.subsys.u.usb.product = productId; diff --git a/src/xen/xs_internal.c b/src/xen/xs_internal.c index 8ddcd7e..7e2d477 100644 --- a/src/xen/xs_internal.c +++ b/src/xen/xs_internal.c @@ -335,7 +335,7 @@ xenStoreDomainGetVNCPort(virConnectPtr conn, int domid) tmp = virDomainDoStoreQuery(conn, domid, "console/vnc-port"); if (tmp != NULL) { - virStrToLong_i(tmp, NULL, 10, &ret); + ignore_value(virStrToLong_i(tmp, NULL, 10, &ret)); VIR_FREE(tmp); } return ret; diff --git a/src/xenxs/xen_xm.c b/src/xenxs/xen_xm.c index 7eb183e..f6492b5 100644 --- a/src/xenxs/xen_xm.c +++ b/src/xenxs/xen_xm.c @@ -1026,8 +1026,13 @@ xenParseXM(virConfPtr conf, int xendConfigVersion, if (VIR_STRDUP(graphics->data.vnc.keymap, key + 7) < 0) goto cleanup; } else if (STRPREFIX(key, "vncdisplay=")) { - virStrToLong_i(key + 11, NULL, 10, - &graphics->data.vnc.port); + if (virStrToLong_i(key + 11, NULL, 10, + &graphics->data.vnc.port) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("invalid vncdisplay value '%s'"), + key + 11); + goto cleanup; + } graphics->data.vnc.port += 5900; } } else { -- 2.0.0
On 21.7.2014 10:51, Peter Krempa wrote:
Mostly introduced by the "domtop" program.
Peter Krempa (3): examples: domtop: Fix uninitialized variable use examples: domtop: Avoid leaking memory util: Check return value from virStrToLong* functions
examples/domtop/domtop.c | 10 +++------- src/util/virsexpr.c | 6 +++--- src/util/virstring.h | 30 ++++++++++++++++++++---------- src/vbox/vbox_tmpl.c | 4 ++-- src/xen/xs_internal.c | 2 +- src/xenxs/xen_xm.c | 9 +++++++-- 6 files changed, 36 insertions(+), 25 deletions(-)
ACK series, I've also tested and it fixes all coverity issues. Pavel
On 07/21/14 15:04, Pavel Hrdina wrote:
On 21.7.2014 10:51, Peter Krempa wrote:
Mostly introduced by the "domtop" program.
Peter Krempa (3): examples: domtop: Fix uninitialized variable use examples: domtop: Avoid leaking memory util: Check return value from virStrToLong* functions
examples/domtop/domtop.c | 10 +++------- src/util/virsexpr.c | 6 +++--- src/util/virstring.h | 30 ++++++++++++++++++++---------- src/vbox/vbox_tmpl.c | 4 ++-- src/xen/xs_internal.c | 2 +- src/xenxs/xen_xm.c | 9 +++++++-- 6 files changed, 36 insertions(+), 25 deletions(-)
ACK series, I've also tested and it fixes all coverity issues.
Pavel
Pushed; Thanks. Peter
participants (2)
-
Pavel Hrdina -
Peter Krempa