[libvirt] [PATCH 1/2] xen: use typical allocations
The next patch will add a syntax check that flags this usage in xen as awkward - while it was valid memory management, it was very hard to maintain. Swapping to a more traditional allocation may be a bit slower, but easier to understand. * src/xen/xend_internal.c (xenDaemonListDomainsOld): Use two-level allocation, rather than abusing allocation function. (xenDaemonLookupByUUID): Update caller. --- src/xen/xend_internal.c | 42 +++++++++++++++++++----------------------- 1 files changed, 19 insertions(+), 23 deletions(-) diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c index 81ff325..fa39e8b 100644 --- a/src/xen/xend_internal.c +++ b/src/xen/xend_internal.c @@ -753,12 +753,10 @@ xend_wait_for_devices(virConnectPtr xend, const char *name) char ** xenDaemonListDomainsOld(virConnectPtr xend) { - size_t extra = 0; struct sexpr *root = NULL; char **ret = NULL; int count = 0; int i; - char *ptr; struct sexpr *_for_i, *node; root = sexpr_get(xend, "/xend/domain"); @@ -769,32 +767,22 @@ xenDaemonListDomainsOld(virConnectPtr xend) _for_i = _for_i->u.s.cdr, node = _for_i->u.s.car) { if (node->kind != SEXPR_VALUE) continue; - extra += strlen(node->u.value) + 1; count++; } - /* - * We can'tuse the normal allocation routines as we are mixing - * an array of char * at the beginning followed by an array of char - * ret points to the NULL terminated array of char * - * ptr points to the current string after that array but in the same - * allocated block - */ - if (virAlloc((void *)&ptr, - (count + 1) * sizeof(char *) + extra * sizeof(char)) < 0) + if (VIR_ALLOC_N(ret, count + 1) < 0) { + virReportOOMError(); goto error; - - ret = (char **) ptr; - ptr += sizeof(char *) * (count + 1); + } i = 0; for (_for_i = root, node = root->u.s.car; _for_i->kind == SEXPR_CONS; _for_i = _for_i->u.s.cdr, node = _for_i->u.s.car) { if (node->kind != SEXPR_VALUE) continue; - ret[i] = ptr; - strcpy(ptr, node->u.value); - ptr += strlen(node->u.value) + 1; + ret[i] = strdup(node->u.value); + if (!ret[i]) + goto no_memory; i++; } @@ -803,6 +791,12 @@ xenDaemonListDomainsOld(virConnectPtr xend) error: sexpr_free(root); return ret; + +no_memory: + for (i = 0; i < count; i++) + VIR_FREE(ret[i]); + VIR_FREE(ret); + goto error; } @@ -2493,16 +2487,18 @@ xenDaemonLookupByUUID(virConnectPtr conn, const unsigned char *uuid) id = xenDaemonDomainLookupByName_ids(conn, *tmp, &ident[0]); if (id >= 0) { if (!memcmp(uuid, ident, VIR_UUID_BUFLEN)) { - name = strdup(*tmp); - - if (name == NULL) - virReportOOMError(); - + name = *tmp; break; } } tmp++; } + tmp = names; + while (*tmp) { + if (*tmp != name) + VIR_FREE(*tmp); + tmp++; + } VIR_FREE(names); } else { /* New approach for xen >= 3.0.4 */ char *domname = NULL; -- 1.7.4.4
Bug introduced in commit 675464b. On an OOM, this would try to dereference a char* and free the contents as a pointer, which is doomed to failure. Adding a syntax check will prevent mistakes like this in the future. * cfg.mk (sc_prohibit_internal_functions): New syntax check. (exclude_file_name_regexp--sc_prohibit_internal_functions): Add exemptions. * daemon/remote.c (remoteRelayDomainEventIOError) (remoteRelayDomainEventIOErrorReason) (remoteRelayDomainEventGraphics, remoteRelayDomainEventBlockJob): Use correct free function. --- cfg.mk | 11 ++++++++++- daemon/remote.c | 28 ++++++++++++++-------------- 2 files changed, 24 insertions(+), 15 deletions(-) diff --git a/cfg.mk b/cfg.mk index 95c5eff..9f4aa3e 100644 --- a/cfg.mk +++ b/cfg.mk @@ -212,7 +212,7 @@ useless_free_options = \ # y virDomainWatchdogDefFree # n virDrvNodeGetCellsFreeMemory (returns int) # n virDrvNodeGetFreeMemory (returns long long) -# n virFree (dereferences param) +# n virFree - dereferences param # n virFreeError # n virHashFree (takes 2 args) # y virInterfaceDefFree @@ -306,6 +306,12 @@ sc_flags_usage: halt='flags should be unsigned' \ $(_sc_search_regexp) +# Avoid functions that should only be called via macro counterparts. +sc_prohibit_internal_functions: + @prohibit='vir(Free|AllocN?|ReallocN|File(Close|Fclose|Fdopen)) *\(' \ + halt='use VIR_ macros instead of internal functions' \ + $(_sc_search_regexp) + # Avoid functions that can lead to double-close bugs. sc_prohibit_close: @prohibit='([^>.]|^)\<[fp]?close *\(' \ @@ -706,6 +712,9 @@ exclude_file_name_regexp--sc_prohibit_fork_wrappers = \ exclude_file_name_regexp--sc_prohibit_gethostname = ^src/util/util\.c$$ +exclude_file_name_regexp--sc_prohibit_internal_functions = \ + ^src/(util/(memory|util|virfile)\.[hc]|esx/esx_vi\.c)$$ + exclude_file_name_regexp--sc_prohibit_newline_at_end_of_diagnostic = \ ^src/rpc/gendispatch\.pl$$ diff --git a/daemon/remote.c b/daemon/remote.c index 74e759a..245d41c 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -250,8 +250,8 @@ static int remoteRelayDomainEventIOError(virConnectPtr conn ATTRIBUTE_UNUSED, return 0; mem_error: virReportOOMError(); - virFree(data.srcPath); - virFree(data.devAlias); + VIR_FREE(data.srcPath); + VIR_FREE(data.devAlias); return -1; } @@ -296,9 +296,9 @@ static int remoteRelayDomainEventIOErrorReason(virConnectPtr conn ATTRIBUTE_UNUS mem_error: virReportOOMError(); - virFree(data.srcPath); - virFree(data.devAlias); - virFree(data.reason); + VIR_FREE(data.srcPath); + VIR_FREE(data.devAlias); + VIR_FREE(data.reason); return -1; } @@ -374,17 +374,17 @@ static int remoteRelayDomainEventGraphics(virConnectPtr conn ATTRIBUTE_UNUSED, mem_error: virReportOOMError(); - virFree(data.authScheme); - virFree(data.local.node); - virFree(data.local.service); - virFree(data.remote.node); - virFree(data.remote.service); + VIR_FREE(data.authScheme); + VIR_FREE(data.local.node); + VIR_FREE(data.local.service); + VIR_FREE(data.remote.node); + VIR_FREE(data.remote.service); if (data.subject.subject_val != NULL) { for (i = 0 ; i < data.subject.subject_len ; i++) { - virFree(data.subject.subject_val[i].type); - virFree(data.subject.subject_val[i].name); + VIR_FREE(data.subject.subject_val[i].type); + VIR_FREE(data.subject.subject_val[i].name); } - virFree(data.subject.subject_val); + VIR_FREE(data.subject.subject_val); } return -1; } @@ -422,7 +422,7 @@ static int remoteRelayDomainEventBlockJob(virConnectPtr conn ATTRIBUTE_UNUSED, mem_error: virReportOOMError(); - virFree(data.path); + VIR_FREE(data.path); return -1; } -- 1.7.4.4
On Tue, Sep 20, 2011 at 12:11:32PM -0600, Eric Blake wrote:
Bug introduced in commit 675464b. On an OOM, this would try to dereference a char* and free the contents as a pointer, which is doomed to failure.
Adding a syntax check will prevent mistakes like this in the future.
* cfg.mk (sc_prohibit_internal_functions): New syntax check. (exclude_file_name_regexp--sc_prohibit_internal_functions): Add exemptions. * daemon/remote.c (remoteRelayDomainEventIOError) (remoteRelayDomainEventIOErrorReason) (remoteRelayDomainEventGraphics, remoteRelayDomainEventBlockJob): Use correct free function. --- cfg.mk | 11 ++++++++++- daemon/remote.c | 28 ++++++++++++++-------------- 2 files changed, 24 insertions(+), 15 deletions(-)
diff --git a/cfg.mk b/cfg.mk index 95c5eff..9f4aa3e 100644 --- a/cfg.mk +++ b/cfg.mk @@ -212,7 +212,7 @@ useless_free_options = \ # y virDomainWatchdogDefFree # n virDrvNodeGetCellsFreeMemory (returns int) # n virDrvNodeGetFreeMemory (returns long long) -# n virFree (dereferences param) +# n virFree - dereferences param # n virFreeError # n virHashFree (takes 2 args) # y virInterfaceDefFree @@ -306,6 +306,12 @@ sc_flags_usage: halt='flags should be unsigned' \ $(_sc_search_regexp)
+# Avoid functions that should only be called via macro counterparts. +sc_prohibit_internal_functions: + @prohibit='vir(Free|AllocN?|ReallocN|File(Close|Fclose|Fdopen)) *\(' \ + halt='use VIR_ macros instead of internal functions' \ + $(_sc_search_regexp) + # Avoid functions that can lead to double-close bugs. sc_prohibit_close: @prohibit='([^>.]|^)\<[fp]?close *\(' \ @@ -706,6 +712,9 @@ exclude_file_name_regexp--sc_prohibit_fork_wrappers = \
exclude_file_name_regexp--sc_prohibit_gethostname = ^src/util/util\.c$$
+exclude_file_name_regexp--sc_prohibit_internal_functions = \ + ^src/(util/(memory|util|virfile)\.[hc]|esx/esx_vi\.c)$$ + exclude_file_name_regexp--sc_prohibit_newline_at_end_of_diagnostic = \ ^src/rpc/gendispatch\.pl$$
diff --git a/daemon/remote.c b/daemon/remote.c index 74e759a..245d41c 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -250,8 +250,8 @@ static int remoteRelayDomainEventIOError(virConnectPtr conn ATTRIBUTE_UNUSED, return 0; mem_error: virReportOOMError(); - virFree(data.srcPath); - virFree(data.devAlias); + VIR_FREE(data.srcPath); + VIR_FREE(data.devAlias); return -1; }
@@ -296,9 +296,9 @@ static int remoteRelayDomainEventIOErrorReason(virConnectPtr conn ATTRIBUTE_UNUS
mem_error: virReportOOMError(); - virFree(data.srcPath); - virFree(data.devAlias); - virFree(data.reason); + VIR_FREE(data.srcPath); + VIR_FREE(data.devAlias); + VIR_FREE(data.reason); return -1; }
@@ -374,17 +374,17 @@ static int remoteRelayDomainEventGraphics(virConnectPtr conn ATTRIBUTE_UNUSED,
mem_error: virReportOOMError(); - virFree(data.authScheme); - virFree(data.local.node); - virFree(data.local.service); - virFree(data.remote.node); - virFree(data.remote.service); + VIR_FREE(data.authScheme); + VIR_FREE(data.local.node); + VIR_FREE(data.local.service); + VIR_FREE(data.remote.node); + VIR_FREE(data.remote.service); if (data.subject.subject_val != NULL) { for (i = 0 ; i < data.subject.subject_len ; i++) { - virFree(data.subject.subject_val[i].type); - virFree(data.subject.subject_val[i].name); + VIR_FREE(data.subject.subject_val[i].type); + VIR_FREE(data.subject.subject_val[i].name); } - virFree(data.subject.subject_val); + VIR_FREE(data.subject.subject_val); } return -1; } @@ -422,7 +422,7 @@ static int remoteRelayDomainEventBlockJob(virConnectPtr conn ATTRIBUTE_UNUSED,
mem_error: virReportOOMError(); - virFree(data.path); + VIR_FREE(data.path); return -1; }
Gahhh, I got it wrong, I wanted to use VIR_FREE and failed to notice my mistake ... ACK, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On Tue, Sep 20, 2011 at 12:11:31PM -0600, Eric Blake wrote:
The next patch will add a syntax check that flags this usage in xen as awkward - while it was valid memory management, it was very hard to maintain. Swapping to a more traditional allocation may be a bit slower, but easier to understand.
* src/xen/xend_internal.c (xenDaemonListDomainsOld): Use two-level allocation, rather than abusing allocation function. (xenDaemonLookupByUUID): Update caller. --- src/xen/xend_internal.c | 42 +++++++++++++++++++----------------------- 1 files changed, 19 insertions(+), 23 deletions(-)
diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c index 81ff325..fa39e8b 100644 --- a/src/xen/xend_internal.c +++ b/src/xen/xend_internal.c @@ -753,12 +753,10 @@ xend_wait_for_devices(virConnectPtr xend, const char *name) char ** xenDaemonListDomainsOld(virConnectPtr xend) { - size_t extra = 0; struct sexpr *root = NULL; char **ret = NULL; int count = 0; int i; - char *ptr; struct sexpr *_for_i, *node;
root = sexpr_get(xend, "/xend/domain"); @@ -769,32 +767,22 @@ xenDaemonListDomainsOld(virConnectPtr xend) _for_i = _for_i->u.s.cdr, node = _for_i->u.s.car) { if (node->kind != SEXPR_VALUE) continue; - extra += strlen(node->u.value) + 1; count++; }
- /* - * We can'tuse the normal allocation routines as we are mixing - * an array of char * at the beginning followed by an array of char - * ret points to the NULL terminated array of char * - * ptr points to the current string after that array but in the same - * allocated block - */ - if (virAlloc((void *)&ptr, - (count + 1) * sizeof(char *) + extra * sizeof(char)) < 0) + if (VIR_ALLOC_N(ret, count + 1) < 0) { + virReportOOMError(); goto error; - - ret = (char **) ptr; - ptr += sizeof(char *) * (count + 1); + }
i = 0; for (_for_i = root, node = root->u.s.car; _for_i->kind == SEXPR_CONS; _for_i = _for_i->u.s.cdr, node = _for_i->u.s.car) { if (node->kind != SEXPR_VALUE) continue; - ret[i] = ptr; - strcpy(ptr, node->u.value); - ptr += strlen(node->u.value) + 1; + ret[i] = strdup(node->u.value); + if (!ret[i]) + goto no_memory; i++; }
@@ -803,6 +791,12 @@ xenDaemonListDomainsOld(virConnectPtr xend) error: sexpr_free(root); return ret; + +no_memory: + for (i = 0; i < count; i++) + VIR_FREE(ret[i]); + VIR_FREE(ret); + goto error; }
@@ -2493,16 +2487,18 @@ xenDaemonLookupByUUID(virConnectPtr conn, const unsigned char *uuid) id = xenDaemonDomainLookupByName_ids(conn, *tmp, &ident[0]); if (id >= 0) { if (!memcmp(uuid, ident, VIR_UUID_BUFLEN)) { - name = strdup(*tmp); - - if (name == NULL) - virReportOOMError(); - + name = *tmp; break; } } tmp++; } + tmp = names; + while (*tmp) { + if (*tmp != name) + VIR_FREE(*tmp); + tmp++; + } VIR_FREE(names); } else { /* New approach for xen >= 3.0.4 */ char *domname = NULL;
ACK, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On Tue, Sep 20, 2011 at 12:11:31PM -0600, Eric Blake wrote:
The next patch will add a syntax check that flags this usage in xen as awkward - while it was valid memory management, it was very hard to maintain. Swapping to a more traditional allocation may be a bit slower, but easier to understand.
BTW I pushed both, I would like to add them to 0.9.6, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
participants (2)
-
Daniel Veillard -
Eric Blake