1:11 p.m.
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
1:11 p.m.
New subject: [libvirt] [PATCH 2/2] remote: fix crash on OOM
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
3:14 a.m.
New subject: [libvirt] [PATCH 2/2] remote: fix crash on OOM
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(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
3:16 a.m.
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(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
4:50 a.m.
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(a)veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
4811
days inactive
4812
days old
4 comments
2 participants
participants (2)
-
Daniel Veillard -
Eric Blake