[libvirt] [PATCH 1/3] umlConnectTapDevice: initialize tapfd variable
There is possibility to jump to 'cleanup' label without tapfd variable being initialized. In the label, VIR_FORCE_CLOSE(tapfd) is called which can have fatal consequences. --- src/uml/uml_conf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/uml/uml_conf.c b/src/uml/uml_conf.c index a4088f2..38dcfbb 100644 --- a/src/uml/uml_conf.c +++ b/src/uml/uml_conf.c @@ -109,7 +109,7 @@ umlConnectTapDevice(virConnectPtr conn, const char *bridge) { bool template_ifname = false; - int tapfd; + int tapfd = -1; if (!net->ifname || STRPREFIX(net->ifname, VIR_NET_GENERATED_PREFIX) || -- 1.8.2.1
In bf1fe848 I've introduced 'newName' variable to substitute the old 'const char *name' as previously we had an ugly code there: name = strdup(name); However, some parts of the function were not updated, so they were still calling VIR_FREE(name) instead of VIR_FREE(newName). --- src/conf/nwfilter_params.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/conf/nwfilter_params.c b/src/conf/nwfilter_params.c index ac5f7ed..48b89b7 100644 --- a/src/conf/nwfilter_params.c +++ b/src/conf/nwfilter_params.c @@ -647,13 +647,13 @@ virNWFilterHashTablePut(virNWFilterHashTablePtr table, int copyName) { if (!virHashLookup(table->hashTable, name)) { + char *newName; if (copyName) { - char *newName; if (VIR_STRDUP(newName, name) < 0) return -1; if (VIR_REALLOC_N(table->names, table->nNames + 1) < 0) { - VIR_FREE(name); + VIR_FREE(newName); return -1; } table->names[table->nNames++] = newName; @@ -661,7 +661,7 @@ virNWFilterHashTablePut(virNWFilterHashTablePtr table, if (virHashAddEntry(table->hashTable, name, val) < 0) { if (copyName) { - VIR_FREE(name); + VIR_FREE(newName); table->nNames--; } return -1; -- 1.8.2.1
On 23/05/13 22:05, Michal Privoznik wrote:
In bf1fe848 I've introduced 'newName' variable to substitute the old 'const char *name' as previously we had an ugly code there:
name = strdup(name);
However, some parts of the function were not updated, so they were still calling VIR_FREE(name) instead of VIR_FREE(newName). --- src/conf/nwfilter_params.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/conf/nwfilter_params.c b/src/conf/nwfilter_params.c index ac5f7ed..48b89b7 100644 --- a/src/conf/nwfilter_params.c +++ b/src/conf/nwfilter_params.c @@ -647,13 +647,13 @@ virNWFilterHashTablePut(virNWFilterHashTablePtr table, int copyName) { if (!virHashLookup(table->hashTable, name)) { + char *newName; if (copyName) { - char *newName; if (VIR_STRDUP(newName, name) < 0) return -1;
if (VIR_REALLOC_N(table->names, table->nNames + 1) < 0) { - VIR_FREE(name); + VIR_FREE(newName); return -1; } table->names[table->nNames++] = newName; @@ -661,7 +661,7 @@ virNWFilterHashTablePut(virNWFilterHashTablePtr table,
if (virHashAddEntry(table->hashTable, name, val) < 0) { if (copyName) { - VIR_FREE(name); + VIR_FREE(newName); table->nNames--; } return -1; ACK.
Previously, we were freeing verr fields instead of rerr which we've allocated just a line above. --- src/rpc/virnetmessage.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/rpc/virnetmessage.c b/src/rpc/virnetmessage.c index 4c6703d..f27a236 100644 --- a/src/rpc/virnetmessage.c +++ b/src/rpc/virnetmessage.c @@ -521,13 +521,13 @@ void virNetMessageSaveError(virNetMessageErrorPtr rerr) rerr->level = verr->level; if (verr->str1 && VIR_ALLOC(rerr->str1) == 0 && VIR_STRDUP_QUIET(*rerr->str1, verr->str1) < 0) - VIR_FREE(verr->str1); + VIR_FREE(rerr->str1); if (verr->str2 && VIR_ALLOC(rerr->str2) == 0 && VIR_STRDUP_QUIET(*rerr->str2, verr->str2) < 0) - VIR_FREE(verr->str2); + VIR_FREE(rerr->str2); if (verr->str3 && VIR_ALLOC(rerr->str3) == 0 && VIR_STRDUP_QUIET(*rerr->str3, verr->str3) < 0) - VIR_FREE(verr->str2); + VIR_FREE(rerr->str3); rerr->int1 = verr->int1; rerr->int2 = verr->int2; } else { -- 1.8.2.1
On 23/05/13 22:05, Michal Privoznik wrote:
Previously, we were freeing verr fields instead of rerr which we've allocated just a line above. --- src/rpc/virnetmessage.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/rpc/virnetmessage.c b/src/rpc/virnetmessage.c index 4c6703d..f27a236 100644 --- a/src/rpc/virnetmessage.c +++ b/src/rpc/virnetmessage.c @@ -521,13 +521,13 @@ void virNetMessageSaveError(virNetMessageErrorPtr rerr) rerr->level = verr->level; if (verr->str1 && VIR_ALLOC(rerr->str1) == 0 && VIR_STRDUP_QUIET(*rerr->str1, verr->str1) < 0) - VIR_FREE(verr->str1); + VIR_FREE(rerr->str1); if (verr->str2 && VIR_ALLOC(rerr->str2) == 0 && VIR_STRDUP_QUIET(*rerr->str2, verr->str2) < 0) - VIR_FREE(verr->str2); + VIR_FREE(rerr->str2); if (verr->str3 && VIR_ALLOC(rerr->str3) == 0 && VIR_STRDUP_QUIET(*rerr->str3, verr->str3) < 0) - VIR_FREE(verr->str2); + VIR_FREE(rerr->str3); rerr->int1 = verr->int1; rerr->int2 = verr->int2; } else { ACK
On 23/05/13 22:05, Michal Privoznik wrote:
There is possibility to jump to 'cleanup' label without tapfd variable being initialized. In the label, VIR_FORCE_CLOSE(tapfd) is called which can have fatal consequences. --- src/uml/uml_conf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/uml/uml_conf.c b/src/uml/uml_conf.c index a4088f2..38dcfbb 100644 --- a/src/uml/uml_conf.c +++ b/src/uml/uml_conf.c @@ -109,7 +109,7 @@ umlConnectTapDevice(virConnectPtr conn, const char *bridge) { bool template_ifname = false; - int tapfd; + int tapfd = -1;
if (!net->ifname || STRPREFIX(net->ifname, VIR_NET_GENERATED_PREFIX) || ACK.
participants (2)
-
Michal Privoznik -
Osier Yang