5:28 a.m.
*** BLUAAARRRGHB HERE ***
Ján Tomko (4):
Set the number of elements to 0 in virNetwork*Clear
Don't check validity of missing attributes in DNS SRV XML
Remove double space in error messages
Remove redundant free in virNetworkDNSHostDefParseXML
src/conf/network_conf.c | 14 +++++++++-----
1 file changed, 9 insertions(+), 5 deletions(-)
--
1.8.1.5
5:28 a.m.
New subject: [libvirt] [PATCH 1/4] Set the number of elements to 0 in virNetwork*Clear
Leaving it at -1 causes invalid free in virNetworkDefUpdateDNSHost
if virNetworkDNSHostDefParseXML fails and virNetworkDNSHostDefClear
gets called twice.
---
src/conf/network_conf.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
index d616e12..e3e0e89 100644
--- a/src/conf/network_conf.c
+++ b/src/conf/network_conf.c
@@ -136,6 +136,7 @@ virNetworkIpDefClear(virNetworkIpDefPtr def)
while (def->nhosts--)
virNetworkDHCPHostDefClear(&def->hosts[def->nhosts]);
+ def->nhosts = 0;
VIR_FREE(def->hosts);
VIR_FREE(def->tftproot);
@@ -161,6 +162,7 @@ virNetworkDNSHostDefClear(virNetworkDNSHostDefPtr def)
while (def->nnames--)
VIR_FREE(def->names[def->nnames]);
VIR_FREE(def->names);
+ def->nnames = 0;
}
static void
@@ -190,6 +192,7 @@ virNetworkDNSDefClear(virNetworkDNSDefPtr def)
virNetworkDNSSrvDefClear(&def->srvs[def->nsrvs]);
VIR_FREE(def->srvs);
}
+ def->ntxts = def->nhosts = def->nsrvs = 0;
}
static void
@@ -206,6 +209,7 @@ virNetworkForwardDefClear(virNetworkForwardDefPtr def)
virNetworkForwardIfDefClear(&def->ifs[i]);
}
VIR_FREE(def->ifs);
+ def->nifs = def->npfs = 0;
}
void
--
1.8.1.5
5:36 a.m.
New subject: [libvirt] [PATCH 1/4] Set the number of elements to 0 in virNetwork*Clear
On 26.07.2013 12:28, Ján Tomko wrote:
Leaving it at -1 causes invalid free in virNetworkDefUpdateDNSHost
if virNetworkDNSHostDefParseXML fails and virNetworkDNSHostDefClear
gets called twice.
---
src/conf/network_conf.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
index d616e12..e3e0e89 100644
--- a/src/conf/network_conf.c
+++ b/src/conf/network_conf.c
@@ -136,6 +136,7 @@ virNetworkIpDefClear(virNetworkIpDefPtr def)
while (def->nhosts--)
virNetworkDHCPHostDefClear(&def->hosts[def->nhosts]);
+ def->nhosts = 0;
I think we need a sligthly different approach here. What if def->nhost
is 0 before entering the while() loop? It gets decreased to -1 and ...
We need:
while (def->nhosts)
virNetworkDHCPHostDefCelar( .... [--def->nhosts]);
or just use for (i = 0; i < def->nhosts; i++)
theFunction( ...[i]);
Michal
5:54 a.m.
New subject: [libvirt] [PATCH v2] Set the number of elements 0 in virNetwork*Clear
Decrementing it when it was already 0 causes an invalid free
in virNetworkDefUpdateDNSHost if virNetworkDNSHostDefParseXML
fails and virNetworkDNSHostDefClear gets called twice.
virNetworkForwardDefClear left the number untouched even if it
freed all the elements.
---
src/conf/network_conf.c | 21 +++++++++++----------
1 file changed, 11 insertions(+), 10 deletions(-)
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
index d616e12..490b04d 100644
--- a/src/conf/network_conf.c
+++ b/src/conf/network_conf.c
@@ -134,8 +134,8 @@ virNetworkIpDefClear(virNetworkIpDefPtr def)
VIR_FREE(def->family);
VIR_FREE(def->ranges);
- while (def->nhosts--)
- virNetworkDHCPHostDefClear(&def->hosts[def->nhosts]);
+ while (def->nhosts)
+ virNetworkDHCPHostDefClear(&def->hosts[--def->nhosts]);
VIR_FREE(def->hosts);
VIR_FREE(def->tftproot);
@@ -158,8 +158,8 @@ virNetworkDNSTxtDefClear(virNetworkDNSTxtDefPtr def)
static void
virNetworkDNSHostDefClear(virNetworkDNSHostDefPtr def)
{
- while (def->nnames--)
- VIR_FREE(def->names[def->nnames]);
+ while (def->nnames)
+ VIR_FREE(def->names[--def->nnames]);
VIR_FREE(def->names);
}
@@ -176,18 +176,18 @@ static void
virNetworkDNSDefClear(virNetworkDNSDefPtr def)
{
if (def->txts) {
- while (def->ntxts--)
- virNetworkDNSTxtDefClear(&def->txts[def->ntxts]);
+ while (def->ntxts)
+ virNetworkDNSTxtDefClear(&def->txts[--def->ntxts]);
VIR_FREE(def->txts);
}
if (def->hosts) {
- while (def->nhosts--)
- virNetworkDNSHostDefClear(&def->hosts[def->nhosts]);
+ while (def->nhosts)
+ virNetworkDNSHostDefClear(&def->hosts[--def->nhosts]);
VIR_FREE(def->hosts);
}
if (def->srvs) {
- while (def->nsrvs--)
- virNetworkDNSSrvDefClear(&def->srvs[def->nsrvs]);
+ while (def->nsrvs)
+ virNetworkDNSSrvDefClear(&def->srvs[--def->nsrvs]);
VIR_FREE(def->srvs);
}
}
@@ -206,6 +206,7 @@ virNetworkForwardDefClear(virNetworkForwardDefPtr def)
virNetworkForwardIfDefClear(&def->ifs[i]);
}
VIR_FREE(def->ifs);
+ def->nifs = def->npfs = 0;
}
void
--
1.8.1.5
6 a.m.
New subject: [libvirt] [PATCH v2] Set the number of elements 0 in virNetwork*Clear
On 26.07.2013 12:54, Ján Tomko wrote:
Decrementing it when it was already 0 causes an invalid free
in virNetworkDefUpdateDNSHost if virNetworkDNSHostDefParseXML
fails and virNetworkDNSHostDefClear gets called twice.
virNetworkForwardDefClear left the number untouched even if it
freed all the elements.
---
src/conf/network_conf.c | 21 +++++++++++----------
1 file changed, 11 insertions(+), 10 deletions(-)
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
index d616e12..490b04d 100644
--- a/src/conf/network_conf.c
+++ b/src/conf/network_conf.c
@@ -134,8 +134,8 @@ virNetworkIpDefClear(virNetworkIpDefPtr def)
VIR_FREE(def->family);
VIR_FREE(def->ranges);
- while (def->nhosts--)
- virNetworkDHCPHostDefClear(&def->hosts[def->nhosts]);
+ while (def->nhosts)
+ virNetworkDHCPHostDefClear(&def->hosts[--def->nhosts]);
VIR_FREE(def->hosts);
VIR_FREE(def->tftproot);
@@ -158,8 +158,8 @@ virNetworkDNSTxtDefClear(virNetworkDNSTxtDefPtr def)
static void
virNetworkDNSHostDefClear(virNetworkDNSHostDefPtr def)
{
- while (def->nnames--)
- VIR_FREE(def->names[def->nnames]);
+ while (def->nnames)
+ VIR_FREE(def->names[--def->nnames]);
VIR_FREE(def->names);
}
@@ -176,18 +176,18 @@ static void
virNetworkDNSDefClear(virNetworkDNSDefPtr def)
{
if (def->txts) {
- while (def->ntxts--)
- virNetworkDNSTxtDefClear(&def->txts[def->ntxts]);
+ while (def->ntxts)
+ virNetworkDNSTxtDefClear(&def->txts[--def->ntxts]);
VIR_FREE(def->txts);
}
if (def->hosts) {
- while (def->nhosts--)
- virNetworkDNSHostDefClear(&def->hosts[def->nhosts]);
+ while (def->nhosts)
+ virNetworkDNSHostDefClear(&def->hosts[--def->nhosts]);
VIR_FREE(def->hosts);
}
if (def->srvs) {
- while (def->nsrvs--)
- virNetworkDNSSrvDefClear(&def->srvs[def->nsrvs]);
+ while (def->nsrvs)
+ virNetworkDNSSrvDefClear(&def->srvs[--def->nsrvs]);
VIR_FREE(def->srvs);
}
}
@@ -206,6 +206,7 @@ virNetworkForwardDefClear(virNetworkForwardDefPtr def)
virNetworkForwardIfDefClear(&def->ifs[i]);
}
VIR_FREE(def->ifs);
+ def->nifs = def->npfs = 0;
}
void
ACK
Michal
6:05 a.m.
New subject: [libvirt] [PATCH v2] Set the number of elements 0 in virNetwork*Clear
On 07/26/2013 01:00 PM, Michal Privoznik wrote:
On 26.07.2013 12:54, Ján Tomko wrote:
> Decrementing it when it was already 0 causes an invalid free
> in virNetworkDefUpdateDNSHost if virNetworkDNSHostDefParseXML
> fails and virNetworkDNSHostDefClear gets called twice.
>
> virNetworkForwardDefClear left the number untouched even if it
> freed all the elements.
> ---
> src/conf/network_conf.c | 21 +++++++++++----------
> 1 file changed, 11 insertions(+), 10 deletions(-)
ACK
Michal
Thanks, I've pushed the series.
Jan
5:28 a.m.
New subject: [libvirt] [PATCH 2/4] Don't check validity of missing attributes in DNS SRV XML
This fixes a crash if one of them is missing.
https://bugzilla.redhat.com/show_bug.cgi?id=988718
---
src/conf/network_conf.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
index e3e0e89..5faac92 100644
--- a/src/conf/network_conf.c
+++ b/src/conf/network_conf.c
@@ -923,7 +923,7 @@ virNetworkDNSSrvDefParseXML(const char *networkName,
" of network %s"), networkName);
goto error;
}
- if (strlen(def->service) > DNS_RECORD_LENGTH_SRV) {
+ if (def->service && strlen(def->service) > DNS_RECORD_LENGTH_SRV) {
virReportError(VIR_ERR_XML_DETAIL,
_("Service name '%s' in network %s is too long, limit
is %d bytes"),
def->service, networkName, DNS_RECORD_LENGTH_SRV);
@@ -939,7 +939,8 @@ virNetworkDNSSrvDefParseXML(const char *networkName,
}
/* Check whether protocol value is the supported one */
- if (STRNEQ(def->protocol, "tcp") && (STRNEQ(def->protocol,
"udp"))) {
+ if (def->protocol && STRNEQ(def->protocol, "tcp") &&
+ (STRNEQ(def->protocol, "udp"))) {
virReportError(VIR_ERR_XML_DETAIL,
_("Invalid protocol attribute value '%s' "
" in DNS SRV record of network %s"),
--
1.8.1.5
5:44 a.m.
New subject: [libvirt] [PATCH 2/4] Don't check validity of missing attributes in DNS SRV XML
On 26.07.2013 12:28, Ján Tomko wrote:
This fixes a crash if one of them is missing.
https://bugzilla.redhat.com/show_bug.cgi?id=988718
---
src/conf/network_conf.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
index e3e0e89..5faac92 100644
--- a/src/conf/network_conf.c
+++ b/src/conf/network_conf.c
@@ -923,7 +923,7 @@ virNetworkDNSSrvDefParseXML(const char *networkName,
" of network %s"), networkName);
goto error;
}
- if (strlen(def->service) > DNS_RECORD_LENGTH_SRV) {
+ if (def->service && strlen(def->service) > DNS_RECORD_LENGTH_SRV)
{
virReportError(VIR_ERR_XML_DETAIL,
_("Service name '%s' in network %s is too long,
limit is %d bytes"),
def->service, networkName, DNS_RECORD_LENGTH_SRV);
@@ -939,7 +939,8 @@ virNetworkDNSSrvDefParseXML(const char *networkName,
}
/* Check whether protocol value is the supported one */
- if (STRNEQ(def->protocol, "tcp") && (STRNEQ(def->protocol,
"udp"))) {
+ if (def->protocol && STRNEQ(def->protocol, "tcp")
&&
+ (STRNEQ(def->protocol, "udp"))) {
virReportError(VIR_ERR_XML_DETAIL,
_("Invalid protocol attribute value '%s' "
" in DNS SRV record of network %s"),
ACK
Michal
5:28 a.m.
New subject: [libvirt] [PATCH 3/4] Remove double space in error messages
---
src/conf/network_conf.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
index 5faac92..2561c35 100644
--- a/src/conf/network_conf.c
+++ b/src/conf/network_conf.c
@@ -920,7 +920,7 @@ virNetworkDNSSrvDefParseXML(const char *networkName,
if (!(def->service = virXMLPropString(node, "service")) &&
!partialOkay) {
virReportError(VIR_ERR_XML_DETAIL,
_("Missing required service attribute in DNS SRV record
"
- " of network %s"), networkName);
+ "of network %s"), networkName);
goto error;
}
if (def->service && strlen(def->service) > DNS_RECORD_LENGTH_SRV) {
@@ -943,7 +943,7 @@ virNetworkDNSSrvDefParseXML(const char *networkName,
(STRNEQ(def->protocol, "udp"))) {
virReportError(VIR_ERR_XML_DETAIL,
_("Invalid protocol attribute value '%s' "
- " in DNS SRV record of network %s"),
+ "in DNS SRV record of network %s"),
def->protocol, networkName);
goto error;
}
--
1.8.1.5
5:44 a.m.
New subject: [libvirt] [PATCH 3/4] Remove double space in error messages
On 26.07.2013 12:28, Ján Tomko wrote:
---
src/conf/network_conf.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
index 5faac92..2561c35 100644
--- a/src/conf/network_conf.c
+++ b/src/conf/network_conf.c
@@ -920,7 +920,7 @@ virNetworkDNSSrvDefParseXML(const char *networkName,
if (!(def->service = virXMLPropString(node, "service")) &&
!partialOkay) {
virReportError(VIR_ERR_XML_DETAIL,
_("Missing required service attribute in DNS SRV record
"
- " of network %s"), networkName);
+ "of network %s"), networkName);
goto error;
}
if (def->service && strlen(def->service) > DNS_RECORD_LENGTH_SRV)
{
@@ -943,7 +943,7 @@ virNetworkDNSSrvDefParseXML(const char *networkName,
(STRNEQ(def->protocol, "udp"))) {
virReportError(VIR_ERR_XML_DETAIL,
_("Invalid protocol attribute value '%s' "
- " in DNS SRV record of network %s"),
+ "in DNS SRV record of network %s"),
def->protocol, networkName);
goto error;
}
ACK
Michal
5:28 a.m.
New subject: [libvirt] [PATCH 4/4] Remove redundant free in virNetworkDNSHostDefParseXML
ip has to be NULL at this point.
---
src/conf/network_conf.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
index 2561c35..2040d26 100644
--- a/src/conf/network_conf.c
+++ b/src/conf/network_conf.c
@@ -859,7 +859,6 @@ virNetworkDNSHostDefParseXML(const char *networkName,
virReportError(VIR_ERR_XML_DETAIL,
_("Missing IP address in network '%s' DNS HOST
record"),
networkName);
- VIR_FREE(ip);
goto error;
}
--
1.8.1.5
5:46 a.m.
New subject: [libvirt] [PATCH 4/4] Remove redundant free in virNetworkDNSHostDefParseXML
On 26.07.2013 12:28, Ján Tomko wrote:
ip has to be NULL at this point.
---
src/conf/network_conf.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/src/conf/network_conf.c b/src/conf/network_conf.c
index 2561c35..2040d26 100644
--- a/src/conf/network_conf.c
+++ b/src/conf/network_conf.c
@@ -859,7 +859,6 @@ virNetworkDNSHostDefParseXML(const char *networkName,
virReportError(VIR_ERR_XML_DETAIL,
_("Missing IP address in network '%s' DNS HOST
record"),
networkName);
- VIR_FREE(ip);
goto error;
}
ACK
Increasing context size would show the line just above these where we
can clearly see without need to open the network_conf.c that @ip has to
be NULL to take this path. Nevermind.
Michal
6:18 a.m.
On Fri, Jul 26, 2013 at 12:28:20PM +0200, Ján Tomko wrote:
*** BLUAAARRRGHB HERE ***
Ján Tomko (4):
Set the number of elements to 0 in virNetwork*Clear
Don't check validity of missing attributes in DNS SRV XML
Remove double space in error messages
Remove redundant free in virNetworkDNSHostDefParseXML
src/conf/network_conf.c | 14 +++++++++-----
1 file changed, 9 insertions(+), 5 deletions(-)
IMHO you should be including one or more test data files to demonstrate
the crash & fix.
Daniel
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
7:14 a.m.
On 07/26/2013 01:18 PM, Daniel P. Berrange wrote:
On Fri, Jul 26, 2013 at 12:28:20PM +0200, Ján Tomko wrote:
> Ján Tomko (4):
> Set the number of elements to 0 in virNetwork*Clear
> Don't check validity of missing attributes in DNS SRV XML
> Remove double space in error messages
> Remove redundant free in virNetworkDNSHostDefParseXML
>
> src/conf/network_conf.c | 14 +++++++++-----
> 1 file changed, 9 insertions(+), 5 deletions(-)
IMHO you should be including one or more test data files to demonstrate
the crash & fix.
That can't be done with just data files, a new test for network XML sections
is needed.
virNetworkDefUpdateSection seems like the right function to test.
Jan
4137
days inactive
4137
days old
13 comments
3 participants
participants (3)
-
Daniel P. Berrange -
Ján Tomko -
Michal Privoznik